GPII-1526: Port native addons to nan #2

javihernandez · 2015-12-08T00:56:09Z

I've ported our native modules to nan, so this is ready for you to start reviewing it.
The only missing piece is the packageKit addon, which is being ported by @klown and will end up in this branch for you to review too.

amb26 · 2015-12-08T17:59:55Z

gpii/node_modules/gsettingsBridge/nodegsettings/nodegsettings.cc

-  args[1]->ToString()->WriteAscii(key);
-  if (args[2]->IsBoolean()) {
-    status = g_settings_set_boolean(settings,key,args[2]->BooleanValue());
+  settings = g_settings_new(*String::Utf8Value(schema));


This would be a good opportunity to fix our long-standing bug due to never deallocating this object. By my reading - we should call g_object_unref, but I may misunderstand - http://stackoverflow.com/questions/2848273/should-a-g-object-new-have-a-matching-g-object-unref
What do you think?

Yes.
Certainly I did in the Gnome Control Panel for the Magnifier, e.g.:
https://git.gnome.org/browse/gnome-control-center/tree/panels/universal-access/zoom-options.c#n438

Actually g_clear_object (&settings) would be better:
https://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#g-clear-object

@amb26, @klown,

ACTUALLY YES, so I've also added some "deallocating" code in both gsettings and packageKit modules.

@javihernandez
I wrote:

Actually g_clear_object (&settings) would be better.

I've updated the packagekit add-on in that regard (nodepackagekit.cc), and pushed it this morning. If you want to keep it in your GPII-1526 branch, you'll have to pull my latest in.

@klown,

I've just merged in your changes

* klown/GPII-1526: GPII-1526: Ported PackageKit addon to nan.

Used g_clear_object() instead of the conditional g_object_unref(). Cleaned up white space.

Conflicts: gpii/node_modules/packagekit/nodepackagekit/nodepackagekit.cc

* klown/GPII-1526: GPII-1526: Ported PackageKit addon to nan. Conflicts: gpii/node_modules/packagekit/nodepackagekit/nodepackagekit.cc

Attempt #2. Conflicts: gpii/node_modules/packagekit/nodepackagekit/nodepackagekit.cc

…lier

amb26 · 2015-12-10T20:16:31Z

gpii/node_modules/gsettingsBridge/nodegsettings/nodegsettings.cc

  for (i = 0; keys[i]; i++) {
-    togo->Set(i,String::New(keys[i]));
+    togo->Set(i,String::NewFromUtf8(isolate, keys[i]));


I don't see this strategy for constructing strings advertised in the public NAN docs - where did you find the recommendation?

What I see in the docs is this: https://github.com/nodejs/nan/blob/master/doc/string_bytes.md#api_nan_encode which seems to avoid the need for the bizarre "Isolate" reference...

Right,

My fault, I saw the weird "Isolate" thing in a few examples from nodejs official addon examples, which wasn't using Nan. In fact, now I'm realizing that would be better to use Nan::New(keys[i]).ToLocalChecked() instead, since internally we're making use of a MaybeLocal type. Is that correct?

@javihernandez
I used Nan::Encode() to construct strings without the need for Isolate, as @amb26 suggested.
Look inside makePac() of nodepackagekit.cc
https://github.com/klown/linux/blob/GPII-1526/gpii/node_modules/packagekit/nodepackagekit/nodepackagekit.cc#L85

I haven't addressed other uses of Isolate in that code yet, though.

I wrote:

I haven't addressed other uses of Isolate in that code yet, though.

I have removed the use of Isolate in nodepackagekit.cc when allocating v8 Arrays and a Boolean. There is still a use in one case with a ThrowError. I'll address that on Mon.

I wrote:

I haven't addressed other uses of Isolate in that code yet, though.

I have removed the use of Isolate in nodepackagekit.cc when allocating v8 Arrays and a
Boolean. There is still a use in one case with a ThrowError. I'll address that on Mon.

Done. There are no more instances of Isolate in nodepackagekit.cc.

Modified ut8StringFromValue() to use Nan string functions. Addresses amb26's comment: amb26#2

Modified makePac() to eliminate using Isolate object for string construction. Addresses amb26's comment: amb26#2

Reomved usage of Isolate with respect to allocating a v8 Array and Boolean.

Modified makePac() to use a loop to get various properties of an individual package.

Removed use of Isolate by the ThrowError() within performAction(). Added unit test of this ThrowError() by calling performAction() such that it fails. Addresses amb26's comment: amb26#2

Also, updated the code to use Local handles when possible.

* klown/GPII-1526: GPII-1526: PackageKit nan port: Remove Isolate. GPII-1526: PackageKit nan port. GPII-1526: PackageKit nan port: Isolate GPII-1526: PackageKit nan port: strings. GPII-1526: Port of PackageKit addon to nan.

Modified performAction(): For unsupported actions, free and NULL-ify allocated data structures prior to throwing an error. amb26#2 (comment)

The test of setting the screen resolution to impossible dimensions was erroneously using the physical size of the display as the impossible resolution. Changed to simply use a resolution of (-1 ,-1). amb26#2

* klown/GPII-1526: GPII-1526: Xrandr add-on nan port: fixed memory leak. GPII-1526: Xrandr add-on nan port: improved unit test. GPII-1526: PackageKit nan port: freed data structures. GPII-1526: XRandr nan port: throws errors instead of exit(). GPII-1526: PackageKit nan port: added comment re: NanUtf8 strings.

klown · 2016-04-28T15:44:09Z

@javihernandez @amb26 A heads up: GPII needs to support Fedora-23/glib-2.46.
I merged all of @javihernandez's changes with mine and re-built everything. nodegsettings.cc does not compile because it depends on glib-2.46 (for g_settings_schema_list_keys()) and Fedora-22 only goes as high as glib-2.44. I can upgrade to Fedora-23 to solve this, no problem; but then so does GPII as a whole (or some other distro that includes glib-2.46).

javihernandez · 2016-04-28T15:59:55Z

@javihernandez @amb26 A heads up: GPII needs to support Fedora-23/glib-2.46.
I merged all of @javihernandez's changes with mine and re-built everything. nodegsettings.cc does not compile because it depends on glib-2.46 (for g_settings_schema_list_keys()) and Fedora-22 only goes as high as glib-2.44. I can upgrade to Fedora-23 to solve this, no problem; but then so does GPII as a whole (or some other distro that includes glib-2.46).

oh, I see, and sorry for that. Do you want me to add backwards compatibility or just do a "heads up" communication to the rest of the team? (mostly thinking about our provisioning system)

amb26 · 2016-04-28T21:06:56Z

I think a headsup is fine. I don't see a need for us to depend on a version of Fedora that will receive EOL before we produce the APCP release.

amb26 · 2016-05-02T17:43:21Z

gpii/node_modules/packagekit/nodepackagekit/nodepackagekit_test.js

@@ -155,6 +155,17 @@ var fluid = require("universal"),
        return matchPkg;
    };

+    pkTests.testPerformActionFailure = function() {


Two linting failures in this function - please ensure to run "grunt lint" before issuing pull - see @the-t-in-rtf work on git commit hooks

Sorry, will fix these as well as the one below.

Just in case, here's the archive of the note:

http://lists.idrc.ocad.ca/pipermail/architecture/2016-April/004051.html

It typically prevents me from committing badly formatted code a few times a week, totally worth it.

Thanks for the link, @the-t-in-rtf !
Aside: as a test, I added the hook to my copy of universal, added a lint violation to gpii.js, and tried to commit it. Nothing happened. It turns out that universal's Gruntfile.js checks only the files under the "gpii", "tests", and "examples" directories. Adding "./*.js" to the list fixed that and reported two errors in gpii.js, besides my cooked-up one. Is there a reason for not linting the files in the top level directory?

Not sure if that was directed at me. My concern there would be cleanly avoid linting node_modules and other content that isn't part of the project. It seems like your approach handles that.

@the-t-in-rtf That was not directed at you, specifically. And, you are correct that we don't want to lint all the third-party material. But, gpii.js, index.js, and possibly package.json are GPII specific, and they aren't being linted (and gpii.js shows lint errors).
However, this is a tangent to this pull request. I'll shut up now, and raise an appropriate JIRA for it.

Set DEBUG flag to true so that progress is printed always. Also, tweaked progressCallback() to tidy up its output, and to flush stdout to ensure that the percentages are printed immediately.

* klown/GPII-1526: GPII-1774: Display progress for PackageKit node add-on. GPII-1526: Fixed lint errors in nodepackagekit unit tests.

javihernandez · 2016-05-06T01:36:42Z

@amb26

I've included the latest changes from Joseph and fixed the implementation of Orca's get/set methods (they return promises only if the settings file doesn't exist, and tries to create it). One question, do the operations that require read/write access to files must be enclosed inside a try/catch block, and rejecting the promise if anything goes wrong?

amb26 · 2016-05-06T09:31:41Z

Yes, they must!

javihernandez · 2016-05-06T11:56:07Z

Yes, they must!

Ok, done

javihernandez · 2016-05-09T10:16:31Z

@amb26,

is there anything else do you want to see happening before accepting this?

amb26 · 2016-05-11T11:24:08Z

gpii/node_modules/orca/orcaSettingsHandler.js

+        };
+
+        if (exists) {
+            try {


Very repetitive. This logic should be captured in the getSettings method itself.
Also, the standard form for a rejection payload is
{
isError: true,
message: "Couldn't get settings: " + err
}

amb26 · 2016-05-16T15:17:00Z

gpii/node_modules/orca/orcaSettingsHandler.js

-                return;
-            }
-        }
+        var getSettings = that.getSettings(payload);


This can all just be
return that.getSettings(payload)

javihernandez added 5 commits December 7, 2015 19:47

GPII-1526: Add nan as a devDependency

9549617

GPII-1526: Poting gsettings addon to Nan

3e8d645

GPII-1526: Removed useless tests.js file under gsettingsBridge folder

90cc2a4

GPII-1526: Ported ALSA addon to nan

3336d29

GPII-1526: Ported XRandR addon to nan

a39071a

javihernandez force-pushed the GPII-1526 branch from ada21f0 to a39071a Compare December 8, 2015 00:59

amb26 reviewed Dec 8, 2015
View reviewed changes

klown and others added 3 commits December 8, 2015 15:45

GPII-1526: Ported PackageKit addon to nan.

9730ffd

GPII-1526: Ported PackageKit addon to nan

f9e26e7

* klown/GPII-1526: GPII-1526: Ported PackageKit addon to nan.

GPII-1526: Freeing some g_objects

02f28c0

javihernandez mentioned this pull request Dec 9, 2015

GPII-1526: Ported PackageKit addon to nan. #3

Closed

klown and others added 7 commits December 9, 2015 09:19

GPII-1526: Ported PackageKit addon to nan.

e751a74

Used g_clear_object() instead of the conditional g_object_unref(). Cleaned up white space.

Merge branch 'javi-GPII-1526' into GPII-1526

94ca3a5

Conflicts: gpii/node_modules/packagekit/nodepackagekit/nodepackagekit.cc

GPII-1526: Use g_object_clear as a replacement of g_object_unref

bba7880

* klown/GPII-1526: GPII-1526: Ported PackageKit addon to nan. Conflicts: gpii/node_modules/packagekit/nodepackagekit/nodepackagekit.cc

GPII-1526: Updated reference to universal in AcceptanceTests.js

d832f0e

Merge branch 'javi-GPII-1526' into GPII-1526

d29d56d

Attempt #2. Conflicts: gpii/node_modules/packagekit/nodepackagekit/nodepackagekit.cc

GPII-1526: Replace fluid's makeChangeApplier with makeHolderChangeApp…

b9d6154

…lier

Merge branch 'javi-GPII-1526' into GPII-1526

0736498

amb26 reviewed Dec 10, 2015
View reviewed changes

klown and others added 8 commits December 11, 2015 12:03

GPII-1526: Port of PackageKit addon to nan.

6edd7f3

Modified ut8StringFromValue() to use Nan string functions. Addresses amb26's comment: amb26#2

GPII-1526: PackageKit nan port: strings.

e6c6119

Modified makePac() to eliminate using Isolate object for string construction. Addresses amb26's comment: amb26#2

GPII-1526: PackageKit nan port: Isolate

d43caeb

Reomved usage of Isolate with respect to allocating a v8 Array and Boolean.

GPII-1526: PackageKit nan port.

fbd1b71

Modified makePac() to use a loop to get various properties of an individual package.

GPII-1526: PackageKit nan port: Remove Isolate.

f9b7c7a

Removed use of Isolate by the ThrowError() within performAction(). Added unit test of this ThrowError() by calling performAction() such that it fails. Addresses amb26's comment: amb26#2

GPII-1526: Removed Isolate

5e025f7

Also, updated the code to use Local handles when possible.

GPII-1526: Use console.log instead of the deprecated util.puts

6dd8caf

GPII-1526: Remove Isolate from PackageKit addon

9f666e3

* klown/GPII-1526: GPII-1526: PackageKit nan port: Remove Isolate. GPII-1526: PackageKit nan port. GPII-1526: PackageKit nan port: Isolate GPII-1526: PackageKit nan port: strings. GPII-1526: Port of PackageKit addon to nan.

klown and others added 4 commits April 25, 2016 12:23

GPII-1526: PackageKit nan port: freed data structures.

072c174

Modified performAction(): For unsupported actions, free and NULL-ify allocated data structures prior to throwing an error. amb26#2 (comment)

GPII-1526: Xrandr add-on nan port: improved unit test.

138c49d

The test of setting the screen resolution to impossible dimensions was erroneously using the physical size of the display as the impossible resolution. Changed to simply use a resolution of (-1 ,-1). amb26#2

GPII-1526: Xrandr add-on nan port: fixed memory leak.

a52700c

amb26 reviewed May 2, 2016
View reviewed changes

Joseph and others added 2 commits May 2, 2016 16:13

GPII-1526: Fixed lint errors in nodepackagekit unit tests.

a58f217

GPII-1774: Display progress for PackageKit node add-on.

79732b5

Set DEBUG flag to true so that progress is printed always. Also, tweaked progressCallback() to tidy up its output, and to flush stdout to ensure that the percentages are printed immediately.

klown mentioned this pull request May 4, 2016

Latest fixes based on Antranig's comments javihernandez/linux#21

Merged

javihernandez added 2 commits May 6, 2016 02:01

GPII-1526: Orca's get/set methods now return promises

b249205

GPII-1526: Joseph's fix for GPII-1774 and some minor linting

40c869c

* klown/GPII-1526: GPII-1774: Display progress for PackageKit node add-on. GPII-1526: Fixed lint errors in nodepackagekit unit tests.

GPII-1526: Added try/catch blocks in case orca settings handler fails

8870197

amb26 reviewed May 11, 2016
View reviewed changes

GPII-1526: WIP promise-based orca settings handler

f62ed1b

amb26 reviewed May 16, 2016
View reviewed changes

javihernandez added 2 commits May 17, 2016 19:09

GPII-1526: Addressed last comments from Antranig wrt Orca

a01ca21

GPII-1526: Use promise.map instead of .then when calling loadSettings

1968794

amb26 merged commit 1968794 into amb26:GPII-1318 May 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPII-1526: Port native addons to nan #2

GPII-1526: Port native addons to nan #2

javihernandez commented Dec 8, 2015

amb26 Dec 8, 2015

klown Dec 8, 2015

klown Dec 8, 2015

javihernandez Dec 9, 2015

klown Dec 9, 2015

javihernandez Dec 9, 2015

amb26 Dec 10, 2015

javihernandez Dec 11, 2015

klown Dec 11, 2015

klown Dec 11, 2015

klown Dec 14, 2015

klown commented Apr 28, 2016

javihernandez commented Apr 28, 2016

amb26 commented Apr 28, 2016

amb26 May 2, 2016

klown May 2, 2016

the-t-in-rtf May 3, 2016

klown May 3, 2016

the-t-in-rtf May 4, 2016 •

edited

klown May 4, 2016

javihernandez commented May 6, 2016

amb26 commented May 6, 2016

javihernandez commented May 6, 2016

javihernandez commented May 9, 2016

amb26 May 11, 2016

amb26 May 16, 2016

GPII-1526: Port native addons to nan #2

GPII-1526: Port native addons to nan #2

Conversation

javihernandez commented Dec 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klown commented Apr 28, 2016

javihernandez commented Apr 28, 2016

amb26 commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-t-in-rtf May 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javihernandez commented May 6, 2016

amb26 commented May 6, 2016

javihernandez commented May 6, 2016

javihernandez commented May 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-t-in-rtf May 4, 2016 •

edited