Detect node.js in configure as an optional dependency

RESOLVED FIXED in Firefox 63

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

As the first step to integrating Node.js in the build system, we want to introduce a configure check that finds the location of a valid Node.js executable. Landing a check for an *optional* Node dependency will enable us to flush out problems with the configure check before we make it mandatory.
The intent is for the build system to soon require Node.js to build
Firefox. But we aren't ready to make Node.js a build requirement
just yet.

The goal of this commit is to implement configure detection for
Node.js so that we can a) work out detection bugs b) give people a
means to validate system compatibility *before* we throw the switch to
require Node.js.

This commit introduces configure logic for finding a Node.js
executable, resolving its version, and validating its suitability.

By default, if Node.js cannot be found or there is an error resolving
its version, we print some warning messages and move on.

If --enable-nodejs is used (not the default), errors are raised
if Node.js cannot be found or its version isn't suitable.

Once we require Node.js, the added code can likely be simplied.
When writing the code, I went out of my way to make failures as
non-fatal as possible. e.g. normally we'd say that failures to run
`node --version` would be fatal. I'm purposefully trying to not have
this configure check break anyone's environment, even if failure
occurs. Again, the goal is to introduce the configure checks first
in a non-fatal way such that we can debug failures so the flag day
transition is simpler.
Greg, thanks for putting this together!
Comment on attachment 8987678 [details]
Bug 1471028 - Detect Node.js in configure; r?glandium

Mike Hommey [:glandium] has approved the revision.

https://phabricator.services.mozilla.com/D1818
Attachment #8987678 - Flags: review+
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe18414fe685
Detect Node.js in configure; r=glandium
https://hg.mozilla.org/mozilla-central/rev/fe18414fe685
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Posted file build-error.txt
On Win 10 I get this error when I try to build. What is wrong or missing on my side? Backing out this patch let it build again.

On Mac I have no problem.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/0c5ad222605b
fix check for outdated node versions; r=bustage, a=bustage
(In reply to Richard Marti (:Paenglab) from comment #6)
> On Win 10 I get this error when I try to build. What is wrong or missing on
> my side? Backing out this patch let it build again.
> 
> On Mac I have no problem.

It looks like the:

env[b'NODE_DISABLE_COLORS'] = b'1'

in build/moz.configure/node.configure is unusual; all the other `env` modifications in configure don't use binary strings.  Can you try changing it to:

env['NODE_DISABLE_COLORS'] = '1'

and seeing if you can get through configure?
Flags: needinfo?(richard.marti)
  env['NODE_DISABLE_COLORS'] = '1'
doesn't help. Commenting out the line doesn't help either.
Jörg beat me with the reply. ;-)

It doesn't help.
Flags: needinfo?(richard.marti)
The error pointed to line 29:
 0:06.14   File "c:/mozilla-source/comm-central/build/moz.configure/node.configure", line 29, in nodejs_version
 0:06.14     stderr=subprocess.STDOUT)
so making it:
        out = subprocess.check_output([node, '--version'],  <===== removed env=env,
                                      stderr=subprocess.STDOUT)
by removing |env=env| where indicated gets me past the problem.
Depends on: 1473331
(In reply to Jorg K (GMT+2) from comment #11)
> The error pointed to line 29:
>  0:06.14   File
> "c:/mozilla-source/comm-central/build/moz.configure/node.configure", line
> 29, in nodejs_version
>  0:06.14     stderr=subprocess.STDOUT)
> so making it:
>         out = subprocess.check_output([node, '--version'],  <===== removed
> env=env,
>                                       stderr=subprocess.STDOUT)
> by removing |env=env| where indicated gets me past the problem.

This is not the right fix.

In any event, this is obviously not a straightforward fix, so let's take the discussion to bug 1473331.
You need to log in before you can comment on or make changes to this bug.