Update node-gyp to 7.0.0

[adamretter]

What it does

Fixes: #8332

Updates the version of node-gyp to 7.0.0 to resolve issues with compilation on macOS Catalina. Also resolves issues with compilation when using Python3.

The related discussion is here - #7349 (comment)

How to test

Run the CI.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[adamretter]

@kittaakos Here is the PR for updating node-gyp that I promised.

[vince-fugnitto]

@adamretter thank you for the changes, I have a few comments regarding the pull-request:

Can you be please sure to squash your commits when you are satisfied for a review.
In general we request that a pull-request includes a single commit unless it can be broken down logically.

• https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history

I don't think the extra builds are necessary (2x from previously) and overall it only makes CI and development slower.
There was an original comment made on the issue regarding this point:

I will send a separate PR to increase the number of your Travis CI jobs
Watch out, that might get rejected as we do not want to unnecessarily increase the build time.

• Installation failed with LTS Node version12.16.1 #7349 (comment)

If such CI tests are useful for your use-cases, you can include them in your own pipelines but for the overall project it might not be the case, and due to shared resources it will actually slow down development.

[kittaakos]

Here is the PR for updating node-gyp that I promised.

Thank you, @adamretter. @marechal-p, can you please help with the review? No hurry though, the PR should be on hold until 30.7.

[duncdrum]

@vince-fugnitto currently Theia has a hard dependency on EOL python2, and so do all downstream devs. This PR introduces the possibility to drop python2 and use python3. I see value in theia actually running tests for both python versions here, at least until the transition from 2 to 3 is deemed complete.

[adamretter]

Can you be please sure to squash your commits when you are satisfied for a review.
In general we request that a pull-request includes a single commit unless it can be broken down logically.

@vince-fugnitto Sure of course, I wanted to keep it fat until I got some feedback. As it is easier to remove a commit than edit existing commits.

I don't think the extra builds are necessary (2x from previously) and overall it only makes CI and development slower.

I fear without the CI tests you will have regressions. At the moment Theia does not build on all platforms with both Python 2 and 3, as a project you might not have been aware of that as you don't have appropriate CI tests for that.
This PR fixes that issue and adds tests to prove it is working. If you want to keep both Python 2 and 3 compatibility, I think you must have tests for that, otherwise you can't know if it is working. Otherwise, do you just want to move to Python 3? It's an option but there are a lot of installed Python2 bases out there!

[adamretter]

No hurry though, the PR should be on hold until 30.7.

@kittaakos Does this mean that this PR will not be in the next release? I was really hoping that it would so that updating to the next release will fix the problems we have been experiencing.

[kittaakos]

@kittaakos Does this mean that this PR will not be in the next release?

Correct. You can try to find someone for the review and the verifications, but probably this change won't make it to the next release. As a workaround, you can use resolutions.

[vince-fugnitto]

@adamretter @duncdrum a compromise would be to include one new job which targets python3 so we can test for regressions, but now we have double the jobs straining the CI resources and also making development much slower. We also have theia-apps where CI is run if you're interested in including tests there.

I also don't expect regressions since packages will slowly phase python2 which is EOL.
node-gyp did not support python2 previously and now that it has been updated why do we expect that this will regress?

@kittaakos Does this mean that this PR will not be in the next release? I was really hoping that it would so that updating to the next release will fix the problems we have been experiencing.

If the node-gyp update is critical for the release, you can open a pull-request or update this one which only updates the dependency and I'd be happy to try and review it before the release.

---

cc @marcdumais-work what do you think of doubling all our CI jobs to test python3.

[adamretter]

@adamretter @duncdrum a compromise would be to include one new job which targets python3 so we can test for regressions, but now we have double the jobs straining the CI resources and also making development much slower. We also have theia-apps where CI is run if you're interested in including tests there.

You previously had 4 jobs, I only added 2 more... so not quite double ;-) Regardless, I agree a compromise would be to only test Python 3 on either Linux or Mac, and I can make that the case if you like. It is worth being aware perhaps that we saw different issues on each of those platforms in our testing (before this PR). Let me know if you would like me to reduce the 2 jobs to 1, and if so, should I test for Python3 on Linux or Mac?

I also don't expect regressions since packages will slowly phase python2 which is EOL.
node-gyp did not support python2 previously and now that it has been updated why do we expect that this will regress?

I think you mean "node-gyp DID support"? I guess I am cautious, in my experience things do break upstream when you don't expect them - so I tend to add plenty of CI to ensure no regressions.

If the node-gyp update is critical for the release, you can open a pull-request or update this one which only updates the dependency and I'd be happy to try and review it before the release.

Hopefully if you can advise on the CI issue, and we make the changes, then this can be that PR ;-) Also it's not neccessarily critical as kittaakos pointed out, we can use the resolutions element in the mean time.

[vince-fugnitto]

@adamretter @duncdrum a compromise would be to include one new job which targets python3 so we can test for regressions, but now we have double the jobs straining the CI resources and also making development much slower. We also have theia-apps where CI is run if you're interested in including tests there.

You previously had 4 jobs, I only added 2 more... so not quite double ;-) Regardless, I agree a compromise would be to only test Python 3 on either Linux or Mac, and I can make that the case if you like. It is worth being aware perhaps that we saw different issues on each of those platforms in our testing (before this PR). Let me know if you would like me to reduce the 2 jobs to 1, and if so, should I test for Python3 on Linux or Mac?

We have 4 jobs temporarily at the moment (until node 10 is dropped), but with these changes we've increased to 5 always (reason I was hesitant due to resources and development time). I'll leave others like @marcdumais-work @svenefftinge to make that decision as I shared my opinion for now, I don't think we should increase the number of jobs.

The total CI time increased from 1 hr 38 min 34 sec (master) to 2 hrs 21 min 55 sec (if the jobs are all counted).

[kittaakos]

It is worth being aware perhaps that we saw different issues on each of those platforms in our testing

Building or testing? I thought you had an issues with rebuilding the natives.

[duncdrum]

@kittaakos building, node-gyp has some known oddities and reported issues with different macOS releases. On a clean system / VMs these are easy to avoid, but on real world systems where ppl have other python or node projects it can become messy very quickly.

[kittaakos]

@kittaakos building, node-gyp has some known oddities and reported issues with different macOS releases.

I see; we can run the build with different Python versions if the community agrees, but not the tests. I do not want to test Python2, or Python3, or node-gyp. Do I overlook something?

[adamretter]

@kittaakos The issues were when building Theia using node-gyp and different Python versions and different platforms.

The problem was not so much node-gyp - but Theia's use of a very old node-gyp.

[vince-fugnitto]

I ran a license compatibility check for the updated node-gyp dependency based on our documented steps and everything looks good from that point of view 👍

For the case of the additional jobs, I think a single new linux job (to test building with python3) would be sufficient at this time. I'll open an issue to phase out support for EOL python2 which we can then use to update the CI jobs and documentation to only use python3. For the new job, we should only run the build however and not the test suite as Akos previously mentioned.

[adamretter]

@vince-fugnitto Okay cool thanks. I have pushed that now.

[vince-fugnitto]

A couple of things:

• We previously discussed (Update node-gyp to 7.0.0 #8216 (comment), Update node-gyp to 7.0.0 #8216 (review)) we do not want to run the tests for the new python3 job, but the latest changes still seem to run the suite (https://travis-ci.com/github/eclipse-theia/theia/jobs/364988943)
• Please be sure to fix the ECA and sign-off issue. In addition, we should also properly squash the commits (one commit for the node-gyp bump, and one commit for the new travis job), so the pull-request can be approved and merged.

[vince-fugnitto]

I think simpler names for all the jobs would be sufficient and better as they will be less likely subject to change (since travis controls the default operating system version and it can be updated in the future). In addition, most of the information added to the build name is already displayed:

Something like (Linux - Python2) can be sufficient.

[adamretter]

@vince-fugnitto I think I have addressed your suggestions now - let me know if any more changes are needed. Thanks

[vince-fugnitto]

@vince-fugnitto I think I have addressed your suggestions now - let me know if any more changes are needed. Thanks

The following comment hasn't been resolved in the latest changes: #8216 (comment).
In addition, it looks like the mac build was bumped to use node 12 in this pull-request, what is the reason behind it?

[marcdumais-work]

Minor (optional): reflect in the job name that this one only builds (no tests)?

[marcdumais-work]

LGTM, thanks @adamretter.

I added one small comment, about one of the job's names, that I do not care about strongly enough to block merging - you may address it if you happen to agree.

[vince-fugnitto]

@adamretter sorry, I wasn't notified of the recent changes, do you mind rebasing the pull-request to resolve the conflict and I'll merge?

[akosyakov]

It looks like we still have node-gyp 6.

[adamretter]

@vince-fugnitto I have now rebased this onto master as requested.

@akosyakov I notice that there us dependency on electron-rebuild, which itself also has a dependency on node-gyp 6. I have sent a PR also to that project to resolve that - electron/rebuild#379. In the meantime should I override the dependency from electron-rebuild? or wait for a new release of that project?

[akosyakov]

I see I think you can go on as is if it solves your issues and send another PR when electron builder is upgraded.

[adamretter]

@akosyakov okay great, so this is ready to merge then?

[marcdumais-work]

Ci for Mac had failed, because of a problem with vscode-ripgrep, that seems unrelated to this PR.I have restarted that build.

[akosyakov]

@vince-fugnitto @marcdumais-work Can you merge when you think it is fine?

[marcdumais-work]

Just to be safe, I pushed a copy of this PR on my fork and will let CI do its work there. With the benefit of a configured GH token, I hope CI will pass and confirm the PR is ok.

[marcdumais-work]

Mac CI passed on my fork: https://travis-ci.org/github/marcdumais-work/theia/jobs/717297793, so I think we're good here.
Merging.

[adamretter]

Thanks everybody :-)