1353460 - bootstrap.py fails to set the rust path to environment variable PATH without restarting the shell

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Utkarsh Anand]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

python bootstrap.py


Actual results:

<code>
Could not find a Rust compiler.
Will try to install Rust.
Downloading rustup-init... Ok
Running rustup-init...
info: syncing channel updates for 'stable-x86_64-unknown-linux-gnu'
142.1 KiB / 142.1 KiB (100 %) 109.1 KiB/s ETA:   0 s                
info: downloading component 'rustc'
 36.4 MiB /  36.4 MiB (100 %)  10.2 MiB/s ETA:   0 s                
info: downloading component 'rust-std'
 50.0 MiB /  50.0 MiB (100 %)   9.9 MiB/s ETA:   0 s                
info: downloading component 'cargo'
info: installing component 'rustc'
info: installing component 'rust-std'
info: installing component 'cargo'
info: default toolchain set to 'stable'

  stable installed - rustc 1.16.0 (30cf806ef 2017-03-10)


Rust installation complete. You should now have rustc and cargo
in /home/utkarsh/.cargo/bin

The installer tries to add these to your default shell PATH, so
restarting your shell and running this script again may work.
If it doesn't, you'll need to add the new command location
manually.

If restarting doesn't work, edit your shell initialization
script, which may be called ~/.bashrc or ~/.bash_profile or
~/.profile, and add the following line:

    source /home/utkarsh/.cargo/env

Then restart your shell and run the bootstrap script again.
</code>

Then failed to find rust compiler while building firefox from source


Expected results:

It should have atleast displayed a message, Restart before building after the script ends (with mercurial configuration setup.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Opening a new terminal and building firefox worked fine, but using the same terminal did not.  The "restarting your shell" message from rust is pretty far up in the output at that point, and it's not clear it applies to the user running boostrap.

Comment 2

[Utkarsh Anand]

Attachment: Bug 1353460 Asked the user to restart the shell before building from source.

Rust is not installed via package manager by default.
bootstrap manually installs it and prepends the appropriate path to PATH.
But it requires us to start a new shell window in order to build, say, firefox.
It won't detect rust compiler otherwise.

Review commit: https://reviewboard.mozilla.org/r/126582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126582/

Comment 3

[Jan Beich]

Note, bug 1320940 comment 5 implied downstream packages on Tier1 platforms as undesired.

Comment 4

[Utkarsh Anand]

(In reply to Jan Beich from comment #3)
> Note, bug 1320940 comment 5 implied downstream packages on Tier1 platforms
> as undesired.

Speaking specifically for fedora, they almost always keep it up to date! 
Have a look here: https://koji.fedoraproject.org/koji/packageinfo?packageID=22720

Also, if it's approved, should I add cargo to the list too? It was pre-installed on my system, so I didn't add it!

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126582/#review129418

Judging from the commit history of this file, I think :gps is a good person to review this.

I think what you've done installs *two* copies of rust -- one via the OS package, and one installed "manually".  That's going to be problematic, since they are probably different versions!

I suspect that restarting the shell is unavoidable when installing rust locally.  That's OK, we just need to make sure the user is warned about it.  My thinking is to check whether `rustc` is in the $PATH that the bootstrap script started with, and points to the `rustc` that it installed.  If not, then it should issue a warning at the very end to suggest that the user restart their terminal.

Greg, you might have other ideas? :)

Comment 6

[Utkarsh Anand]

(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> Comment on attachment 8854650 [details]
> Bug 1353460 Added 'rust' to the list of packages to be installed on fedora.
> 
> https://reviewboard.mozilla.org/r/126582/#review129418
> 
> Judging from the commit history of this file, I think :gps is a good person
> to review this.
> 
> I think what you've done installs *two* copies of rust -- one via the OS
> package, and one installed "manually".  That's going to be problematic,
> since they are probably different versions!
> 
> I suspect that restarting the shell is unavoidable when installing rust
> locally.  That's OK, we just need to make sure the user is warned about it. 
> My thinking is to check whether `rustc` is in the $PATH that the bootstrap
> script started with, and points to the `rustc` that it installed.  If not,
> then it should issue a warning at the very end to suggest that the user
> restart their terminal.
> 
> Greg, you might have other ideas? :)

I'll do it.

Comment 7

[Utkarsh Anand]

Attachment: Bug 1353460 Asked the user to restart the shell before building from source.

When downloading rust manually, it's mandatory to restart the shell,
if the cargo bin path doesn't exist in /usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/home/utkarsh/.cargo/bin:/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/utkarsh/.local/bin:/home/utkarsh/bin.

Review commit: https://reviewboard.mozilla.org/r/126812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126812/

Comment 8

[Utkarsh Anand]

Comment on attachment 8854861 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126812/diff/1-2/

Comment 9

[Ralph Giles (:rillian)]

(In reply to Dustin J. Mitchell [:dustin] from comment #5)

> I suspect that restarting the shell is unavoidable when installing rust
> locally.

Yes. AFAICT a script being able to export an environment variable would be a security violation.

> That's OK, we just need to make sure the user is warned about it.

Which I tried to do, but clearly the text could be better, or better positioned. Printing a warning at the end of the script run is a good idea. Note we'd need to check the version again, since there may be a rustc and cargo already on-path which are too old.

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126582/#review129530

::: python/mozboot/mozboot/centosfedora.py:70
(Diff revision 1)
>              ]
>  
>              self.packages += [
>                  'python2-devel',
>                  'redhat-rpm-config',
> +                'rust',

FWIW I don't think we should do this. Fedora has been pretty quick with deploying new rust releases, but not always a quick as gecko has been in requiring them. So this will occassionally result in a double-download.

Centos doesn't package rust at all, so this will likely fail on non-fedora systems.

Fedora packages cargo separately from rustc, so if we do do this, we should also include the `cargo` package.

Further, I don't think Fedora has a story for installing cross-libstd yet, so the bootstrap script is setting up for a fennec build we definitely need to install rustup. Maybe move the `rust` and `cargo` packages to `browser_packages`?

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8854861 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126812/#review129534

::: commit-message-9d8fa:2
(Diff revision 2)
> +Bug 1353460 Asked the user to restart the shell before building from source.
> +When downloading rust manually, it's mandatory to restart the shell,

Please put a blank line between the summary and body of your commit message.

::: python/mozboot/mozboot/bootstrap.py:287
(Diff revision 2)
>          if not have_clone:
>              print(SOURCE_ADVERTISE)
>  
>          print(self.finished % name)
> +        system_env_path = os.environ["PATH"].split(":")
> +        if os.environ["HOME"] + "/.cargo/bin" not in system_env_path:

You can use `self.which('rustc')` to check the current path for the executable directly, instead of relying on a hard-coded CARGO_HOME here. If that doesn't work, `self.cargo_home()` will construct the expected local install path for you.

You might also be able to just call `ensure_rust_modern()` again, or abstract the `if not version: ...` block into a helper function and call that from your check. It already does what you want, you just might want to simplify the message so people are less likely to skip it.

Comment 12

[Ralph Giles (:rillian)]

And thanks for taking this on!

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126582/#review129540

Stealing review from gps per IRC. It looks like you're reverting this in the next patch anyway, so please just squash this into the other change when you resubmit.

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126582/#review129542

Comment 15

[Utkarsh Anand]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126582/diff/1-2/

Comment 16

[Ralph Giles (:rillian)]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126582/#review129646

::: python/mozboot/mozboot/bootstrap.py:287
(Diff revision 2)
>          if not have_clone:
>              print(SOURCE_ADVERTISE)
>  
>          print(self.finished % name)
> +        system_env_path = os.environ["PATH"].split(":")
> +        if self.instance.which('rustc')[:-6] not in system_env_path:

`which('rustc')` will return `None` if rustc isn't in the `PATH` so you should be able to just

```python
if not self.instance.which('rustc'):
```
here?

Note it will not print the shell warning if there's another rustc in PATH, e.g. from a system package, but if it's too old the build will still fail at configure time. Just wanted to make sure that's what you intended.

Comment 17

[Josh Stone]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #10)
> Further, I don't think Fedora has a story for installing cross-libstd yet,
> so the bootstrap script is setting up for a fennec build we definitely need
> to install rustup.

FWIW, basic multilib does work -- e.g. x86_64 can install rust-std-static.i686 and have working 32-bit builds.  But that doesn't help for fennec, I know.

Comment 18

[Ralph Giles (:rillian)]

Thanks for the pointer. That's still helpful in that it's enough to run the cargo cross-compilation tests.

Looks like the package has no path conflicts between platforms, it's just that the arm packages aren't available in the x86_64 repo, and Fedora doesn't target Android. Something like a `rust-std-armv7-linux-androideabi.noarch` package?

Comment 19

[Josh Stone]

Generally yes, you just need /usr/lib/rustlib/$triple/lib/*.rlib, and the C toolchain to link it.  I think Rust's build system requires the NDK to build rust-std for android targets, so I'm not sure how we would get that into Fedora proper.

Comment 20

[Utkarsh Anand]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126582/#review129646

> `which('rustc')` will return `None` if rustc isn't in the `PATH` so you should be able to just
> 
> ```python
> if not self.instance.which('rustc'):
> ```
> here?
> 
> Note it will not print the shell warning if there's another rustc in PATH, e.g. from a system package, but if it's too old the build will still fail at configure time. Just wanted to make sure that's what you intended.

Ummm.. actually it was late night, so I was in a hurry and didn't check what which('rustc') was doing. I just printed it to check the output. I'll fix that ASAP. :)

Comment 21

[Utkarsh Anand]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #16)
> Comment on attachment 8854650 [details]
> Bug 1353460 Asked the user to restart the shell before building from source.
> 
> https://reviewboard.mozilla.org/r/126582/#review129646
> 
> ::: python/mozboot/mozboot/bootstrap.py:287
> (Diff revision 2)
> >          if not have_clone:
> >              print(SOURCE_ADVERTISE)
> >  
> >          print(self.finished % name)
> > +        system_env_path = os.environ["PATH"].split(":")
> > +        if self.instance.which('rustc')[:-6] not in system_env_path:
> 
> `which('rustc')` will return `None` if rustc isn't in the `PATH` so you
> should be able to just
> 
> ```python
> if not self.instance.which('rustc'):
> ```
> here?
> 
> Note it will not print the shell warning if there's another rustc in PATH,
> e.g. from a system package, but if it's too old the build will still fail at
> configure time. Just wanted to make sure that's what you intended.

There! I found which in base.py class BaseBootstrapper. You're right! :-P

Comment 22

[Utkarsh Anand]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126582/diff/2-3/

Comment 23

[Ralph Giles (:rillian)]

Comment on attachment 8854650 [details]
Bug 1353460 Asked the user to restart the shell before building from source.

https://reviewboard.mozilla.org/r/126582/#review129944

Thanks for the fixes.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3fc93fb5498e
Asked the user to restart the shell before building from source. r=rillian

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3fc93fb5498e

Comment 26

[Ralph Giles (:rillian)]

Thanks for contributing, Utkarsh!

Comment 27

[Utkarsh Anand]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #26)
> Thanks for contributing, Utkarsh!

My pleasure! :) I'll be happy to help out whenever I can. Thanks, for the reviews and guidance.