Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@smashwilson
Copy link
Contributor

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This upgrades our bundled npm from 6.2.0 to the latest release, npm 6.13.0.

Alternate Designs

Because my primary motivations for doing this are to take advantage of node-gyp improvements, I thought about adding a direct dependency on node-gyp, which would let us control our node-gyp version directly moving forward. However, relying on npm's node-gyp dependency means that we benefit from npm's diligence in vetting compatibility and so on, and makes us less likely to neglect to upgrade node-gyp as well as npm in the future.

Benefits

The newer version of node-gyp introduced by this bump - v5.0.5 - features Python 3 compatibility and support for VS2017 and VS2019. Significantly, this will unblock us from being able to test Atom packages on GitHub Actions, whose Windows runners include (at the oldest) VS2017. This should unblock me on atom/github#2298, although I won't know for sure

The newer node-gyp also includes an upstream fix for the hack we used to #499 forever ago, introduced in #673 and #703. This should address Python-related issues like #818 and #698.

Beyond that, this will also allow us to take advantage of other npm improvements as found in their CHANGELOG.

Possible Drawbacks

This is a minor-version dependency bump and the npm client is used by far more people than those who use apm, so it should be quite safe. There's always a risk that we'll break some package or other though.

Verification Process

We have good unit test coverage for the problem described in #499.

Applicable Issues

#499, #818, #698, likely others.

@smashwilson smashwilson mentioned this pull request Nov 17, 2019
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Apr 5, 2020

Hi, I was looking into migrating atom to Python 3 and found this PR.

I see that this PR currently breaks the tests for this repo, but the built apm seems to work properly.

Steps to reproduce:

  • Check out this branch
  • npm install to install dependencies and build apm
  • ./bin/apm install tree-view

Explanation/results of steps to reproduce (demonstrates that apm works):

You can run ./bin/apm install [some-package] e.g. ./bin/apm install tree-view and it works on this branch. (i.e. it successfully installs tree-view to ~/.atom/packages/tree-view). Works with all typical Python configurations, i.e. with python symlinked to python2, python symlinked to python3, or no versionless python whatsoever (assuming there is still at least one of python2 or python3 on the PATH). Tested on Ubuntu 20.04 Beta.

Note:

I can confirm this PR is an improvement over stable/release apm, or master of this repo, as they both do not support python3.

(In stable apm 2.4.3 as bundled with atom 1.45.0, trying to do apm install tree-view only works only with python symlinked to python2. It errors out with python symlinked to python3, or with no versionless python whatsoever.)

See also:

My minimal branch with similar results to this PR (working apm binary, but broken tests): master...DeeDeeG:update-npm

@smashwilson
Copy link
Contributor Author

Yeah, I think the CI on this repo is just haunted at this point 😅 Although the failures on this branch are different from the ones you can get on master, I noticed.

@DeeDeeG

This comment has been minimized.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Apr 29, 2020

Short summary of this comment: I know what's breaking the tests on Python 2, and have patches to fix all but four of the failures (in two very similar/almost identical tests) under Python 2. Many more tests fail on Python 3, and I'm stumped for the moment on how to fix those.


I found out what the issues are that are breaking the tests. (At least under Python 2). Variations on "Node isn't sure what package to use, or where to find it."

  • npm "deduped" away a version of node-gyp, such that the version under the npm directory (node_modeules/npm/node_modules/node-gyp) isn't populated, but the general/shared version (node_modules/node-gyp) is still populated. A test hard-coded the former path, which is empty now, so node-gyp isn't found, so it doesn't run, and the test fails.
    • Solution: Change hard-coded node-gyp path in the tests from the former path to the latter one. See this commit that fixes the issue.
    • Potentially better solution: Find a way to dynamically use node-gyp wherever it's actually located, don't use a hard-coded path. [Edit: Done, see this commit and the docs for require.resolve()
  • The tests seem confused about which version of tar to use. Part of the tests end up calling the 2.x version of tar with syntax that would have worked for version 4.x of tar, but it's the wrong major version. There was a breaking change in command-line syntax starting with version 3.x of tar. Can't figure out what went wrong exactly to cause this???
    • Solution: Get npm to record better info in package-lock.json
      • I can't figure out what steps to do to make this happen, but I was able to do it several times and commit it, so... Problem solved, for now?
    • Even better solution: Use the same major version of tar as npm uses, so node doesn't have to guess which version of tar to use among two mutually-incompatible major versions. (npm is using tar 4.x at the moment. I have a commit to switch to that if folks here would like to do that.)

There's another issue as well (I don't have a patch for this at the moment; I'm not very fluent with JS so it's a lot to pick up at once here):

  • Newer node-gyp verifies that your Python binary is an actual Python binary. So the dummy files we use are ignored, and the tests that check if you can configure a custom Python path fail. The feature works in node-gyp, but we would need to supply a real Python binary as the dummy file to have these tests work. Newer node-gyp also formats its errors differently now, so the tests need to be re-written to account for that.
    • Solution: Re-write these tests in a way that works with newer node-gyp.

These fixes are enough to make all the tests pass using Python 2.7, but several more tests fail when using Python 3 (I'm using Python 3.8.2 for my version of python3). So there would be additional work to track those down. Most/all of these seem somehow obscurely related to node-gyp, but I can't say for sure. I am tempted to try updating all the old pinned dependencies in this repo and see if that fixes the test failures under Python 3.


There's another issue that tends to get fixed automatically by running enough npm install commands:

  • A misformed git dependency URL was committed somehow in package-lock.json.
Could not install from "node_modules/jasmine-focused/jasmine-node@git+https:/github.com/kevinsawicki/jasmine-node.git#81af4f953a2b7dfb5bde8331c05362a4b464c5ef" as it does not contain a package.json file.
  • Solution: Find and replace jasmine-node@git+https://github.com with git+https://github.com in package-lock.json

@smashwilson
Copy link
Contributor Author

Ooh thanks for taking the time to dig in further, @DeeDeeG 😍

Can you create a new pull request with the commits from this ref and yours? You're a lot closer than I was, and I likely won't have time to do a bunch of cherry-picking to take this one over the edge before you could.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Apr 29, 2020

I posted #887 with the minimal-diff/less-adventurous versions of the fixes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants