Chris Peterson [:cpeterson] Reporter
	Description • 8 years ago

To add libclang as a build dependency (bug 1310852), we will need to change configure to detect libclang and `mach bootstrap` to install libclang.

	Chris Peterson [:cpeterson] Reporter
	Comment 1 • 8 years ago

P2 because rust-bindgen and libclang are a high priority but they are not immediate blockers for Stylo.

	Chris Peterson [:cpeterson] Reporter
	Comment 2 • 8 years ago

Assigning to Nathan because he said he can help with libclang. (Thanks!)

Changing priority to P1 because he is actively working on this now.

	Chris Peterson [:cpeterson] Reporter
	Comment 3 • 8 years ago

Nathan says that Windows libclang dependency is now in tooltool. He still needs to update mach bootstrap and document new mozconfig flags.

	Nathan Froyd [:froydnj] Assignee
	Comment 4 • 8 years ago

For Stylo development, LLVM packages are required due to Stylo's
extensive use of bindgen--generating Rust bindings to Gecko's C++ code.
While people can install LLVM via their system package manager, we've
opted to download the LLVM packages used on Mozilla infrastructure for
building Gecko.  Using Mozilla's packages for LLVM ensures that they
work, and also makes it easier/trivial to integrate support for other
things (e.g. Mozilla's static checkers) into `mach bootstrap`.

Asking for feedback only because while I think this is the design I like, I'm
not totally convinced of the tooltool checksums we need to use.  For instance,
I'm not sure that on Windows, we want to use the very latest LLVM build like we
do for our automation builds--that might not work to build Stylo?  OTOH, I'm
not sure what sort of testing we need to do, or if we can just let people find
problems with `mach bootstrap` on their own, since they're won't be a large
userbase.

	Chris Manchester (limited bugmail, email directly)
	Comment 5 • 8 years ago

Comment on attachment 8849261 [details] [diff] [review]
download LLVM packages from mach bootstrap for Stylo development

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

I'm not really up to speed on our conventions for the bootstrapper, I'd have :gps sign off on this. Regarding the commit message I'd say we want to find someone to try building stylo with these packages on each platform before landing.

::: python/mozboot/mozboot/base.py
@@ +300,5 @@
> +                h.update(data)
> +
> +            if h.hexdigest() != package_sha512sum:
> +                os.remove(abspath)
> +                raise ValueError('Hash of downloaded file does not match expected hash')

There's some code that does something like this in `tools/mach_commands.py`... maybe it should be consolidated? Other interactions with tooltool seem to install and use tooltool.py.

::: python/mozboot/mozboot/bootstrap.py
@@ +225,5 @@
>          if cls is None:
>              raise NotImplementedError('Bootstrap support is not yet available '
>                                        'for your OS.')
>  
> +        print(cls)

Leftover debug print.

@@ +272,5 @@
>  
> +        # Install the clang packages needed for developing stylo.  Eventually
> +        # these will be required (possibly installed via system packages), but
> +        # for now, we'll require developers to export an environment variable
> +        # to trigger stylo package install.

Is an environment variable really a desirable entry point for this? I wonder if we could put it in the top level prompt (alongside Desktop, Android, Android Artifact) to make it straightforward for people who want it but not increase the number of times someone gets prompted.

@@ +282,5 @@
> +                high=2)
> +
> +            # The best place to install our packages is in the state directory
> +            # we have.  If the user doesn't have one, we need them to re-run
> +            # bootstrap and create the directory.

`choice` isn't checked here.

::: python/mozboot/mozboot/windows.py
@@ +10,5 @@
>  
> +CLANG_TOOLTOOL_FILENAME = 'clang.tar.xz'
> +CLANG_TOOLTOOL_SHA512SUM = 'cd3ed31acefd185f441632158dde73538c62bab7ebf2a8ec630985ab345938ec522983721ddb1bead1de22d5ac1571d50a958ae002364d739f2a78c6e7244222'
> +
> +

My recollection is that this is the bootstrap file for msys2, which is experimental and not yet generally used, and mozillabuild.py is the very minimal version we use on the current windows builds.

	Mike Hommey [:glandium]
	Comment 6 • 8 years ago

(In reply to Chris Manchester (:chmanchester) from comment #5)
> ::: python/mozboot/mozboot/base.py
> @@ +300,5 @@
> > +                h.update(data)
> > +
> > +            if h.hexdigest() != package_sha512sum:
> > +                os.remove(abspath)
> > +                raise ValueError('Hash of downloaded file does not match expected hash')
> 
> There's some code that does something like this in
> `tools/mach_commands.py`... maybe it should be consolidated? Other
> interactions with tooltool seem to install and use tooltool.py.

I have work in progress that adds a mach command that wraps tooltool.

	Nathan Froyd [:froydnj] Assignee
	Comment 7 • 8 years ago

(In reply to Chris Manchester (:chmanchester) from comment #5)
> ::: python/mozboot/mozboot/base.py
> @@ +300,5 @@
> > +                h.update(data)
> > +
> > +            if h.hexdigest() != package_sha512sum:
> > +                os.remove(abspath)
> > +                raise ValueError('Hash of downloaded file does not match expected hash')
> 
> There's some code that does something like this in
> `tools/mach_commands.py`... maybe it should be consolidated? Other
> interactions with tooltool seem to install and use tooltool.py.

I'm not super-excited about using tooltool.py, since that requires actual manifests, but I guess we could try...or wait to see what glandium has in store.

> @@ +272,5 @@
> >  
> > +        # Install the clang packages needed for developing stylo.  Eventually
> > +        # these will be required (possibly installed via system packages), but
> > +        # for now, we'll require developers to export an environment variable
> > +        # to trigger stylo package install.
> 
> Is an environment variable really a desirable entry point for this? I wonder
> if we could put it in the top level prompt (alongside Desktop, Android,
> Android Artifact) to make it straightforward for people who want it but not
> increase the number of times someone gets prompted.

My thinking here was that Stylo isn't (yet) a mandatory part of the build.  People who want to do Stylo development can flip the environment variable while not downloading a bunch of stuff/taking up disk space for people who don't need Stylo.

I don't think we should add another option to that toplevel prompt--we already have complaints about the wall o' explanatory text that prompt requires, and we should be looking at ways to reduce that, not add more options.

	Nathan Froyd [:froydnj] Assignee
	Comment 8 • 8 years ago

This will be useful for downloading files from tooltool, which requires
a SHA512 checksum.

	Nathan Froyd [:froydnj] Assignee
	Comment 9 • 8 years ago

Revisions since comment 5:

* Stylo mozconfig suggestions
* Better downloading via http_download_and_save
* Removed environment variable requirement for Stylo bootstrap
* Better guidance in Stylo message

I didn't try to use a generic tooltool tool because there doesn't seem to be
one in the tree, and downloading the file directly from tooltool is easy in any
event.

	Ralph Giles (:rillian)
	Comment 10 • 8 years ago

Comment on attachment 8857177 [details] [diff] [review]
part 1 - add ability to use a generic digest algorithm for http_download_and_save

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

::: python/mozboot/mozboot/base.py
@@ +661,5 @@
> +    def http_download_and_save(self, url, dest, hexhash, digest='sha256'):
> +        """Download the given url and save it to dest.  hexhash is a checksum
> +        that will be used to validate the downloaded file using the given
> +        digest algorithm.  The value of digest can be any value accepted by
> +        hashlib.new.  The default algorithm used is SHA256."""

I was confused by the `SHA256` capitalization here. It turns out `hashlib.new()` accepts both upper- and lower-case versions of the algorithm name, but the default argument value here and `hashlib.algorithms` both use lowercase variants, so maybe use the same thing here, with quotes to make it more clear a string is expected?

FWIW, the docs also say `hashlib.sha256()` is much faster than `hashlib.new('sha256')`. This was true when I tried it, but not enough to matter compared to the network access, so I think doing the fallback this way is fine.

	Ralph Giles (:rillian)
	Comment 11 • 8 years ago

Comment on attachment 8857178 [details] [diff] [review]
part 2 - download LLVM packages from mach bootstrap for Stylo development

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

Overall, looks fine. Some suggestions below. I'd like to see it again with the missing fine included.

::: python/mozboot/mozboot/archlinux.py
@@ +8,5 @@
>  import subprocess
>  import glob
>  
>  from mozboot.base import BaseBootstrapper
> +from mozboot.linux_common import StyloInstall

Did you forget to add linux_common.py to the patch?

::: python/mozboot/mozboot/base.py
@@ +294,5 @@
> +        elif package_filename.endswith('.tar.bz2'):
> +            cmd = ['tar', 'jxf', downloaded_filename]
> +        elif package_filename.endswith('.tar.xz'):
> +            cmd = ['tar', 'Jxf', downloaded_filename]
> +        else:

Looks like we only use zip files for msvc in tooltool manifests, but it might be worth including here for completeness.

@@ +301,5 @@
> +
> +        print('Unpacking %s...' % downloaded_filename)
> +
> +        with open(os.devnull, 'w') as stdout:
> +            subprocess.check_call(cmd, stdout=stdout, cwd=state_dir)

`tar` shouldn't print anything unless there's an error, right?

@@ +305,5 @@
> +            subprocess.check_call(cmd, stdout=stdout, cwd=state_dir)
> +
> +        print('Unpacking %s...DONE' % downloaded_filename)
> +
> +        os.remove(downloaded_filename)

Please put this in a finally: clause so we don't leave large files around if the tar command fails.

::: python/mozboot/mozboot/bootstrap.py
@@ +284,5 @@
> +            # we have.  If the user doesn't have one, we need them to re-run
> +            # bootstrap and create the directory.
> +            #
> +            # XXX Android bootstrap just assumes the existence of the state
> +            # directory and writes the NDK into it.  Should we do the same?

If we do, we should remove the prompt and just print a status message about where things are being installed. That should be a separate issue though.

I think it's nice to prompt. Some developers really care about their environment being modified, and it's nice to have the chance to pick a different location. A better approach would be to encapsulate this sort of thing into an `ensure_state_directory` method which anything which wants to write to the state directory can call.

I'm fine with this for now though.

@@ +334,5 @@
> +        if application == 'browser':
> +            self.instance.suggest_browser_mozconfig(stylo=installed_stylo,
> +                                                    state_dir=get_state_dir())
> +        else:
> +            getattr(self.instance, 'suggest_%s_mozconfig' % application)()

This is kind of ugly. What about setting `installed_stylo` on the instance object and then checking that (and calling `get_state_dir`) inside `suggest_browser_mozconfig`?

::: python/mozboot/mozboot/windows.py
@@ +89,5 @@
> +
> +    def ensure_stylo_packages(self, state_dir):
> +        self.install_tooltool_clang_package(state_dir,
> +                                            CLANG_TOOLTOOL_FILENAME,
> +                                            CLANG_TOOLTOOL_SHA512SUM)

This is fine, but for the rust installer I put all the hashes and url generation in rust.py and queried it from a shared method in base.py. I think that's less typing and makes it easier to update for new releases.

	Mike Hommey [:glandium]
	Comment 12 • 8 years ago

Comment on attachment 8857178 [details] [diff] [review]
part 2 - download LLVM packages from mach bootstrap for Stylo development

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

::: python/mozboot/mozboot/base.py
@@ +270,5 @@
> +            % __name__)
> +
> +    def install_tooltool_clang_package(self, state_dir,
> +                                       package_filename, package_sha512sum):
> +        TOOLTOOL_API = 'https://api.pub.build.mozilla.org/tooltool/sha512/'

Please don't reimplement tooltool. We have it in the tree. As a matter of fact, my WIP for bug 1313111 includes a mach command that allows to download toolchain packages from tooltool, with a transition plan for pulling taskcluster artifacts later on. Coincidentally, I was going to resume work on that this afternoon.

	Mike Hommey [:glandium]
	Comment 13 • 8 years ago

Comment on attachment 8857178 [details] [diff] [review]
part 2 - download LLVM packages from mach bootstrap for Stylo development

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

::: python/mozboot/mozboot/mozillabuild.py
@@ +11,5 @@
>  from mozboot.base import BaseBootstrapper
>  
> +# Clang + LLVM 3.9.0
> +CLANG_TOOLTOOL_FILENAME = 'clang.tar.xz'
> +CLANG_TOOLTOOL_SHA512SUM = 'cd3ed31acefd185f441632158dde73538c62bab7ebf2a8ec630985ab345938ec522983721ddb1bead1de22d5ac1571d50a958ae002364d739f2a78c6e7244222'

Adding one more location that needs to be updated when we change clang versions is not really great.

	Nathan Froyd [:froydnj] Assignee
	Comment 14 • 8 years ago

(In reply to Mike Hommey [:glandium] from comment #12)
> ::: python/mozboot/mozboot/base.py
> @@ +270,5 @@
> > +            % __name__)
> > +
> > +    def install_tooltool_clang_package(self, state_dir,
> > +                                       package_filename, package_sha512sum):
> > +        TOOLTOOL_API = 'https://api.pub.build.mozilla.org/tooltool/sha512/'
> 
> Please don't reimplement tooltool. We have it in the tree. As a matter of
> fact, my WIP for bug 1313111 includes a mach command that allows to download
> toolchain packages from tooltool, with a transition plan for pulling
> taskcluster artifacts later on. Coincidentally, I was going to resume work
> on that this afternoon.

We do have it--at least I see a wrapper of sorts in mozharness, not sure if that's the one you're thinking of.  What's bad about redoing the tiny bit we need here?  Are you thinking that we should be hardcoding the paths to the tooltool manifests that we use for automation, and then extracting the clang bit out of that, then writing a temporary manifest just for the one file we need to get, then calling tooltool on that?  That sounds like a lot of unnecessary pain.

	Mike Hommey [:glandium]
	Comment 15 • 8 years ago

The full tooltool is in taskcluster/docker/recipes/tooltool.py. And you don't need to invoke it with a manifest, you can import it, although not directly from that location. In fact, I already have all the code for that, I "just" need to fix the artifact download code for whatever the issue I found in it was. That's what I was planning to do this afternoon.

	Nathan Froyd [:froydnj] Assignee
	Comment 16 • 8 years ago

(In reply to Ralph Giles (:rillian) | needinfo me from comment #11)
> ::: python/mozboot/mozboot/archlinux.py
> @@ +8,5 @@
> >  import subprocess
> >  import glob
> >  
> >  from mozboot.base import BaseBootstrapper
> > +from mozboot.linux_common import StyloInstall
> 
> Did you forget to add linux_common.py to the patch?

Doh, I did.

> ::: python/mozboot/mozboot/base.py
> @@ +294,5 @@
> > +        elif package_filename.endswith('.tar.bz2'):
> > +            cmd = ['tar', 'jxf', downloaded_filename]
> > +        elif package_filename.endswith('.tar.xz'):
> > +            cmd = ['tar', 'Jxf', downloaded_filename]
> > +        else:
> 
> Looks like we only use zip files for msvc in tooltool manifests, but it
> might be worth including here for completeness.

Eh.  This covers the platforms we need for clang tarballs, which are produced by our infra; if this was a more general tooltool download mechanism, zip files might be worth including.

> @@ +301,5 @@
> > +
> > +        print('Unpacking %s...' % downloaded_filename)
> > +
> > +        with open(os.devnull, 'w') as stdout:
> > +            subprocess.check_call(cmd, stdout=stdout, cwd=state_dir)
> 
> `tar` shouldn't print anything unless there's an error, right?

It shouldn't, but why not be sure?

> @@ +305,5 @@
> > +            subprocess.check_call(cmd, stdout=stdout, cwd=state_dir)
> > +
> > +        print('Unpacking %s...DONE' % downloaded_filename)
> > +
> > +        os.remove(downloaded_filename)
> 
> Please put this in a finally: clause so we don't leave large files around if
> the tar command fails.

Good idea, I'll do that.

> @@ +334,5 @@
> > +        if application == 'browser':
> > +            self.instance.suggest_browser_mozconfig(stylo=installed_stylo,
> > +                                                    state_dir=get_state_dir())
> > +        else:
> > +            getattr(self.instance, 'suggest_%s_mozconfig' % application)()
> 
> This is kind of ugly. What about setting `installed_stylo` on the instance
> object and then checking that (and calling `get_state_dir`) inside
> `suggest_browser_mozconfig`?

I guess we could call get_state_dir in the appropriate suggest_browser_mozconfigs.  I'll try that.

> ::: python/mozboot/mozboot/windows.py
> @@ +89,5 @@
> > +
> > +    def ensure_stylo_packages(self, state_dir):
> > +        self.install_tooltool_clang_package(state_dir,
> > +                                            CLANG_TOOLTOOL_FILENAME,
> > +                                            CLANG_TOOLTOOL_SHA512SUM)
> 
> This is fine, but for the rust installer I put all the hashes and url
> generation in rust.py and queried it from a shared method in base.py. I
> think that's less typing and makes it easier to update for new releases.

Eh, I think it works out to about the same.  And given that clang is on more like a six-month release schedule rather than a six-week one, we're not updating as frequently.  And we're probably not updating right away, given that bindgen will need changes to work correctly with new releases and so forth.  I'm not too worried about this.

	Ralph Giles (:rillian)
	Comment 17 • 8 years ago

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

> Adding one more location that needs to be updated when we change clang
> versions is not really great.

FWIW, I wrote in update script for the rust installer stuff. You (Nathan) might consider adding something like that to help maintain all the links, e.g. by copying from a manifest. I don't think that should hold up this patch though. (In reply to Nathan 

Froyd [:froydnj] from comment #16)

> > `tar` shouldn't print anything unless there's an error, right?
> 
> It shouldn't, but why not be sure?

It's more code, and swallowing command output doesn't make it easier to report or debug bootstrap failures. I don't feel strongly about it though.

	Nathan Froyd [:froydnj] Assignee
	Comment 18 • 8 years ago

OK, revised patch.  I think this addresses most of comment 16:

* linux_common.py has been added.

* tooltool hashes etc. have been script-generated; I wasn't going to do this,
  but wanted to experiment, and doing so found that some packages would have
  been out of date and the Windows package needs an exception anyway.  So
  winning all around!

* The suggest_browser_mozconfig bits have been reworked.

* Removal of the downloaded filename has been moved to a finally block.

Apologies for taking so long to get around to the revisions.

	Ralph Giles (:rillian)
	Comment 19 • 8 years ago

Comment on attachment 8862953 [details] [diff] [review]
part 2 - download LLVM packages from mach bootstrap for Stylo development

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

Looks much cleaner. Thanks. r=me with the two nits below addressed.

::: python/mozboot/mozboot/base.py
@@ +133,5 @@
>  tool or package manager on your system, or directly from https://rust-lang.org/
>  '''
>  
> +STYLO_MOZCONFIG = '''
> +Paste the lines between the chevrons (>>> and <<<) into your mozconfig file:

The prompt asks if one wants "to download packages to work on stylo" but STYLO_MOZCONFIG unconditionally suggest `--enable-stylo`. To me "download" suggests a potential rather than a definite desire.

I'd prefix this message with a "To enable stylo in your build, paste..."

::: python/mozboot/mozboot/bootstrap.py
@@ +272,5 @@
>  
>          state_dir_available = os.path.exists(state_dir)
>  
> +        # Install the clang packages needed for developing stylo.
> +        installed_stylo = False

It looks like this variable is no longer necessary.

	Jan Beich
	Comment 20 • 8 years ago

Comment on attachment 8862953 [details] [diff] [review]
part 2 - download LLVM packages from mach bootstrap for Stylo development

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

::: python/mozboot/mozboot/mozillabuild.py
@@ +82,5 @@
>          pass
>  
> +    def suggest_browser_mozconfig(self):
> +        if self.stylo:
> +            print(STYLO_MOZCONFIG.format(state_dir=self.state_dir))

Doesn't seem to work.

$ python2.7
>>> STYLO_MOZCONFIG = 'ac_add_options --with-clang-path=%(state_dir)/clang/bin/clang.exe'
>>> print(STYLO_MOZCONFIG.format(state_dir='/home/foo/.mozbuild'))
ac_add_options --with-clang-path=%(state_dir)/clang/bin/clang.exe

vs.

$ python2.7
>>> STYLO_MOZCONFIG = 'ac_add_options --with-clang-path=%(state_dir)s/clang/bin/clang.exe'
>>> print(STYLO_MOZCONFIG % { 'state_dir': '/home/foo/.mozbuild'})
ac_add_options --with-clang-path=/home/foo/.mozbuild/clang/bin/clang.exe

	Nathan Froyd [:froydnj] Assignee
	Comment 21 • 8 years ago

(In reply to Jan Beich from comment #20)
> ::: python/mozboot/mozboot/mozillabuild.py
> @@ +82,5 @@
> >          pass
> >  
> > +    def suggest_browser_mozconfig(self):
> > +        if self.stylo:
> > +            print(STYLO_MOZCONFIG.format(state_dir=self.state_dir))
> 
> Doesn't seem to work.

Thanks, fixed.

Also added a message to indicate that we're downloading the clang package from tooltool; since we're using Python's urllib, there's no progress bar as one might get with curl/wget, so the process was eerily silent.

	Pulsebot
	Comment 22 • 8 years ago

Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03002274c129
part 1 - add ability to use a generic digest algorithm for http_download_and_save; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/8248949d1c74
part 2 - download LLVM packages from mach bootstrap for Stylo development; r=rillian

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/03002274c129
https://hg.mozilla.org/mozilla-central/rev/8248949d1c74