Closed Bug 1286799 Opened 3 years ago Closed 3 years ago

mach bootstrap should install Rust via rustup if not present on all supported platforms

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ted, Assigned: rillian)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

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.
Assignee: nobody → giles
Comment on attachment 8811378 [details]
Bug 1286799 - mozboot: Abstract version checking.

https://reviewboard.mozilla.org/r/93496/#review94254
Attachment #8811378 - Flags: review+
Comment on attachment 8811379 [details]
Bug 1286799 - mozboot: Use a helper for mercurial version detection.

https://reviewboard.mozilla.org/r/93498/#review94258
Attachment #8811379 - Flags: 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.
Attachment #8811380 - Flags: review+
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'))`.
Attachment #8811996 - Flags: 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('')
Comment on attachment 8811998 [details]
Bug 1286799 - mozboot: Unbuffer stdout.

https://reviewboard.mozilla.org/r/93878/#review94274
Attachment #8811998 - Flags: review?(gps) → review+
Blocks: 1318756
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).
Attachment #8811996 - Flags: review?(gps) → review-
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.
Attachment #8811997 - Flags: review?(gps) → 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.
Attachment #8812342 - Flags: review?(gps) → 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.
Attachment #8812881 - Flags: review?(gps) → review+
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
Attachment #8811996 - Flags: review?(gps) → review+
Comment on attachment 8812342 [details]
Bug 1286799 - mozboot: Use requests to download rustup manifest.

https://reviewboard.mozilla.org/r/94126/#review94984
Attachment #8812342 - Flags: review?(gps) → review+
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
Depends on: 1319860
See Also: → 1320940
Blocks: 1321691
Depends on: 1326393
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.