Hi, I’ve been working on a patch for Seahorse to fix a CPU issue. It appears that the IOChannel wasn’t catching the HUP signal anytime it dropped to the CLI to run a SSH command (change a key passphrase or generate a new one). This was causing the main_loop thread to run at 100% CPU until Seahorse was closed. In my patch I catch the HUP and it fixes the error. My questions:
What should I do with the HUP in particular? Anything or just acknowledge it?
What should I do to get this patch over the finish line and merged?
Fixing a CPU bug like this is in my opinion a fairly important thing - I agree with the original issue reporter; I found the report when I noticed my laptop sounded like it was going to take off after generating a key. Is it worthy to include a bit of text in the next release notes mentioning this behavior is corrected? (when it’s merged/fixed)
I got the idea for it code wise just by reading the doc’s but it doesn’t fully explain why the HUP bit is in their and it certainly doesn’t mention that excluding or failing to capture it will cause a race condition. If I understand it correctly or with help from others I’d be happy to amend the docs to be more explicit.
I’d like to continue to work on Seahorse. I find the mix of language interesting. I’ve struggled to get tests written up since it doesn’t have any. Is this my lack of experience with Vala or just a general side effect of the language? I tried mining secrets from some other apps (Geary namely) but I have’t been successful yet. Is it viable to think the application could be ported to something like Rust or Python? Is Rust in particular mature enough from a Gnome perspective to use for this and stay away from an interpreter if it’s used in mobile?
Update: I found the following thread (broken link still - #newbie) http s://discourse.gnome.org/t/cpu-100-due-to-g-io-hup/3464/6 which I hadn’t seen yet. It’s my exact case. So it confirms I need to handle the HUP. I’d just like to know what I’m expected to do with it over IO_IN …
Links are intentionally broken, apparently newbie me is limited to 2 links per post.
I just wanted to bring this to the top. Any chance anyone would be able to mentor me to get this across the finish line and help resolve this issue with me?
Sorry, I can’t really help you as far as getting this merged but I do have two suggestions for you.
From a quick glance, it looks like you are trying to merge four commits, out of which really only one is valid and touches a single line? The other three seem to correct issues you introduced.
If that’s right, then I would suggest that you close that request and re-submit it cleanly. i.e. up to date and a single commit with a message that’s informative and follows their style.
I would also suggest you hold off on any talk of rewriting any project that you wish to contribute to until you’ve actually contributed for a while.
And yes, that you haven’t been able to write test cases for an unfamiliar codebase in an unfamiliar language is not a side effect of either just a lack of experience.
Thank you! I can definitely clean-up the current patch and resubmit the MR as a single one.
Agreed on the rewriting. I mentioned it because of the difficulty I’ve had finding and then creating functioning tests for Vala and the project is partially ported from C to Vala. If anyone has a suggestion on docs or additional code bases they know are good examples I’m all ears and willing to stick it out with Vala but don’t want to spin my wheels too much.
As far as testing goes, I would focus on those things which are easily testable first.
Here’s some examples to get you started.
TestCase.vala
/* TestCase.vala
*
* Originally from libgee : https://wiki.gnome.org/action/show/Projects/Libgee
*
* This copy contains minor modifications.
*
*
* Copyright (C) 2009 Julien Peeters
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 2.1 of the License, or (at your option) any later version.
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
* You should have received a copy of the GNU Lesser General Public
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*
* Author:
* Julien Peeters <contact@julienpeeters.fr>
*/
public abstract class TestCase : Object {
public delegate void TestMethod ();
GLib.TestSuite suite;
/* Need to hold a ref to these */
TestWrapper [] tests = new TestWrapper[0];
public TestCase (string name) {
suite = new GLib.TestSuite(name);
}
public void add_test (string name, owned TestMethod test) {
TestWrapper t = new TestWrapper(name, (owned) test, this);
tests += t;
suite.add(new GLib.TestCase(t.name, t.set_up, t.run, t.tear_down ));
}
/* Called before every test.run */
public virtual void set_up () {
return;
}
/* Called after every test.run */
public virtual void tear_down () {
return;
}
public GLib.TestSuite get_suite () {
return suite;
}
class TestWrapper {
[CCode (notify = false)]
public string name { get; private set; }
TestMethod test;
TestCase test_case;
public TestWrapper (string name, owned TestMethod test, TestCase test_case) {
this.name = name;
this.test = (owned) test;
this.test_case = test_case;
}
public void set_up (void * fixture) {
test_case.set_up();
return;
}
public void run (void * fixture) {
test();
return;
}
public void tear_down (void * fixture) {
test_case.tear_down();
return;
}
}
}
TestSomeFeature.vala
public class TestSomeFeature : TestCase {
public TestSomeFeature () {
base("SomeFeature");
add_test("SomeFeature::test1", test1);
// add_test("SomeFeature::test2", test2);
}
public override void set_up () {
// Set up anything needed for this run
return;
}
public void test1 () {
// assert() that whatever you expect to happen happened
return;
}
public override void tear_down () {
// Release anything created in setup
return;
}
}
TestMain.vala
public static int main (string [] args) {
Test.init(ref args);
TestSuite root = TestSuite.get_root();
root.add_suite(new TestSomeFeature().get_suite());
// root.add_suite(new TestSomeOtherFeature().get_suite());
return Test.run();
}