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

Update to semver ≥ 7 #2005

Closed
guimard opened this issue Dec 26, 2019 · 14 comments
Closed

Update to semver ≥ 7 #2005

guimard opened this issue Dec 26, 2019 · 14 comments

Comments

@guimard
Copy link
Contributor

guimard commented Dec 26, 2019

  • Node Version: 10.17.0
  • Platform: Linux/Debian
  • Compiler: gcc 9
  • Module:

node-gyp is not compatible with semver ≥ 7. I found this little fix that seems to fix the problem:

--- a/lib/find-python.js
+++ b/lib/find-python.js
@@ -226,7 +226,7 @@
       }
       this.addLog(`- version is "${version}"`)

-      const range = semver.Range(this.semverRange)
+      const range = new semver.Range(this.semverRange)
       var valid = false
       try {
         valid = range.test(version)
@goddessfreya
Copy link

Also affects Arch linux.

@gengjiawen
Copy link
Member

Can you send a PR for this ? Thanks.

guimard added a commit to guimard/node-gyp that referenced this issue Dec 29, 2019
@guimard
Copy link
Contributor Author

guimard commented Dec 29, 2019

Done in #2006 😉

@rvagg
Copy link
Member

rvagg commented Dec 30, 2019

so, we have "semver": "^5.7.1", how is >=7 getting involved and why is this a problem for folks?

@goddessfreya
Copy link

Without the patch above people encounter this error: https://gist.github.com/goddessfreya/90babde3d51401c51ef9e3d717bd5f05#file-gistfile1-txt-L181

@vladimiry
Copy link

vladimiry commented Dec 30, 2019

This seems to be Arch Linux issue. They for some reason decided not to respect the npm version defined in package.json as @rvagg pointed out but use a system-wide installed semver package, see https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/node-gyp#n28:

 # Experimental dedup
  rm -r "$pkgdir"/usr/lib/node_modules/$pkgname/node_modules/semver

So if you download the package archive by clicking https://www.archlinux.org/packages/community/any/node-gyp/download/ the /usr/lib/node_modules/node-gyp/node_modules/semver/ folder will be missed there as they seem to be relying on the system-wide version.

@goddessfreya
Copy link

goddessfreya commented Dec 30, 2019

@vladimiry That explanation does not explain why it is also broken on Debian.

I checked the Debian experimental package and apparently they apply this patch to get node-gyp to work:

Description: fix for semver 7
Author: Xavier Guimard <yadd@debian.org>
Forwarded: <no|not-needed|url proving that it has been forwarded>
Last-Update: 2019-12-26

--- a/lib/find-python.js
+++ b/lib/find-python.js
@@ -226,7 +226,7 @@
       }
       this.addLog(`- version is "${version}"`)
 
-      const range = semver.Range(this.semverRange)
+      const range = new semver.Range(this.semverRange)
       var valid = false
       try {
         valid = range.test(version)

This bug obviously effects more than one distribution.

@vladimiry
Copy link

apparently they apply this patch to get node-gyp to work:

It's because instead of respecting npm versioning they prefer to maintain a single/system-wide set of npm modules that they put to respository. This is, in my opinion, a vicious way when it comes to installing/using the npm modules.

@cclauss
Copy link
Contributor

cclauss commented Dec 30, 2019

Is there are way to do this conditionally so that
const range = semver.Range(this.semverRange) is executed if semver < 7 and const range = new semver.Range(this.semverRange) with new added, is executed if semver >= 7 so that our code continues to support both?

@vladimiry
Copy link

By the way, here is the workaround that should be working on any Linux system where the npm v5.2.0+ package is installed:

  • Uninstalling the node-gyp package from the system.
  • Running npx node-gyp instead of node-gyp. What is the "npx" tool?

And well, it's better not to install/use any npm module from the at least Arch/Debian repositories since they don't respect npm versioning.

@guimard
Copy link
Contributor Author

guimard commented Dec 30, 2019

so, we have "semver": "^5.7.1", how is >=7 getting involved and why is this a problem for folks?

Hi @rvagg,

I'm Debian Developer. For security reason, our policy does not accept embedded code. That's why there is only one semver in our distribution. I posted here my patch, published in our package.

Cheers,
Xavier

@rvagg
Copy link
Member

rvagg commented Dec 30, 2019

what on earth ...

  # Experimental dedup
  rm -r "$pkgdir"/usr/lib/node_modules/$pkgname/node_modules/semver

Linux package managers 🤦‍♂

@guimard I know all about these policies, they're outdated and conflict with Node.js' packaging style and these kinds of problems have been popping up for years. This is no surprise and why I always recommend people (a) never install Node.js from their distro, but use https://github.com/nodesource/distributions instead and (b) never ever use packaged Node.js packages from their distro, because these kinds of breakages are inevitable.

OK, so semver.Range in pre-7 uses the instanceof trick to make new optional: https://github.com/npm/node-semver/blob/v5.7.1/semver.js#L856-L858

In post-7 it uses an ES class, so there's no way to pull off the instanceof trick: https://github.com/npm/node-semver/blob/master/classes/range.js

But this means that we should be able to just stick a new in and it'll be compatible with the current and future version(s) of semver, for this specific instance. I would normally put my foot down and tell Debian & Arch to suffer from the problems their policies create, but this one seems easy enough.

Even with this fix it's extremely probable that using node-gyp (and any other bundled Node.js package) from your Linux distro will make more problems show up that we have zero test coverage for and could be extremely risky--and yes @guimard even "security" concerns because they create these unknown edges where we have zero understanding of the interactions between packages outside of the specific ranges that we test for.

So anyone reading this -- ditch your distro version of node-gyp and anything else you've installed from their and use npm directly to install them, npm install node-gyp -g is easy.

@vladimiry
Copy link

vladimiry commented Dec 30, 2019

For security reason, our policy does not accept embedded code. That's why there is only one semver in our distribution.

Thanks for the info. That must be a pain maintaining npm versioning through the manual patches. I personally think it's very hard to impossible to do not being a core developer of all the involved npm modules. Asking as a curiosity matter, do you use some sort of script that scans all the npm packages in the dist repo and then lists all the cases of npm versioning breaking (automated dependencies versioning breaking detection)?

@guimard
Copy link
Contributor Author

guimard commented Dec 30, 2019

Hi @vladimiry,

for node.js modules packaged in Debian, we never use npm but our dependency system (debian/control)

guimard added a commit to guimard/node-gyp that referenced this issue Dec 30, 2019
@rvagg rvagg closed this as completed in f242ce4 Jan 6, 2020
rvagg pushed a commit that referenced this issue Jan 6, 2020
Fixes: #2005
Reviewed-By: Rod Vagg <rod@vagg.org>
PR-URL: #2006
link2xt pushed a commit to deltachat/deltachat-node that referenced this issue Jan 11, 2020
Otherwise system versions are run and trigger
nodejs/node-gyp#2005 bug which is already
fixed in node-gyp 6.1.0.
Jikstra pushed a commit to deltachat/deltachat-node that referenced this issue Jan 24, 2020
Otherwise system versions are run and trigger
nodejs/node-gyp#2005 bug which is already
fixed in node-gyp 6.1.0.
rvagg pushed a commit that referenced this issue Feb 3, 2020
Fixes: #2005
Reviewed-By: Rod Vagg <rod@vagg.org>
PR-URL: #2006
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants