Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v3.8.1 proposal (became v4.0.0) #1718

Closed
wants to merge 4 commits into from
Closed

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 12, 2019

Edit: @Fishrock123

Please see this comment for the final status of this issue: #1718 (comment)

v4.0.0 is out, now we have to consider the v3.x branch closed, let's move on to master now which I'm rebasing and will put up a v5.0.0 proposal to see what that might look like


v3.8.1 2019-04-12

Due to security concerns this version drops support for Node.js versions < 4.0.0

  • [3578b2abf0] - deps: explicit limit on supported engines (Refael Ackermann)
  • [ec8505e15f] - deps: updated tar package version to 4.4.8 (Pobegaylo Maksim) #1713
  • [6e1e425ffb] - (BREAKING for node < 4) Upgrade to tar v3 (isaacs) #1212
  • [e6699d13cd] - test: fix addon test for Node.js 12 and V8 7.4 (Richard Lau) #1705
  • [0c6bf530a0] - lib: use print() for python version detection (GreenAddress) #1534
Checklist
  • npm install && npm test passes
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

@refack
Copy link
Contributor Author

refack commented Apr 12, 2019

@richardlau
Copy link
Member

fcb0117025 needs the rest of the changes to lib/install.js from https://github.com/nodejs/node-gyp/pull/1212/files#diff-f6618e1cc731d58106a806b7679a7616.

Could you also swap the ordering so that the update to tar 4.4.8 comes last? That way every commit would build in case we'd ever want to bisect.

isaacs and others added 4 commits April 12, 2019 11:14
Tar version 3 performs better and is more well tested than its
predecessor.  npm will be using this in the near future, so there is no
benefit in shipping a node-gyp that uses the slower and less reliable
fstream-based tar.

This drops support for node 0.x, and thus should be considered a
breaking semver-major change.

PR-URL: nodejs#1212
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
PR-URL: nodejs#1713
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor Author

refack commented Apr 12, 2019

Rebased over 6e1e425
https://ci.nodejs.org/job/nodegyp-test-pull-request/122/ ✔️ (for node >= 4)

@stof
Copy link

stof commented Apr 12, 2019

Shouldn't CI jobs for 0.10 and 0.12 be removed from the matrix as support is dropped ?

@refack
Copy link
Contributor Author

refack commented Apr 12, 2019

Shouldn't CI jobs for 0.10 and 0.12 be removed from the matrix as support is dropped ?

Will do, but only after this PR gets some LGTMs.

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • [3578b2abf0] - deps: explicit limit on supported engines (Refael Ackermann)

nit: maybe reword this commit message? The supported engines was bumped in the earlier tar v3 commit.

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

Shouldn't this be a semver-minor or semver-major though, since we're dropping support for Node.js 4?

@@ -32,11 +31,11 @@
"request": "^2.87.0",
"rimraf": "2",
"semver": "~5.3.0",
"tar": "^2.0.0",
"tar": "4",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be?:

Suggested change
"tar": "4",
"tar": "^4.4.8",

Otherwise users without npm audit may still get the unsecure version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only part left in 6bc9bf1 so I'm removing that commit entirely and it'll end up with ^4.4.8

@pumano
Copy link

pumano commented Apr 15, 2019

looks like version should be 4.0.0 according to semver due to breaking changes

@stof stof mentioned this pull request Apr 15, 2019
2 tasks
@jaredbeck
Copy link

Shouldn't this be a semver-minor or semver-major though, since we're dropping support for Node.js 4?

When updating dependencies (in this case node) SemVer does not necessarily require a major release.

What should I do if I update my own dependencies without changing the public API?
That would be considered compatible since it does not affect the public API.
https://semver.org/spec/v2.0.0.html#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

@stevendarby
Copy link

Can someone explain what the hold up is here please?

Tristramg added a commit to CodeursenLiberte/etherpad-lite that referenced this pull request Apr 16, 2019
@BridgeAR
Copy link
Member

Ping @refack

If this includes a breaking change, it should also be a new major version. I believe it should be possible to release a 3.8.1 that just includes the non breaking commits and then to release v4.0.0 right after.

@muxator
Copy link

muxator commented Apr 16, 2019

Hi, would the major change be limited to requiring Node > 4?

If so, it would be acceptable to explain in the changelog (if it's visible enough) that projects that are already on a recent Node version can upgrade without problems. At least for Etherpad (ether/etherpad-lite#3598) this would be ok.

A minor version that only solves the vulnerability would clearly even better (if technically possible).

@ChALkeR
Copy link
Member

ChALkeR commented Apr 23, 2019

If someone could make this happen, it would be the least disruptive.

@xzyfer Perhaps an issue should be filed to https://github.com/npm/node-tar as the starting point, explaining that the usage of older branches is still high and that updating to tar@4.x comes with a major version bump because of dropping support for Node.js < 4.0.

rwd added a commit to europeana/portal.js that referenced this pull request Apr 23, 2019
@xzyfer
Copy link

xzyfer commented Apr 23, 2019

@ChALkeR done in isaacs/node-tar#212

@ChALkeR
Copy link
Member

ChALkeR commented Apr 23, 2019

@xzyfer Thanks! I left a comment there and also pinged npm about that.

@guybedford
Copy link

If there are concerns with releasing this, there is nothing wrong with making it a major release.

@rvagg
Copy link
Member

rvagg commented Apr 24, 2019

v4.0.0 is out, now we have to consider the v3.x branch closed, let's move on to master now which I'm rebasing and will put up a v5.0.0 proposal to see what that might look like

@rvagg rvagg closed this Apr 24, 2019
@rvagg
Copy link
Member

rvagg commented Apr 24, 2019

master and v3.x were a bit of a mess so I've done a rebase and force push to master so you may want to update your local copies. The old master is preserved at pre-v4.0.0-master and a v5.0.0 proposal is up at #1723 for discussion so we can clean up the backlog, it's getting a bit out of hand.

@benwiley4000
Copy link

Thank you for releasing!!

@xzyfer
Copy link

xzyfer commented Apr 24, 2019

Is there any plan to cut a node-gyp release that doesn't break support for Node < 4 if a fix for node-tar@2 can be found?

@rvagg
Copy link
Member

rvagg commented Apr 24, 2019

@xzyfer I think it'd be reasonable to cut another v3.x if you can get an old branch of node-tar released that will work with it

@BridgeAR
Copy link
Member

What about forking tar, fixing the issue in v2 and to use the fork in node-gyp 3 (ideally we do not need a fork and just a PR and a new release for tar@2)? I know it's just a work around but that's at least something we could do for people who can't update yet.

@xzyfer
Copy link

xzyfer commented Apr 24, 2019

No need to fork. The node-tar folks have said they're open to accepting a PR.

@rvagg
Copy link
Member

rvagg commented Apr 24, 2019

we're having a hard enough time maintaining node-gyp, I don't think we really need another codebase to manage even if limited in scope

@mikern12
Copy link

I mentioned this on this thread (1456ef2) , but I am still having an issue when I update node-gyp. I run npm install node-gyp and get the correct 4.0.0 package, but then I get an error that I something changed and I need to "Run npm rebuild node-sass to download the binding for your current environment." When I do this, it re-loads the 3.8.x version of node-gyp. Error below from the rebuild of node-sass:

gyp ERR! node -v v9.4.0
gyp ERR! node-gyp -v v3.8.0
gyp ERR! This is a bug in node-gyp.
gyp ERR! Try to update node-gyp and file an Issue if it does not help:
gyp ERR! https://github.com/nodejs/node-gyp/issues
Build failed with error code: 7
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! node-sass@4.11.0 postinstall: node scripts/build.js
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the node-sass@4.11.0 postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@Fishrock123 Fishrock123 changed the title v3.8.1 proposal v3.8.1 proposal (became v4.0.0) Apr 24, 2019
@Fishrock123
Copy link
Member

I have edited some info into the original post so as to make this less confusing.

@xzyfer
Copy link

xzyfer commented May 15, 2019

node-tar@2.2.2 has been released which patches the vulnerability. Would it be possible to cut release a node-gyp@2.x with this update so that upstreams aren't forced break old node BC?

@xzyfer
Copy link

xzyfer commented May 15, 2019

NVM looks like node-gyp depended on node-tar@^2.0.0 which should mean the issue is resolved.

@erikeckhardt
Copy link

Github is still reporting the vulnerability as existing because NIST's vulnerability report didn't consider 2.2.2 of tar to be acceptable. So, I contacted NIST directly about 2.2.2 not having the vulnerability, and got the following response:

After review of the CVEs, the information provided, and the configurations we have made the appropriate modifications. Please allow up to 24 hours for these changes to populate on the website and in the data feeds.

So stay tuned, because soon tar 2.2.2 will be recognized as an acceptable, non-vulnerable version, and appropriate versions of node-gyp using tar@^2.0.0 will no longer be flagged as vulnerable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet