bug 1177128 implemented this for Windows, but we should do this for all platforms. It's OK if we only sort this out for our Tier-1 platforms at the moment, but obviously it would be best if `mach bootstrap` installed everything we needed from the get go. We should install via rustup because: a) Most distros do not have a usable Rust package yet b) rustup is way more useful We shouldn't go installing rustup if there's already a usable rustc available.

Comment on attachment 8811378 [details] Bug 1286799 - mozboot: Abstract version checking. https://reviewboard.mozilla.org/r/93496/#review93564 ::: python/mozboot/mozboot/base.py:335 (Diff revision 1) > + An optional env argument allows modifying environment > + variable during the invocation to set options, PATH, > + etc. That's a nice feature. But it is unused!

Comment on attachment 8811378 [details] Bug 1286799 - mozboot: Abstract version checking. https://reviewboard.mozilla.org/r/93496/#review93564 > That's a nice feature. But it is unused! See the next patch.

Comment on attachment 8811378 [details] Bug 1286799 - mozboot: Abstract version checking. https://reviewboard.mozilla.org/r/93496/#review94254

Comment on attachment 8811379 [details] Bug 1286799 - mozboot: Use a helper for mercurial version detection. https://reviewboard.mozilla.org/r/93498/#review94258

Comment on attachment 8811380 [details] Bug 1286799 - mozboot: Ensure an up-to-date rust toolchain. https://reviewboard.mozilla.org/r/93500/#review94262 ::: python/mozboot/mozboot/base.py:505 (Diff revision 2) > + if modern: > + print('Your version of Rust (%s) is new enough.' % version) > + return > + > + if not version: > + '''Rust wasn't in PATH. Try a few things.''' This should be a regular `#` comment. The triple quote comments ("docstrings") are part of the AST and typically only used to define module, function, class, or method documentation or used to define multi line literals.

Comment on attachment 8811380 [details] Bug 1286799 - mozboot: Ensure an up-to-date rust toolchain. https://reviewboard.mozilla.org/r/93500/#review94262 > This should be a regular `#` comment. The triple quote comments ("docstrings") are part of the AST and typically only used to define module, function, class, or method documentation or used to define multi line literals. You fixed this in the next commit. So dropping the issue.

Comment on attachment 8811996 [details] Bug 1286799 - mozboot: Install or upgrade rust. https://reviewboard.mozilla.org/r/93874/#review94268 This is mostly good. Just a few (mostly Python) things to clean up. Also, it would be nice to consolidate the code in mozillabuild.py with this new code. But I don't want to block this landing for that. ::: python/mozboot/mozboot/base.py:561 (Diff revision 2) > + We should have determined that rustup is available first.""" > + rustup = self.which('rustup') Let's pass the path to `rustup` into this function since the only caller has already found it. ::: python/mozboot/mozboot/base.py:568 (Diff revision 2) > + self.check_output([rustup, 'update'], > + stderr=subprocess.STDOUT) > + > + def install_rust(self): > + """Download and run the rustup installer.""" > + import rust You should be able to move this to the top of the file. And the import should be written as `import mozboot.rust` or `from mozboot.rust import x, y`. `import rust` works because Python 2 allows relative imports. This behavior goes away in Python 3 and in files with `from __future__ import absolute_import`. We don't like relying on relative imports because they can be ambiguous. e.g. if your package has a module `os`, then `import os` is ambiguous because `os` collides with a stdlib module! ::: python/mozboot/mozboot/base.py:580 (Diff revision 2) > + print('ERROR: Could not download installer.') > + sys.exit(1) > + rustup_init = os.path.join(tempfile.gettempdir(), os.path.basename(url)) > + try: > + self.http_download_and_save(url, rustup_init, checksum) > + os.chmod(rustup_init, stat.S_IRWXU) I could nitpick this not honoring umask. Ideally you would do a bitwise OR against the existing file mode to preserve group and other bits. ::: python/mozboot/mozboot/base.py:586 (Diff revision 2) > + self.check_output([rustup_init, '-y', '--default-host', platform], > + stderr=subprocess.STDOUT) > + finally: > + try: > + os.remove(rustup_init) > + except FileNotFoundError: FileNotFoundError only exists on Python 3 :/ To do this in Python 2, you need: except OSError as e: if e.errno != errno.ENOENT: raise ::: python/mozboot/mozboot/rust.py:61 (Diff revision 2) > + > +def platform(): > + '''Determine the appropriate rust platform string for the current host''' > + if sys.platform.startswith('darwin'): > + return 'x86_64-apple-darwin' > + elif sys.platform.startswith('win32') or sys.platform.startswith('msys'): `str.startswith` accepts a tuple so you can match multiple prefixes. e.g. `sys.platform.startswith(('win32', 'msys'))`.

Comment on attachment 8811997 [details] Bug 1286799 - mozboot: Add a script to fetch rust installer hashes. https://reviewboard.mozilla.org/r/93876/#review94272 It's tempting to put the hashes in a standalone JSON file to make updating them easier. Although at some point we end up reinventing tooltool, heh. ::: python/mozboot/mozboot/rust.py:111 (Diff revision 2) > + return None > + > +def http_download_and_hash(url): > + import hashlib > + import urllib2 > + f = urllib2.urlopen(url) Fun fact: Python's built-in HTTP libraries don't always do security verification properly. The best way to guarantee Python does something reasonable is to use the 3rd party `requests` package. This package is vendored in mozilla-central. So it would be easy to hack up the `__main__` function below to add it to the search path. e.g. HERE = os.path.dirname(__file__) sys.path.insert(0, os.path.join(HERE, '..', '..', 'requests')) Of course, you can't `import requests` from the module because it won't be available when running `bootstrap.py` in standalone mode. ::: python/mozboot/mozboot/rust.py:169 (Diff revision 2) > + make_checksums(RUSTUP_VERSION, validate=True) > + exit() > + > + print('Out of date. We use %s. Calculating checksums.' % RUSTUP_VERSION) > + hashes = make_checksums(version) > + print print('')

Comment on attachment 8811998 [details] Bug 1286799 - mozboot: Unbuffer stdout. https://reviewboard.mozilla.org/r/93878/#review94274

Comment on attachment 8811996 [details] Bug 1286799 - mozboot: Install or upgrade rust. https://reviewboard.mozilla.org/r/93874/#review94268 > it would be nice to consolidate the code in mozillabuild.py with this new code. But I don't want to block this landing for that. Agreed. I filed bug 1318756. > Let's pass the path to `rustup` into this function since the only caller has already found it. Ok. > You should be able to move this to the top of the file. And the import should be written as `import mozboot.rust` or `from mozboot.rust import x, y`. > > `import rust` works because Python 2 allows relative imports. This behavior goes away in Python 3 and in files with `from __future__ import absolute_import`. We don't like relying on relative imports because they can be ambiguous. e.g. if your package has a module `os`, then `import os` is ambiguous because `os` collides with a stdlib module! Done, thanks. > I could nitpick this not honoring umask. Ideally you would do a bitwise OR against the existing file mode to preserve group and other bits. Ok. I don't expect it to matter for a tempfile (and if it did, setting u+x won't be sufficient) though? > FileNotFoundError only exists on Python 3 :/ > > To do this in Python 2, you need: > > except OSError as e: > if e.errno != errno.ENOENT: > raise Ah, I copied that from `MozillaBuildBootstrapper.install_rustup()`. > `str.startswith` accepts a tuple so you can match multiple prefixes. e.g. `sys.platform.startswith(('win32', 'msys'))`. nice, thanks!

Comment on attachment 8811996 [details] Bug 1286799 - mozboot: Install or upgrade rust. https://reviewboard.mozilla.org/r/93874/#review94306

Comment on attachment 8811997 [details] Bug 1286799 - mozboot: Add a script to fetch rust installer hashes. https://reviewboard.mozilla.org/r/93876/#review94272 I thought about that too. I want to hack a self-patch function so `python rust.py --update` actually changes the source code, which would be sufficiently convenient, but didn't want to block landing on that. > Fun fact: Python's built-in HTTP libraries don't always do security verification properly. The best way to guarantee Python does something reasonable is to use the 3rd party `requests` package. This package is vendored in mozilla-central. So it would be easy to hack up the `__main__` function below to add it to the search path. e.g. > > HERE = os.path.dirname(__file__) > sys.path.insert(0, os.path.join(HERE, '..', '..', 'requests')) > > Of course, you can't `import requests` from the module because it won't be available when running `bootstrap.py` in standalone mode. Aha. I was aware of this, but assumed I couldn't use requests because of bootstrap mode and was relying on this being fixed in the latest 2.7 releases. Being able to use it for the update script, where we don't even have the local hash for verification, is much better. Fixed in a separate commit. More instructive that way, I think. > print('') right. Is there a lint for this sort of thing?

Comment on attachment 8811996 [details] Bug 1286799 - mozboot: Install or upgrade rust. https://reviewboard.mozilla.org/r/93874/#review94730 Looking better. Unfortunately I spotted some things that elluded me last time. ::: python/mozboot/mozboot/base.py:578 (Diff revision 4) > + url = rust.rustup_url(platform) > + checksum = rust.rustup_hash(platform) > + if not url or not checksum: > + print('ERROR: Could not download installer.') > + sys.exit(1) > + rustup_init = os.path.join(tempfile.gettempdir(), os.path.basename(url)) Sorry for not spotting this in the first review, but this should not use a deterministic filename. This could fail on multi-user systems where multiple accounts are attempting to write to `/tmp/rustup-init` for example. Instead, use `tempfile.mkstemp()` to obtain a unique filename. ::: python/mozboot/mozboot/base.py:583 (Diff revision 4) > + self.check_output([rustup_init, '-y', '--default-host', platform], > + stderr=subprocess.STDOUT) This failed on my Ubuntu 16.10 machine :/ Unfortunately, I'm not sure why it failed because `check_output()` is swallowing stdout. If you want stdout to be seen by the user, you should use `subprocess.check_call()`. Other instances of `check_output()` in this patch should likely also be fixed, as we try to be as transparent as possible with what bootstrap is doing (since we often invoke things with sudo).

Comment on attachment 8811997 [details] Bug 1286799 - mozboot: Add a script to fetch rust installer hashes. https://reviewboard.mozilla.org/r/93876/#review94272 > right. Is there a lint for this sort of thing? I don't think we have Python linting of this code. We should. The problem is then someone would have to go through and fix all the linting failures, which I'm sure there are several of :/

Comment on attachment 8811997 [details] Bug 1286799 - mozboot: Add a script to fetch rust installer hashes. https://reviewboard.mozilla.org/r/93876/#review94734 ::: python/mozboot/mozboot/rust.py:98 (Diff revision 4) > + # > + # schema-version = '1' > + # version = '0.6.5' > + # > + for line in f: > + (key, value) = map(str.strip, line.split('=', 2)) Nit: you don't need the parens around key and value here.

Comment on attachment 8812342 [details] Bug 1286799 - mozboot: Use requests to download rustup manifest. https://reviewboard.mozilla.org/r/94126/#review94738 Mostly good. r- for the Python nits. ::: python/mozboot/mozboot/rust.py:111 (Diff revision 2) > def http_download_and_hash(url): > import hashlib > - import urllib2 > - f = urllib2.urlopen(url) > h = hashlib.sha256() > - while True: > + r = requests.get(url, stream=True) I thought for sure this would raise because `requests` wasn't imported in the current scope. However, you are getting lucky here because the `import requests` below is in an `if __name__ ...` block, which doesn't count as a new scope for variables in Python. Still, this is a bit wonky. So please move the `import requests` to this function. ::: python/mozboot/mozboot/rust.py:144 (Diff revision 2) > + # Hook the requests module from the greater source tree. We can't import > + # this at the module level since we might be imported into the bootstrap > + # script in standalone mode. > + # > + # This is necessary for correct https certificate verification. > + mod_path = os.path.dirname(sys.argv[0]) `sys.argv[0]` may not always be what you think. It is safer to use `__file__` instead.

Comment on attachment 8812881 [details] Bug 1286799 - mozboot: Fix rust detection on windows. https://reviewboard.mozilla.org/r/94430/#review94742 LGTM. Nit: your commit message contains some lines left over from a commit fold.

Comment on attachment 8811996 [details] Bug 1286799 - mozboot: Install or upgrade rust. https://reviewboard.mozilla.org/r/93874/#review94730 > Sorry for not spotting this in the first review, but this should not use a deterministic filename. This could fail on multi-user systems where multiple accounts are attempting to write to `/tmp/rustup-init` for example. > > Instead, use `tempfile.mkstemp()` to obtain a unique filename. Oops! Missed this too. Thanks. > This failed on my Ubuntu 16.10 machine :/ > > Unfortunately, I'm not sure why it failed because `check_output()` is swallowing stdout. If you want stdout to be seen by the user, you should use `subprocess.check_call()`. > > Other instances of `check_output()` in this patch should likely also be fixed, as we try to be as transparent as possible with what bootstrap is doing (since we often invoke things with sudo). I've switched to check_call() and the output seems to be going to the console now.

Comment on attachment 8812342 [details] Bug 1286799 - mozboot: Use requests to download rustup manifest. https://reviewboard.mozilla.org/r/94126/#review94738 > I thought for sure this would raise because `requests` wasn't imported in the current scope. However, you are getting lucky here because the `import requests` below is in an `if __name__ ...` block, which doesn't count as a new scope for variables in Python. > > Still, this is a bit wonky. So please move the `import requests` to this function. I see. I wasn't sure if you wanted it next to the path fiddling. I expected it to raise an exception if anyone tried to call `http_download_and_hash()` outside of the `if __name__ ...` block, but raising an import exception there is better error reporting. > `sys.argv[0]` may not always be what you think. It is safer to use `__file__` instead. Ok.

Comment on attachment 8811996 [details] Bug 1286799 - mozboot: Install or upgrade rust. https://reviewboard.mozilla.org/r/93874/#review94982

Comment on attachment 8812342 [details] Bug 1286799 - mozboot: Use requests to download rustup manifest. https://reviewboard.mozilla.org/r/94126/#review94984

Pushed by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/77a1f8ecdb7c mozboot: Abstract version checking. r=gps https://hg.mozilla.org/integration/autoland/rev/b27d1438823e mozboot: Use a helper for mercurial version detection. r=gps https://hg.mozilla.org/integration/autoland/rev/cb73d012339f mozboot: Ensure an up-to-date rust toolchain. r=gps https://hg.mozilla.org/integration/autoland/rev/e48812c055b5 mozboot: Install or upgrade rust. r=gps https://hg.mozilla.org/integration/autoland/rev/26fc277f84ed mozboot: Add a script to fetch rust installer hashes. r=gps https://hg.mozilla.org/integration/autoland/rev/5cd2f35aa16b mozboot: Unbuffer stdout. r=gps https://hg.mozilla.org/integration/autoland/rev/0c06981308ba mozboot: Use requests to download rustup manifest. r=gps https://hg.mozilla.org/integration/autoland/rev/56d316078f87 mozboot: Fix rust detection on windows. r=gps

https://hg.mozilla.org/mozilla-central/rev/77a1f8ecdb7c https://hg.mozilla.org/mozilla-central/rev/b27d1438823e https://hg.mozilla.org/mozilla-central/rev/cb73d012339f https://hg.mozilla.org/mozilla-central/rev/e48812c055b5 https://hg.mozilla.org/mozilla-central/rev/26fc277f84ed https://hg.mozilla.org/mozilla-central/rev/5cd2f35aa16b https://hg.mozilla.org/mozilla-central/rev/0c06981308ba https://hg.mozilla.org/mozilla-central/rev/56d316078f87