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: H6DhV56n3Cc

MozReview-Commit-ID: It9IumV141L

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!

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!

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.

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.

(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

(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

Added fixes for MozillaBuild MozReview-Commit-ID: DBUCcGXxM0a

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.

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.

Added code to cope with the fact that the node directory is structured a bit differently on Windows. MozReview-Commit-ID: 8PDuRECtC4M

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

https://hg.mozilla.org/mozilla-central/rev/a1da4af0f87f https://hg.mozilla.org/mozilla-central/rev/719fdec4ac27 https://hg.mozilla.org/mozilla-central/rev/a0e53aa1b0fb https://hg.mozilla.org/mozilla-central/rev/f9e703507607 https://hg.mozilla.org/mozilla-central/rev/ba1040e677cb https://hg.mozilla.org/mozilla-central/rev/cde3d3c1697f