Closed Bug 1481693 Opened 4 years ago Closed 4 years ago

teach `mach bootstrap` to install NodeJS from artifact toolchains and configure to prefer it

Categories

(Firefox Build System :: Bootstrap Configuration, enhancement, P1)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Iteration:
63.4 - Aug 20
Tracking Status
firefox63 --- fixed

People

(Reporter: dmosedale, Assigned: dmosedale)

References

(Blocks 3 open bugs)

Details

User Story

As a developer, when I type `mach bootstrap --no-system-changes`, the NodeJS version associated with the current tree revision is installed into my ~/.mozbuild sandbox, so that configure finds NodeJS and builds correctly.

As a developer, when I type `mach bootstrap`, both the toolchain version of NodeJS in addition to any non-toolchain version of NodeJS requested by platform-specific parts of the bootstrap process, so that parts of the tree not yet configured to use the ~/.mozbuild version continue to function correctly.

Acceptance criteria:
  * `mach bootstrap --no-system-changes` installs only the toolchain NodeJS and nothing else
  * `mach bootstrap` installs both the toolchain NodeJS and all of the usual packages (possibly including a second version of Node)
  * `configure` prefers the version of NodeJS in ~/.mozbuild to others

Attachments

(6 files, 10 obsolete files)

3.94 KB, patch
gps
: review+
Details | Diff | Splinter Review
4.47 KB, patch
gps
: review+
Details | Diff | Splinter Review
2.04 KB, patch
gps
: review+
Details | Diff | Splinter Review
4.00 KB, patch
gps
: review+
Details | Diff | Splinter Review
14.36 KB, patch
gps
: review+
Details | Diff | Splinter Review
2.04 KB, patch
gps
: review+
Details | Diff | Splinter Review
No description provided.
Iteration: --- → 63.4 - Aug 20
User Story: (updated)
User Story: (updated)
User Story: (updated)
WIP pull-request, for anyone interested in offering thoughts/feedback ...

https://github.com/dmose/gecko/pull/2
MozReview-Commit-ID: 8PDuRECtC4M
MozReview-Commit-ID: DBUCcGXxM0a
MozReview-Commit-ID: AMYM3rAPVcl
MozReview-Commit-ID: XrMVKZmS4T
Comment on attachment 8998927 [details] [diff] [review]
Teach mach bootstrap to install NodeJS from toolchain artifact, r?gps

Review of attachment 8998927 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good!
Attachment #8998927 - Flags: review+
Comment on attachment 8998928 [details] [diff] [review]
Add --no-system-changes argument to 'mach bootstrap', r?gps

Review of attachment 8998928 [details] [diff] [review]:
-----------------------------------------------------------------

This needs a minor bug fix but is otherwise good.

::: python/mozboot/mozboot/base.py
@@ +161,3 @@
>          self.package_manager_updated = False
>          self.no_interactive = no_interactive
> +        self.no_system_changes = False

This always assigns False!
Attachment #8998928 - Flags: review-
Attachment #8998929 - Flags: review+
Comment on attachment 8998930 [details] [diff] [review]
Factor out install_private_packages from moz_bootstrap, r?gps

Review of attachment 8998930 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozboot/mozboot/bootstrap.py
@@ +310,5 @@
> +            self.instance.state_dir = state_dir
> +            self.instance.ensure_stylo_packages(state_dir, checkout_root)
> +            self.instance.ensure_node_packages(state_dir, checkout_root)
> +
> +        return

Nit: this return is not needed since all Python functions `return None` in the absence of a `return` statement.
Attachment #8998930 - Flags: review+
Attachment #8998931 - Flags: review+
Comment on attachment 8998926 [details] [diff] [review]
Teach node.configure to prefer .mozbuild node to PATH, r?gps

Review of attachment 8998926 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand what adding this function in isolation is doing. There is another function of the same name in toolchain.configure. I'm guessing whatever function is defined last will override all previous definitions? So perhaps this definition is overriding the version in toolchain.configure? Or maybe the version in toolchain.configure runs first and then this one runs. Or vice-versa.

Also, this function is referenced by node.configure.

In short, I'm very confused about how this patch changes anything. If it does have the correct intended effect, then it is only doing so as a side-effect of arcane behavior in moz.configure. At the very least, that needs to be documented somewhere. But I would highly prefer for the code to be more explicit. i.e. the thing searching for node should use this function explicitly.
Attachment #8998926 - Flags: review-
Attachment #8999058 - Attachment is obsolete: true
Attachment #8999059 - Attachment is obsolete: true
Attachment #8999060 - Attachment is obsolete: true
Attachment #8999061 - Attachment is obsolete: true
Attachment #8999062 - Attachment is obsolete: true
Attachment #8999063 - Attachment is obsolete: true
(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8998928 [details] [diff] [review]
> Add --no-system-changes argument to 'mach bootstrap', r?gps
> 
> Review of attachment 8998928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This needs a minor bug fix but is otherwise good.
> 
> ::: python/mozboot/mozboot/base.py
> @@ +161,3 @@
> >          self.package_manager_updated = False
> >          self.no_interactive = no_interactive
> > +        self.no_system_changes = False
> 
> This always assigns False!

Whoops; good catch!  Fixed in the updated version.
MozReview-Commit-ID: AMYM3rAPVcl
Attachment #8999217 - Flags: review?(gps)
Attachment #8998928 - Attachment is obsolete: true
(In reply to Gregory Szorc [:gps] from comment #11)
> Comment on attachment 8998926 [details] [diff] [review]
> Teach node.configure to prefer .mozbuild node to PATH, r?gps
> 
> In short, I'm very confused about how this patch changes anything.

Another excellent catch.  In fact, this function previously worked, because I was actually explicitly causing it to be invoke, but I lost a line of the patch somewhere along the way.  I've updated the patch to call it again, and I've renamed the function in order to avoid any inadvertent name collisions.
MozReview-Commit-ID: 8PDuRECtC4M
Attachment #8999233 - Flags: review?(gps)
Attachment #8998926 - Attachment is obsolete: true
Added fixes for MozillaBuild
MozReview-Commit-ID: DBUCcGXxM0a
Attachment #8999234 - Flags: review?(gps)
Attachment #8998927 - Attachment is obsolete: true
Comment on attachment 8999217 [details] [diff] [review]
Add --no-system-changes argument to 'mach bootstrap', v2

Review of attachment 8999217 [details] [diff] [review]:
-----------------------------------------------------------------

bin/bootstrap.py also wants --no-system-changes added to the argument parser. But that can be done as a follow-up.
Attachment #8999217 - Flags: review?(gps) → review+
Comment on attachment 8999233 [details] [diff] [review]
Teach node.configure to prefer .mozbuild node to PATH, v2

Review of attachment 8999233 [details] [diff] [review]:
-----------------------------------------------------------------

You mentioned on IRC that there were issues if both `node` and `nodejs` exist: due to the way traversal works, we apparently search all paths for executable variant A before variant B. That means we could pick up a `nodejs` executable on a later path even if a sooner path had `node` available.

I don't think this is a deal breaker. But we may want to make check_prog() invert the traversal order by default or by request as a follow-up.
Attachment #8999233 - Flags: review?(gps) → review+
Attachment #8999234 - Flags: review?(gps) → review+
Added code to cope with the fact that the node directory is structured a bit differently on Windows.
MozReview-Commit-ID: 8PDuRECtC4M
Attachment #8999233 - Attachment is obsolete: true
Attachment #8999317 - Flags: review+
Pushed by dmosedale@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1da4af0f87f
Teach node.configure to prefer .mozbuild node to PATH, r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/719fdec4ac27
Teach mach bootstrap to install NodeJS from toolchain artifact, r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e53aa1b0fb
Add --no-system-changes argument to 'mach bootstrap', r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e703507607
Factor out try_to_create_state_dir from mach_bootstrap, r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1040e677cb
Factor out install_private_packages from moz_bootstrap, r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/cde3d3c1697f
Implement no_system_changes for moz_bootstrap, r=gps
Depends on: 1482675
Depends on: 1482676
Depends on: 1496708
Blocks: 1643234
You need to log in before you can comment on or make changes to this bug.