Closed Bug 1690377 Opened 3 years ago Closed 3 years ago

upgrade NodeJS from v10 to v12 LTS

Categories

(Firefox Build System :: Toolchains, task)

task

Tracking

(firefox91 fixed)

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: dmosedale, Assigned: dmosedale)

References

Details

User Story

How to test locally:

- Apply patch from Phabricator (attachment link in this bug)
- Execute `MOZ_SCM_LEVEL=1 mach bootstrap` to get the Node 12 executables from the try servers
- `mach build`, `mach lint -l eslint`, `mach lint -l eslint --fix` with dirty tree (ie prettier),  etc.

Checklist:

X get patch that fetches and repacks submitted
X debug why toolchains aren't being built
X Get try smoking out existing issues
X Post announcement @nodejs, # build, mailing lists
X sort out local eslint issue
X sort out bc7 failures (ignore: bc7 has lots of known intermittents and shouldn't be using NodeJS)
X figure out what where tests get their version of node (from `mach bootstrap`, try should surface issues)
X revert hard configure requirement of Node 12
X post details of which builds will be affected
X sort out `mach npm` PATH issues that makes `./mach npm run bundle`  fail (spun off to bug 1716257, patch posted)
X verify local dev flow (activity-stream, about-welcome, but just in case...)
X test with an existing Node-10 based node_modules
X keep node10 deps and make base-toolchain reference them
X rebase on top of 1715985 once it's on m-c
X request review

* land 
* post details of what people need to do (for most devs, probably nothing) to @nodejs, #build, mailing lists
** Node users notified to check their deps for things that need an upgrade to run on (there may well be none)

Attachments

(1 file, 3 obsolete files)

NodeJS version 12 LTS becomes will be at the end of its life (ie security bugs no longer will be fixed) on April 30th. We should upgrade both Nightly and ESR to NodeJS version 12 LTS before that happens.

Here's the LTS version support roadmap: https://nodejs.org/en/about/releases/

Note that several external projects are dropping support for Node 10, and in those cases when we do downstream syncs we no longer can run the tests in our CI. I assume there is no ETA yet when this upgrade will be done?

Flags: needinfo?(mh+mozilla)

I'm hoping to put a patch on try for this today. That said, if mhommey would prefer to do it instead, I have no objection. :-)

Assignee: nobody → dmosedale

Can we wait for next cycle?

Flags: needinfo?(mh+mozilla)

Meaning 92 Nightly?

Fine by me.

Flags: needinfo?(mh+mozilla)

But maybe this is also a question for Henrik...

Flags: needinfo?(hskupin)

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #8)

Meaning 92 Nightly?

Yes

Flags: needinfo?(mh+mozilla)

I mean it's fine for us and we will upgrade to Puppeteer 9.1.0 instead. But I wonder why we have to keep an unsupported release of Node.js running on our ESR branch for the next year. Is that safe enough?

Flags: needinfo?(hskupin)

I don't know that it's a huge risk, and in the sense that we don't run web-facing servers on it, but we do trust its certificate handling to download and use various things. On balance, I'd prefer that we land it for 91 so that it can be in the ESR, and thus supported on the Node side and more easily upliftable if necessary.

This also means that if there are sec fixes in packages that no longer support Node 10 but do support Node 12, those will be much more easily upliftable as well.

Flags: needinfo?(mh+mozilla)

What do you think, Mike?

Summary: upgrade NodeJS to v12 LTS before v10 LTS is end-of-lifed → upgrade NodeJS from v10 to v12 LTS
User Story: (updated)
User Story: (updated)

We could keep node 10 for the base-toolchain jobs for esr, while upgrading the other jobs to node 12.

Flags: needinfo?(mh+mozilla)

I have yet to find any docs (either via firefox-source-docs or searchfox/yaml inspection) describing the intent behind base-toolchains versus other toolchains, so the intent behind your suggestion and what consequences it would have are not clear to me. Would you please unpack this in a bit more detail? Thanks!

Flags: needinfo?(mh+mozilla)

I have yet to find any docs (either via firefox-source-docs or searchfox/yaml inspection) describing the intent behind base-toolchains versus other toolchains

"Base toolchain" jobs are CI tasks that assert that Firefox sources are still compatible with "baseline" versions of the platforms we depend on, such as GCC, Clang, and (according to this ticket) NodeJS.
Normally, we decide on that "baseline" version that we want to support based on our supported configurations. However, (IIUC), none of the supported systems mandate that NodeJS 10 is still supported - besides, as Dan linked earlier, NodeJS 10 LTS is already entirely unsupported. If we were to decide on a "base toolchain" for NodeJS, it should be NodeJS 12 LTS.

The part that confuses me is that Mike says that we should keep base-toolchain jobs on Node 10 for ESR. I agree that ESR should still use Node 10 instead of 12 for compatibility, but I'm unsure how changing the Node toolchain in central will affect the Node toolchain in ESR. My mistake, missed the part about changing the toolchain for ESR.

Chatted with Dan a bit in Matrix. A few notes:

  • Above, when Dan mentioned ESR, he was referring to ESR 91, not ESR 78. Perhaps there was some confusion here, as I got tripped up by that.
  • Node 10 is EOL as of April 2021. ESR 78 depends on Node 10. It's unclear whether we should:
    • Leave ESR 78 with Node 10, despite the security risk (it's around until late 2021), or
    • Bump ESR 78 to use Node 12 and handle the compatibility issues (it's unclear how many there could be, or how tough they'll be to track down).
    • :glandium, I'm interested in your thoughts on ^
  • Either way, since toolchain configuration only affects current revisions (right?), it's unclear why a base-toolchain job is needed.
User Story: (updated)
User Story: (updated)
User Story: (updated)
Attachment #9224805 - Attachment is obsolete: true
User Story: (updated)
User Story: (updated)
User Story: (updated)

:nchevobbe, I'm seeing regular jest failures on the node(debugger) target. Some of them appear to be related to acorn:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './dist/acorn' is not defined by "exports" in /builds/worker/checkouts/gecko/devtools/client/debugger/node_modules/pretty-fast/node_modules/acorn/package.json

Others are async timeouts.

https://firefoxci.taskcluster-artifacts.net/c7Z_9rjtRVWaSo_6OqDpEQ/0/public/logs/live_backing.log is the log, and yarn install has an awful lot of warnings. Does anything jump out at you? Can you reproduce these locally?

Flags: needinfo?(nchevobbe)

If anyone has any insights as to what's going on with the bc7 failures in that try push, I'd love to hear them, because in the log I looked at, node didn't appear to be invoked anywhere.

User Story: (updated)

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #20)

:nchevobbe, I'm seeing regular jest failures on the node(debugger) target. Some of them appear to be related to acorn:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './dist/acorn' is not defined by "exports" in /builds/worker/checkouts/gecko/devtools/client/debugger/node_modules/pretty-fast/node_modules/acorn/package.json

Others are async timeouts.

https://firefoxci.taskcluster-artifacts.net/c7Z_9rjtRVWaSo_6OqDpEQ/0/public/logs/live_backing.log is the log, and yarn install has an awful lot of warnings. Does anything jump out at you? Can you reproduce these locally?

I do reproduce locally and found the offender. I'll investigate today

Flags: needinfo?(nchevobbe)

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #22)

If anyone has any insights as to what's going on with the bc7 failures in that try push, I'd love to hear them, because in the log I looked at, node didn't appear to be invoked anywhere.

I added some more retriggers, and there's some different failures as well as more greens - I think what you saw is the currently highly frequent bug 1678102.

There are multiple dimensions to the problem.

  • While node upstream EOLed version 10, some downstreams still support it (like Linux distros).
  • The build system doesn't need a ton of node, and I wouldn't be surprised if building Firefox still works with even node 8. The build itself is not at risk, security-wise, it only uses in-tree stuff, in a controlled manner.
  • Running tests has different requirements wrt node.
  • We'd like to force Firefox developers to use a known-secure version of node.

The way we handle the latter is to make configure fail with older versions. This is already a problem with perfectly valid and secure versions of node 10 (because Linux distros backport the fixes, so they don't have the latest version, but they have all the relevant fixes). Soon enough the latter will be handled by --enable-bootstrap being the default on mozilla-central, and it downloading whatever version CI uses.

As per the above, and also more generally, the version configure requires doesn't have to be the exact version the CI uses (or above). This is true for compilers too. The base toolchain jobs test that: the set of oldest versions of things that configure accepts (not for strictly everything, but there's only so much we can do).

Anyways, as long as building and testing with node 10 works, I'd rather configure keep accepting node 10. Hopefully, --enable-bootstrap will be the default before 12 actually becomes a requirement for tests.

Flags: needinfo?(mh+mozilla)

(In reply to Mike Hommey [:glandium] from comment #25)

Anyways, as long as building and testing with node 10 works, I'd rather configure keep accepting node 10. Hopefully, --enable-bootstrap will be the default before 12 actually becomes a requirement for tests.

Is there a tracker for that work?

As a data point, ESLint is planning on dropping node 10 support end of June/start of July. Babel is meant to be dropping support "soon", though their v8 release has been significantly delayed currently.

With both of these, we tend to need to keep them reasonably up to date to cope with new language features (obviously it depends on exactly what is released when, but just to give you an indication).

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #23)

I do reproduce locally and found the offender. I'll investigate today

Wonderful; thanks!

User Story: (updated)

(In reply to Mark Banner (:standard8) from comment #24)

If anyone has any insights as to what's going on with the bc7 failures in that try push...

I added some more retriggers, and there's some different failures as well as more greens - I think what you saw is the currently highly frequent bug 1678102.

Thanks! Looking through them, it seems like the rational thing to do is ignore the bc7 failures, since it has tons of unknown intermittents, and (so far at least), all the logs I've looked at don't implicate Node in any way. I'll keep an eye on future try builds, though.

(In reply to Mike Hommey [:glandium] from comment #25)

There are multiple dimensions to the problem.

[...]

Thanks for the clear, concise explanation of the key factors in play here.

Anyways, as long as building and testing with node 10 works, I'd rather configure keep accepting node 10. Hopefully, --enable-bootstrap will be the default before 12 actually becomes a requirement for tests.

Do you feel it's important to leave the base toolchain at node 10 also (or as a result of that reasoning)?

Flags: needinfo?(mh+mozilla)
User Story: (updated)
User Story: (updated)
User Story: (updated)
User Story: (updated)

@glandium 2nd question: am I correct in thinking that any tests that get their node from mach bootstrap should have any issues automatically caught by try runs? If not, do you know any folks I should check in with to make sure they're going to be ok?

(In reply to Mark Banner (:standard8) from comment #26)

(In reply to Mike Hommey [:glandium] from comment #25)

Anyways, as long as building and testing with node 10 works, I'd rather configure keep accepting node 10. Hopefully, --enable-bootstrap will be the default before 12 actually becomes a requirement for tests.

Is there a tracker for that work?

bug 1694889, but it will be enabled before all the dependencies are closed. I'm planning to enable on windows and/or mac next week.

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #29)

Do you feel it's important to leave the base toolchain at node 10 also (or as a result of that reasoning)?

Yes, at least for 91.

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #30)

@glandium 2nd question: am I correct in thinking that any tests that get their node from mach bootstrap should have any issues automatically caught by try runs? If not, do you know any folks I should check in with to make sure they're going to be ok?

Try runs would use the same thing as mach bootstrap does, which is the same as what normal builds (not base-toolchain) use.

Flags: needinfo?(mh+mozilla)
Depends on: 1715985
User Story: (updated)
User Story: (updated)
User Story: (updated)
Attachment #9224874 - Attachment is obsolete: true
User Story: (updated)
User Story: (updated)
Attachment #9226728 - Attachment description: Bug 1690377 - Upgrade Node 10 toolchain to Node 12, r?glandium!,Standard8! → Bug 1690377 - Upgrade Node 10 toolchain to Node 12, r?mhentges!,Standard8!
Attachment #9226728 - Attachment description: Bug 1690377 - Upgrade Node 10 toolchain to Node 12, r?mhentges!,Standard8! → Bug 1690377 - Upgrade Node 10 toolchain to Node 12, r?mhentges!,Standard8!,glandium!
Pushed by dmosedale@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fdba1128382
Upgrade Node 10 toolchain to Node 12, r=Standard8,mhentges
Flags: needinfo?(dmosedale)

Not sure why try didn't catch that error. Ah well. New patch fixes.

Flags: needinfo?(dmosedale)
Attachment #9227252 - Attachment is obsolete: true
Attachment #9226728 - Attachment description: Bug 1690377 - Upgrade Node 10 toolchain to Node 12, r?mhentges!,Standard8!,glandium! → Bug 1690377 - Upgrade Node 10 toolchain to Node 12, r?mhentges!,Standard8!,glandium
Pushed by dmosedale@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd54d705a55d
Upgrade Node 10 toolchain to Node 12, r=Standard8,mhentges
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
Regressions: 1717198
See Also: → 1762571
You need to log in before you can comment on or make changes to this bug.