(not currently active) Ted Mielczarek Reporter
	Description • 8 years ago

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.

	Gregory Szorc [:gps]
	Comment 4 • 8 years ago
mozreview-review

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!

	Ralph Giles (:rillian) Assignee
	Comment 5 • 8 years ago
mozreview-review-reply

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.

	Gregory Szorc [:gps]
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8811378 [details]
Bug 1286799 - mozboot: Abstract version checking.

https://reviewboard.mozilla.org/r/93496/#review94254

	Gregory Szorc [:gps]
	Comment 16 • 8 years ago
mozreview-review

Comment on attachment 8811379 [details]
Bug 1286799 - mozboot: Use a helper for mercurial version detection.

https://reviewboard.mozilla.org/r/93498/#review94258

	Gregory Szorc [:gps]
	Comment 17 • 8 years ago
mozreview-review

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.

	Gregory Szorc [:gps]
	Comment 18 • 8 years ago
mozreview-review-reply

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.

	Gregory Szorc [:gps]
	Comment 19 • 8 years ago
mozreview-review

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'))`.

	Gregory Szorc [:gps]
	Comment 20 • 8 years ago
mozreview-review

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('')

	Gregory Szorc [:gps]
	Comment 21 • 8 years ago
mozreview-review

Comment on attachment 8811998 [details]
Bug 1286799 - mozboot: Unbuffer stdout.

https://reviewboard.mozilla.org/r/93878/#review94274

	Ralph Giles (:rillian) Assignee
	Comment 22 • 8 years ago
mozreview-review-reply

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!

	Ralph Giles (:rillian) Assignee
	Comment 23 • 8 years ago
mozreview-review

Comment on attachment 8811996 [details]
Bug 1286799 - mozboot: Install or upgrade rust.

https://reviewboard.mozilla.org/r/93874/#review94306

	Ralph Giles (:rillian) Assignee
	Comment 24 • 8 years ago
mozreview-review-reply

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?

	Gregory Szorc [:gps]
	Comment 42 • 8 years ago
mozreview-review

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).

	Gregory Szorc [:gps]
	Comment 43 • 8 years ago
mozreview-review-reply

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

	Gregory Szorc [:gps]
	Comment 44 • 8 years ago
mozreview-review

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.

	Gregory Szorc [:gps]
	Comment 45 • 8 years ago
mozreview-review

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.

	Gregory Szorc [:gps]
	Comment 46 • 8 years ago
mozreview-review

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.

	Ralph Giles (:rillian) Assignee
	Comment 47 • 8 years ago
mozreview-review-reply

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.

	Ralph Giles (:rillian) Assignee
	Comment 48 • 8 years ago
mozreview-review-reply

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.

	Gregory Szorc [:gps]
	Comment 54 • 8 years ago
mozreview-review

Comment on attachment 8811996 [details]
Bug 1286799 - mozboot: Install or upgrade rust.

https://reviewboard.mozilla.org/r/93874/#review94982

	Gregory Szorc [:gps]
	Comment 55 • 8 years ago
mozreview-review

Comment on attachment 8812342 [details]
Bug 1286799 - mozboot: Use requests to download rustup manifest.

https://reviewboard.mozilla.org/r/94126/#review94984

	Pulsebot
	Comment 56 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 57 • 8 years ago
bugherder

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