Closed Bug 1364588 Opened 3 years ago Closed 3 years ago

use `mach artifact toolchain' to download clang packages for stylo

Categories

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

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

This has a number of benefits.
Using this command is more robust than our current method, and brings
several benefits, such as smart caching of the downloaded toolchain.  We
change the clang package downloaded for Windows with this change, but
bindgen has been updated to work well with LLVM 5.0, so there should be
no problems.

I think this should work, though we may need to include a specific Python in
the command invocation for Windows.
Attachment #8867387 - Flags: review?(giles)
Comment on attachment 8867387 [details] [diff] [review]
use `mach artifact toolchain' to download clang Stylo packages

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

::: python/mozboot/mozboot/base.py
@@ +278,4 @@
>  
> +        mach_binary = os.path.join(topsrcdir, 'mach')
> +        if not os.path.exists(mach_binary):
> +            raise ValueError("mach not found at %s" % mach_binary)

This means the stylo download step will fail if bootstrap.py is running out outside the context of a firefox source tree (like in an initial build environment bootstrap) right?

I suppose that's ok, but it might be nice to skip the stylo prompt if mach and the tooltool manifests aren't present.

You could also fall back to pulling the manifests over the network `json.loads()` them to get the hashes. That's code duplication, but better preserves the 'bootstrap' sense of the mozboot module.
Attachment #8867387 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #2)
> ::: python/mozboot/mozboot/base.py
> @@ +278,4 @@
> >  
> > +        mach_binary = os.path.join(topsrcdir, 'mach')
> > +        if not os.path.exists(mach_binary):
> > +            raise ValueError("mach not found at %s" % mach_binary)
> 
> This means the stylo download step will fail if bootstrap.py is running out
> outside the context of a firefox source tree (like in an initial build
> environment bootstrap) right?

Uh.  We have a mode like that?  I had heard rumors, but never understood how that was supposed to work.  You apparently know, though--can you explain?

> You could also fall back to pulling the manifests over the network
> `json.loads()` them to get the hashes. That's code duplication, but better
> preserves the 'bootstrap' sense of the mozboot module.

Then I have to keep all the tooltool code...not sure if it's worth it or not.
Flags: needinfo?(giles)
bootstrap also clones m-c if it's not run from it, so, just defer to after m-c is cloned?
(In reply to Nathan Froyd [:froydnj] from comment #3)

> Uh.  We have a mode like that?  I had heard rumors, but never understood how
> that was supposed to work.  You apparently know, though--can you explain?

The general idea is that one can start with a system with only a python installation, download python/mozboot/bin/bootstrap.py from the repository web interface, and execute it. It will pull the rest of the module and then use it to start installing dependencies.

I can imagine it's handy if one isn't used to using version control to check out the source first, but I don't know how common that situation is. Anyway, this is why mozboot doesn't use any of the common infrastructure available to other python modules in Firefox.

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation or https://github.com/mozilla/gecko-dev/blob/master/python/mozboot/README.rst for example instructions for people to do this.

> Then I have to keep all the tooltool code...not sure if it's worth it or not.

I don't either. Of course you could move the tooltool wrapper into mozboot and have mach call into it.


(In reply to Mike Hommey [:glandium] from comment #4)

> bootstrap also clones m-c if it's not run from it, so, just defer to after
> m-c is cloned?

This is in https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/bootstrap.py#398 btw. I'd forgotten about this, because I always follow the git prompts and this only happens with hg.

At least we're writing an expert system in python instead of sh+m4. :)
Flags: needinfo?(giles)
And we also want to support Git clones in the one-line bootstrapper. It just hasn't been implemented yet.

Also, in the one-line bootstrap mode, we could download extra files (such as tooltool manifests) if we wanted to. Code for that lives at https://dxr.mozilla.org/mozilla-central/source/python/mozboot/bin/bootstrap.py#107. The considerations for adding new files to download are:

* Latency. More individual files, more round trips, slower bootstrap execution.
* Defining paths to files. You'll have to give the bootstrapper paths to certain files, since they may be in a relative source directory under version control or in a temp directory.
(In reply to Gregory Szorc [:gps] from comment #6)

> And we also want to support Git clones in the one-line bootstrapper. It just
> hasn't been implemented yet.

It also seems a bit weird that we prompt for the clone target location. I suspect this is a really a limitation of mozboot's prompting, but "what do you want to call the checkout" seems a trivial question if the point is to automate a lot of complicated steps. I'll file a bug.
 
> * Latency. More individual files, more round trips, slower bootstrap
> execution.

In theory we could address this by promise-ifying the downloads. The manifests we'd need to make this bug work in one-line mode are small, so the latency will be easy to hide behind the larger network and disk-walking tasks we do between startup and getting an answer to the stylo question.
Priority: -- → P1
(In reply to Ralph Giles (:rillian) | needinfo me from comment #5)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> 
> > Uh.  We have a mode like that?  I had heard rumors, but never understood how
> > that was supposed to work.  You apparently know, though--can you explain?
> 
> The general idea is that one can start with a system with only a python
> installation, download python/mozboot/bin/bootstrap.py from the repository
> web interface, and execute it. It will pull the rest of the module and then
> use it to start installing dependencies.

Ah, OK, thanks for the explanation.  I've spent some time wrapping my head around how all of this works.

> > Then I have to keep all the tooltool code...not sure if it's worth it or not.
> 
> I don't either. Of course you could move the tooltool wrapper into mozboot
> and have mach call into it.

I looked at doing this: `mach artifact toolchain` is pretty large on its own and pulls in nine kinds of dependencies from outside (taskcluster stuff, various bits of mozbuild, etc.), and trying to untangle that so we can put it in mozboot doesn't seem like a good idea.  Untangling it so a minimum skeleton of functionality can be used from mozboot (or cut-and-pasted, or whatever) also doesn't feel like the right thing.  No point in turning mozboot into a pile of utility code that people have to import just so the bootstrapper can be nice.  (Maybe the solution is pulling more python directories from hgweb in the bootstrapper?)

I'm kind of inclining towards fixing the problems the current code has (e.g. Windows paths), and leaving `mach artifact toolchain` as future work at this point.

> (In reply to Mike Hommey [:glandium] from comment #4)
> > bootstrap also clones m-c if it's not run from it, so, just defer to after
> > m-c is cloned?

I am not super-excited about doing this, because in my mind it breaks one of the core ideas of bootstrap: run it once, and you have everything, you're ready to go.  There's a bit of code in there that asks you to start a new shell if a suitable version of Rust can't be found, but that doesn't feel like a violation of the core principle.  Especially with bootstrap asking you to run through most of the questions again, I'd rather not ask people to run things multiple times.

WDYT?  Do you think we should just make the current code more viable, or do you think we should try to polish it up?
Flags: needinfo?(giles)
I wasn't suggesting people to run mach bootstrap twice, but for bootstrap to do stuff after it cloned m-c.
I think you should just (a) not prompt to install llvm if bootstrap is running stand-alone, meaning such users will have to run bootstrap again once they have a checkout once stylo is required, or (b) delay the prompt until a checkout is available and call mach from there, as glandium suggested. Those are both ways to get this viable enough to land.
Flags: needinfo?(giles)
These patches haven't been tested, and I'm not quite done with all the
modifications needed, but I thought I might as well post them for potential
feedback while I'm writing later patches in the series.

Now that we're installing Stylo packages through `mach`, we need to have
`mach` available, which is only true after we've checked for a clone of
the tree.  We move the block performing the installation in this commit
and then fixup resulting problems in future commits.

Thanks to glandium for the clarification on what to do here.
Attachment #8871376 - Flags: review?(giles)
If we're running bootstrap from inside a mozilla-central checkout, we'll
have all the necessary files that we need to install tooltool packages
via `mach artifact toolchain`.  If we're running bootstrap from a
downloaded bootstrap.py, however, it's possible that we failed to clone
the repository.  We need to account for that situation and provide
instructions on how to deal with it.

A consequence of these two mechanisms for bootstrap is that we'll have
to make sure we're invoking `mach artifact toolchain` correctly in both
cases, which we'll handle in subsequent commits.

I think doing things this way is kind of fallout from not loudly failing if
we're not able to clone.  WDYT about just sys.exit(1)'ing if we fail to clone
the tree in any way, with a message about re-running `mach bootstrap`, so
people can try again?  Do you think that's any better than this approach?
(In reply to Nathan Froyd [:froydnj] from comment #12)

> I think doing things this way is kind of fallout from not loudly failing if
> we're not able to clone.  WDYT about just sys.exit(1)'ing if we fail to clone
> the tree in any way, with a message about re-running `mach bootstrap`, so
> people can try again?  Do you think that's any better than this approach?

That sounds like something we could do after cloning from git is supported. It might be good enough for these purposes to hack that to just pull https://github.com/mozilla/gecko-dev/ which is easy to add, but doesn't support contributing changes back.
Comment on attachment 8871378 [details] [diff] [review]
part 3 - check for a clone before installing Stylo packages

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

Works for me.
Attachment #8871378 - Flags: review+
Comment on attachment 8871376 [details] [diff] [review]
part 2 - move Stylo package installation

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

Thanks for the separate patches for review. I think you should squash this together with part 3 before landing though, since it has the same problems as the previous patch as it stands.
Attachment #8871376 - Flags: review?(giles) → review+
We'll need this information to be able to locate mach later, since we
can't assume that mach is just several directories above the script
we're currently running.
Attachment #8871430 - Flags: review?(giles)
This change ensures that when we're running bootstrap in standalone
mode, we find the correct mach to invoke.

Still need to check on windows whether we need to invoke mach through Python or
not.  Are we always running this stuff from a shell that can invoke Python
scripts directly?
Attachment #8871434 - Flags: review?(giles)
Attachment #8871430 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #15)
> Thanks for the separate patches for review. I think you should squash this
> together with part 3 before landing though, since it has the same problems
> as the previous patch as it stands.

Technically everything after part 1 is fixing brokenness that part 1 introduced, sooo... :)
Attachment #8871434 - Flags: review?(giles) → review+
This is a fixup to the original patch to use `mach artifact toolchain`,
discovered by testing this patch series on Windows.  I'll fold it in before
committing, but it's easier to review separately.
Attachment #8871825 - Flags: review?(giles)
Blocks: 1368099
Comment on attachment 8871825 [details] [diff] [review]
part 1a - use sys.executable to invoke mach

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

::: python/mozboot/mozboot/base.py
@@ +278,5 @@
>          mach_binary = os.path.join(checkout_root, 'mach')
>          if not os.path.exists(mach_binary):
>              raise ValueError("mach not found at %s" % mach_binary)
>  
> +        # XXX not much we can do here on Windows

You don't say what the error was here. Is sys.executable not a path under cygwin?
Attachment #8871825 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) | needinfo me from comment #20)
> ::: python/mozboot/mozboot/base.py
> @@ +278,5 @@
> >          mach_binary = os.path.join(checkout_root, 'mach')
> >          if not os.path.exists(mach_binary):
> >              raise ValueError("mach not found at %s" % mach_binary)
> >  
> > +        # XXX not much we can do here on Windows
> 
> You don't say what the error was here. Is sys.executable not a path under
> cygwin?

The poorly expressed intent of this comment was that sys.executable can actually be something nonsensical, like None, in which case there's not much we can do for attempting to execute a Python script.  And trying to pass:

  [None, 'mach', 'artifact', ...]

to execv or similar syscall is not going to end very well.  I have not dug around in the Python source to how common a None sys.executable might be, whether it occurs only on weird operating systems or similar.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c41cd44a05
part 1 - use `mach artifact toolchain' to download clang Stylo packages; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/c37162881c6e
part 2 - move Stylo package installation; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a7d1f7f4d7
part 3 - check for a clone before installing Stylo packages; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ba2fb5fe5d
part 4 - compute the firefox checkout root; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/94a6ef48ce1a
part 5 - pass the checkout_root down to stylo package installation; r=rillian
(In reply to Nathan Froyd [:froydnj] from comment #21)

> I have not dug around in the Python source to how common a None sys.executable might be,
> whether it occurs only on weird operating systems or similar.

Ok. I saw that in the docs, and figured that was the idea; the comment just made me think you were handling a specific case.

yay landing!
Blocks: 1368168
Blocks: 1382571
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.