1202921 - sockit-to-me does not compile on node v4.0.0

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kevin Grandon :kgrandon]

Today nodejs v4.0 was released. When trying to compile sockit-to-me, I'm seeing these errors:

npm ERR! sockit-to-me@0.3.2 install: `./tools/copy.js || node-gyp configure build --verbose`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the sockit-to-me@0.3.2 install script './tools/copy.js || node-gyp configure build --verbose'.
npm ERR! This is most likely a problem with the sockit-to-me package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     ./tools/copy.js || node-gyp configure build --verbose
npm ERR! You can get their info via:
npm ERR!     npm owner ls sockit-to-me
npm ERR! There is likely additional logging output above.

Comment 1

[Kevin Grandon :kgrandon]

Aus, please let me know if you have time to look at this. Thanks!

Comment 2

[Kevin Grandon :kgrandon]

Seems like this is a problem of parsing the proper node version, and I assume also adding files in src/.

make: *** No rule to make target `Release/obj.target/sockit/src/node-0.0./addon.o'

https://github.com/mozilla-b2g/gaia/blob/master/tests/jsmarionette/client/sockit-to-me/binding.gyp#L4

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1202921_sockit_node_v4 > mozilla-b2g:master

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8658915 [details] [review]
[gaia] KevinGrandon:bug_1202921_sockit_node_v4 > mozilla-b2g:master

Hey Aus - I think the only thing we need to do is something like this: https://github.com/mozilla-b2g/gaia/pull/31774/files#diff-fa91d6b39d355de64c9781b67309e58bR429

I wasn't sure how you wanted to handle the version, and kind of feel that we only need to support the latest node.js release, or >= 4.0 now that io.js has merged back to mainline.

Please take a look if you can, thanks!

Comment 5

[Kevin Grandon :kgrandon]

(Also please feel free to steal this bug and obsolete my patch if this is wrong)

Comment 6

[:Eli Perelman]

I think the patch is good, but I think that approach will only work if the major version is bumped every time a new major version of Node.js is released. I think it would make sense for the version number of sockit-to-me to match the version number of Node.js, e.g.

sockit-to-me < 4.0.0 supports Node.js 0.10, 0.12
sockit-to-me 4.0.0 supports Node.js 4.0.0
sockit-to-me 5.0.0 supports Node.js 5.0.0

@kgrandon @auswerk thoughts?

Comment 7

[:Eli Perelman]

(The point of the major version of sockit-to-me matching that of Node.js is so you never have to resort to the version number parsing in the current version of sockit-to-me.)

Comment 8

[Aus Lacroix [:aus]]

(In reply to :Eli Perelman from comment #7)
> (The point of the major version of sockit-to-me matching that of Node.js is
> so you never have to resort to the version number parsing in the current
> version of sockit-to-me.)

This would lead to arbitrary releases being required simply because of new node versions. I'm not too keen on that seeing as it might just work with the next release.

The only reason why sockit-to-me didn't compile with node 4.0.0 is because of a version parsing issue in the gyp file. Not because it became incompatible. 

The patch is fine, as far as moving the source files around and removing the version parsing from the gyp file. I left my comments in the PR but one key thing is that the build folder should not be checked in.

Comment 9

[:Eli Perelman]

@aus understood, I am just hoping that the versioning of sockit-to-me can be consistent and reliable semver-wise. I was assuming that breaking changes in Node.js (e.g. 4.x to 5.x) would also most likely mean a breaking change in this because of its native inclusions, but if you believe that to not be the case then I am fine with different versioning. My biggest request is only that breaking changes in sockit-to-me get major version bumps, e.g. this patch could warrant a 1.0.0, and the next breaking change warrants a 2.0.0, etc.

How does that sound?

Comment 10

[Kevin Grandon :kgrandon]

Comment on attachment 8658915 [details] [review]
[gaia] KevinGrandon:bug_1202921_sockit_node_v4 > mozilla-b2g:master

I've removed the build folder and bumped sockit-to-me to 1.0. I think we should use typical semvar semantics for future upgrades to the sockit-to-me package, and strive to track gaia against the latest stable node version. If it's a problem, we can put a cap on it if needed.

Aus - can you please review when you get a chance? Thanks!

Comment 11

[Aus Lacroix [:aus]]

While I did mention on github that this looked great and to ship it, there are a slew of other issues that need to be addressed.

The problem is that sockit-to-me is a local dependency. Everything on infra is using 0.10 / 0.12 and will *NEED* to use the source from the tree. It will *NOT* be able to fallback to fetching it from elsewhere.

This means that we *MUST* update infra docker images to use node v4 before this can actually land as it will break all of Gij/GijTV everywhere where it runs.

Comment 12

[Aus Lacroix [:aus]]

The images that need to be updated are tester (all gecko builds) and gaia-taskenv (gaia, gaia-master)

Comment 13

[Kevin Grandon :kgrandon]

Any ETA for bug 1208230? I'm trying to use this for a different project that now requires node 4. It's a shame that this is inside the gaia tree and we can't iterate on the module without other dependencies.

If we can't close this by next week I may create a temporary fork of the project, but hoping to avoid that.

Comment 14

[Aus Lacroix [:aus]]

(In reply to Kevin Grandon :kgrandon from comment #13)
> Any ETA for bug 1208230? I'm trying to use this for a different project that
> now requires node 4. It's a shame that this is inside the gaia tree and we
> can't iterate on the module without other dependencies.
> 
> If we can't close this by next week I may create a temporary fork of the
> project, but hoping to avoid that.

We could do conditional compilation like we did before but only support 4 and 0.12. That would certainly take care of it. :) I don't think updating and running on node 4.0 by next week is realistic for the entirety of gaia.

I'd obviously prefer to avoid forking, especially for something this trivial seeming.

Comment 15

[Kevin Grandon :kgrandon]

Aus - are you comfortable publishing changes from my branch as a tagged release? We would not land this in gaia until ready, and anything that wanted to use this would have to explicitly opt into using it. We could publish it under the following version:

1.0.0@beta

If you're ok with this please either publish, or add me as a package collaborator and I will do so. My npm username: kevingrandon

Comment 16

[Aus Lacroix [:aus]]

(In reply to Kevin Grandon :kgrandon from comment #15)
> Aus - are you comfortable publishing changes from my branch as a tagged
> release? We would not land this in gaia until ready, and anything that
> wanted to use this would have to explicitly opt into using it. We could
> publish it under the following version:
> 
> 1.0.0@beta
> 
> If you're ok with this please either publish, or add me as a package
> collaborator and I will do so. My npm username: kevingrandon

Yeah, that's fine, I've added you as a collaborator.

Comment 17

[Kevin Grandon :kgrandon]

Thanks, I've published sockit-to-me@1.0.0-beta1 (in the event that we need/want to publish multiple beta versions).

Comment 18

[Aus Lacroix [:aus]]

FYI, version 1.0.1 is final and supports node ^4 || ^5. Enjoy!

Comment 19

[:Eli Perelman]

Thanks aus! With that, this should be resolved. Please re-open if that's not the case.

Comment 20

[Kevin Grandon :kgrandon]

(In reply to :Eli Perelman from comment #19)
> Thanks aus! With that, this should be resolved. Please re-open if that's not
> the case.

This still hasn't been updated in gaia so I would recommend leaving this open. (Or filing a new bug to track that).

Comment 21

[Aus Lacroix [:aus]]

Indeed, I was leaving it open until bug 1208230 lands (which includes the actual code changes for support for node 4, 5.

I'm hoping to land this over the weekend if I can wrap up updates to taskcluster-npm-cache and npm-cache docker worker image. Right now things would work, but we wouldn't have any node module cache which would slow things down too much (+4 minutes per chunk).

Comment 22

[Aus Lacroix [:aus]]

Fixed thanks to bug 1223661 being resolved.

Comment 23

[Aus Lacroix [:aus]]

Comment on attachment 8658915 [details] [review]
[gaia] KevinGrandon:bug_1202921_sockit_node_v4 > mozilla-b2g:master

This was committed as part of bug 1223661.