always require NodeJS by removing --disable-nodejs switch

NEW
Unassigned

Status

--
enhancement
8 months ago
6 months ago

People

(Reporter: dmose, Unassigned)

Tracking

(Blocks: 2 bugs)

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 months ago
Note that we'll first need to figure out another way to make the L10n CI builds work (right now they simply set --disable-nodejs in their mozconfig files).

This is likely to happen early in the 64 nightly timeline.
(Reporter)

Updated

8 months ago
Blocks: 1482436
The source tarball jobs are also using that flag.

Updated

7 months ago
Blocks: 1484361
(Reporter)

Updated

7 months ago
Blocks: 1464123
I think we'll want to logically still keep the ability to run the build system without Node.js to support the special projects and build configurations that don't need it.

What this means is we'll likely want a configure function somewhere for determining whether Node.js is required. We'll internally disable the requirement for things like source tarball "builds."

TBH I'm not sure the best way to implement this. We do have moz.configure files that are per-"project" (e.g. "browser"). But I'm not sure if we can easily have a "shared" moz.configure check that pulls in the results of a function that is defined by each separate project/app/unit/build config. The closest analog we have for this is --disable-compile-environment. But that requires individual builds to add a configure flag to opt in, which is annoying.

We could perhaps still retain the --disable-nodejs flag but make that a fatal error for build configurations that require it (like Firefox).

glandium: do you have any thoughts on how this should be done?
Flags: needinfo?(mh+mozilla)
node.configure is included from toolkit/moz.configure, which is only included for projects that use gecko, so there's no problem here.
Flags: needinfo?(mh+mozilla)
(Reporter)

Comment 4

6 months ago
If the problem is individual builds opting in, is there any existing mechanism for attaching this to classes of builds (eg all L10n builds)?
Yeah, we have plenty of things that respect `--disable-compile-environment`, for example.
I think a fundamental problem here is that we don't explicitly tell configure what we're building: callers can pass various configure flags and then proceed to build firefox, source packages, l10n packages, etc.

Because configure lacks the knowledge to know what the caller is actually requesting, we have to be conservative in rejecting things and/or we end up exposing possibly foot-gunning flags, like --disable-compile-environment and --disable-nodejs.

I think a more user-friendly experience would be to tell configure explicitly what things you intend to build. Then configure can imply options from that. e.g. --with-build=sourcepackage,l10nrepack would imply --disable-compile-environment/--disable-nodejs.

Fixing this seems like a tall order. Yet, doing nothing means `mach build` would encounter an error on the first build action that attempts to use Node.

Perhaps we should add a "verification" step at the top of `mach build` - before the backend is invoked - that inspects the build targets and verifies config.status state is sane?
To be clear, I don't think we should scope bloat this specific bug to tackle larger, long-outstanding issues.

The easiest thing is to do nothing and continue to allow --disable-nodejs to turn off the configure checks for people with workflows that don't require node and don't want to install it. But that will result in some people specifying it and running into build-time errors - possibly minutes into the build - if they do. I wish we didn't have that footgun. But until we have evidence of people falling into this trap, I think it is OK to keep --disable-nodejs around.

i.e. we can start landing build system actions that use Node. If --disable-nodejs is used, they will hit build system errors. As long as any error messages related to missing Node instruct the user to remove --disable-nodejs, I think we'll be fine.

Does this sound reasonable?
(Reporter)

Comment 8

6 months ago
That sounds like a good choice to me.  I'll add --disable-nodejs verbiage into the appropriate action error message.
(Reporter)

Updated

6 months ago
No longer blocks: 1457572
You need to log in before you can comment on or make changes to this bug.