Open Bug 1464123 Opened 7 years ago Updated 2 years ago

[meta] hard-require nodejs in build system

Categories

(Firefox Build System :: General, enhancement, P3)

Product:
Firefox Build System
Component:
General
Version:
3 Branch
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: dmosedale, Unassigned)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: meta)

Attachments

(7 obsolete files)

No description provided.
This is effectively a spin-off of https://bugzilla.mozilla.org/show_bug.cgi?id=1461714#c6, and is based on Nick's work there. There are multiple threads in mozilla.dev.builds discussing this, as well as an Intent-to-Require thread discussing this from February, as well as other more recent threads: https://groups.google.com/forum/#!searchin/mozilla.dev.builds/node%7Csort:date A key thing to note about this bug is that we've adjusted our strategy to try and do things in much smaller pieces, so this patch is merely about having Node be supported by the build infrastructure. No npm packages (nor infrastructure related to them) are part of this bug. That will come along later. Among other bits, this will include: * having configure detect node * setting up the various platforms to be able to use it * modifications to mach to allow node to be invoked and maybe a few other bits as well.
Component: Mach Core → General
Work in progress try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73c0fc3476d43b316d6e68721150ba4f6ad2416 There are a number of different failure modes here (I've already fixed one), tomorrow I'll dig into more of them...
Attachment #8982738 - Flags: feedback?(nalexander)
Attachment #8982738 - Flags: feedback?(gps)
Attachment #8982739 - Flags: feedback?(nalexander)
Attachment #8982739 - Flags: feedback?(gps)
Attachment #8982740 - Flags: feedback?(nalexander)
Attachment #8982740 - Flags: feedback?(gps)
Attachment #8982741 - Flags: feedback?(nalexander)
Attachment #8982741 - Flags: feedback?(gps)
Attachment #8982742 - Flags: feedback?(nalexander)
Attachment #8982742 - Flags: feedback?(gps)
Attachment #8982743 - Flags: feedback?(nalexander)
Attachment #8982743 - Flags: feedback?(gps)
I think this patch are not too far from being ready to go. I still have a few try failures to clean up, and before I land, I'll do these things: * update any relevant wiki pages that I find * post to dev-builds, dev-platform, and firefox-dev warning people that this is about to happen * update "mach bootstrap" for mac (and linux?) We might also want to * add a whitelist about what scripts we're allowing to be executed but I suspect that can happen as part of another bug, if it's something we want to do. Anyway, feedback is entirely welcome on what else needs to happen for this to land!
Comment on attachment 8982738 [details] Bug 1464123 - make generate_buildin_addons handle races better https://reviewboard.mozilla.org/r/248644/#review254870 This one seems to give a slightly better error message for a known race condition in the build. Would you mind filing a bug documenting it?
Comment on attachment 8982739 [details] Bug 1464123 - Add node to configure https://reviewboard.mozilla.org/r/248646/#review254874 This looks pretty good! ::: build/moz.configure/node.configure:11 (Diff revision 1) > + > +# node > +# ==== > + > +# TODO remove node.exe once bug 1382940 addresses ambiguous executables case. > +node = check_prog('NODE', ('node.exe', 'node',), allow_missing=False) Historically, I believe some Linux distros were struggling with whether to name the binary `node` or `nodejs`. It might be worth verifying this works across distros and/or adding in support for checking both forms. This may entail running `<node> -v` on multiple binaries in case `node` isn't node.js. ::: build/moz.configure/node.configure:26 (Diff revision 1) > + out = check_cmd_output(node, '--version', env=env) > + > + match = re.search(r'v(.+)$', out) > + > + if not match: > + raise FatalCheckError('unable to determine node version: %s' % out) This error message should probably print the name of the binary that was executed.
Comment on attachment 8982740 [details] Bug 1464123 - Add node toolchain tasks for linux-x64 and win-x64 https://reviewboard.mozilla.org/r/248648/#review254876 I'm not thrilled about exposing node as a toolchain archive. It feels like something that is more appropriate to bake into the build environment itself. And these days we do that by installing a Debian package in a Docker image. See https://hg.mozilla.org/integration/autoland/rev/4b89260566da and https://hg.mozilla.org/integration/autoland/rev/87840a3dd715 for an example of how to do that. But, uh, actually doing it that way is a bit of work. I'm reluctant to bloat scope on you and say we can defer that to a follow-up. Out of curiosity, why do we need to repackage Node on Windows? What's wrong with the Node in MozillaBuild? That's how we run Python 3.5 in CI. ::: taskcluster/scripts/misc/repack-node.sh:45 (Diff revision 1) > +echo "$SHA256SUM $ARCHIVE" > node.txt > +shasum --algorithm 256 --check node.txt > + > +$UNARCHIVE $ARCHIVE > +mv node-v$VERSION-$ARCH node > +tar caf $UPLOAD_DIR/node.tar.bz2 node Our other toolchains use xz. Please use xz as well.
glandium: do you have an opinion for how we should package Node for CI? i.e. toolchain archive versus Debian package.
Flags: needinfo?(mh+mozilla)
Depends on: 1466574
(In reply to Gregory Szorc [:gps] from comment #11) > Comment on attachment 8982738 [details] > Bug 1464123 - make generate_buildin_addons handle races better > > This one seems to give a slightly better error message for a known race > condition in the build. > > Would you mind filing a bug documenting it? Done in bug 1466574. I've rebased the branch I'm working on, and changed the commit message for that issue to point to the new bug.
Blocks: 1454572
Priority: -- → P1
Blocks: 1466574
No longer depends on: 1466574
Blocks: as-build-meta
No longer blocks: 1466574
Depends on: 1466574
Severity: normal → enhancement
Comment on attachment 8982740 [details] Bug 1464123 - Add node toolchain tasks for linux-x64 and win-x64 https://reviewboard.mozilla.org/r/248648/#review254876 I appreciate your willingness to not bloat the scope here, we definitely want to move as quickly as is reasonable. You're quite right that we don't need to repackage Node on Windows. I'll switch to the MozillaBuild version on Windows. > Our other toolchains use xz. Please use xz as well. Heh, the only reason this was switched from xz to bz2 was because xz was blowing up on Windows. As you've already pointed out, there's no need for a repack on Windows, so I'll put it back.
(In reply to Dan Mosedale (:dmose) from comment #15) > > > > Would you mind filing a bug documenting it? > > Done in bug 1466574. I've rebased the branch I'm working on, and changed > the commit message for that issue to point to the new bug. On Nick's suggestion, I've also just moved the patch over to that bug and requested review on it there.
(In reply to Gregory Szorc [:gps] from comment #14) > glandium: do you have an opinion for how we should package Node for CI? i.e. > toolchain archive versus Debian package. If a backport of a Debian package is enough, a Debian package, but I'm doubtful that'd be enough.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8982738 [details] Bug 1464123 - make generate_buildin_addons handle races better I believe feedback was given here and via other channels. So removing flags.
Attachment #8982738 - Flags: feedback?(nalexander)
Attachment #8982738 - Flags: feedback?(gps)
Attachment #8982739 - Flags: feedback?(nalexander)
Attachment #8982739 - Flags: feedback?(gps)
Attachment #8982740 - Flags: feedback?(nalexander)
Attachment #8982740 - Flags: feedback?(gps)
Attachment #8982741 - Flags: feedback?(nalexander)
Attachment #8982741 - Flags: feedback?(gps)
Attachment #8982742 - Flags: feedback?(nalexander)
Attachment #8982742 - Flags: feedback?(gps)
Attachment #8982743 - Flags: feedback?(nalexander)
Attachment #8982743 - Flags: feedback?(gps)
Iteration: --- → 63.1 - July 9
Depends on: 1470332
Depends on: 1471028
Iteration: 63.1 - July 9 → 63.2 - July 23
Iteration: 63.2 - July 23 → 63.3 - Aug 6
Depends on: 1478995
Depends on: 1481693
No longer depends on: 1470332
Summary: introduce node dependency/infrastructure into build system → make --enable-nodejs true by default
Attachment #8982738 - Attachment is obsolete: true
Attachment #8982739 - Attachment is obsolete: true
Attachment #8982740 - Attachment is obsolete: true
Attachment #8982741 - Attachment is obsolete: true
Attachment #8982742 - Attachment is obsolete: true
Attachment #8982737 - Attachment is obsolete: true
Attachment #8982743 - Attachment is obsolete: true
Summary: make --enable-nodejs true by default → [meta] turn on nodejs by default
Depends on: 1483595
Summary: [meta] turn on nodejs by default → [meta] hard-require nodejs in build system
Depends on: 1482433
Depends on: 1490802
Iteration: 63.3 - Aug 6 → ---
Blocks: 1505119
Keywords: meta
Version: Version 3 → 3 Branch
Assignee: dmose → nobody

Is there a reason why this issue is still open? Isn't nodejs already a hard requirement?

I actually would like to propose a change to the build system which would allow to build the Javascript files and Firefox itself in two separate steps.

This would allow Firefox to be built on platforms which support Rust (mips*, ppc32, ppc64be, riscv64, sparc*) which are supported by the Rust compiler but are not supported by NodeJS or will lose NodeJS support in the near future.

Debian already supports the concept of building non-architecture-dependent code on a different architecture than the architecture-dependent code.

I have explained my reasoning here: https://lists.debian.org/debian-riscv/2020/01/msg00000.html

(In reply to John Paul Adrian Glaubitz from comment #20)

Is there a reason why this issue is still open? Isn't nodejs already a hard requirement?

I see https://bugzilla.mozilla.org/show_bug.cgi?id=1482433, suggesting that it's not already a hard requirement -- although I think that is a lie, and the flag won't allow a regular mach build to succeed. It's their to support certain l10n repackaging tasks.

I actually would like to propose a change to the build system which would allow to build the Javascript files and Firefox itself in two separate steps.

This is basically already possible -- we call such things artifact builds -- although we generally think of it the other way around, with the binaries being built elsewhere. (By binaries I mean executable code -- libxul.so, firefox.exe, etc.)

This would allow Firefox to be built on platforms which support Rust (mips*, ppc32, ppc64be, riscv64, sparc*) which are supported by the Rust compiler but are not supported by NodeJS or will lose NodeJS support in the near future.

Debian already supports the concept of building non-architecture-dependent code on a different architecture than the architecture-dependent code.

I have explained my reasoning here: https://lists.debian.org/debian-riscv/2020/01/msg00000.html

I am personally all for this type of flexibility, although I don't have a good feel on what would actually be required. As I stated above, an artifact build can stick more or less any binaries into a Firefox shell, so the hard part would be teaching the build system to only produce binaries without doing anything that might require Node.js. I.e., you want --disable-nodejs to neuter the front-end of the build system while allowing mach build binaries to succeed. I don't know how hard that might be, or whether it would be suitable to mainline.

Priority: P1 → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: