Closed Bug 1396993 Opened 7 years ago Closed 7 years ago

Look in ~/.cargo/bin for rustc+cargo even they're not in $PATH

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 wontfix, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(1 file)

bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH.
59 bytes, text/x-review-board-request
rillian
: review+
Details 
rustup installs rustc+cargo to ~/.cargo/bin by default, so configure should look there to avoid the annoying case where someone installed rust+cargo (possibly via `mach bootstrap`) but forgot to add ~/.cargo/bin to their PATH. Presumably most people that are doing actual Rust development will have done so, but people that just want to build Firefox may not, so we should make the build work for the common case.
Comment on attachment 8904720 [details]
bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH.

https://reviewboard.mozilla.org/r/176512/#review181578

::: build/moz.configure/rust.configure:13
(Diff revision 1)
> +def rustup_bin_path():
> +    return [expanduser('~/.cargo/bin')]
> +
>  # Rust is required by `rust_compiler` below. We allow_missing here
>  # to propagate failures to the better error message there.
> -rustc = check_prog('RUSTC', ['rustc'], allow_missing=True)
> +rustc = check_prog('RUSTC', ['rustc'], allow_missing=True, paths=rustup_bin_path)

This will only look in ~/.cargo/bin and nowhere else.
Comment on attachment 8904720 [details]
bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH.

https://reviewboard.mozilla.org/r/176512/#review181590

::: build/moz.configure/rust.configure:9
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +@imports(_from='os.path', _import='expanduser')
> +def rustup_bin_path():
> +    return [expanduser('~/.cargo/bin')]

Does this do the correct thing in the msys environment we build under on windows?

Also, it looks like `rustup` checks both `CARGO_HOME` and `RUSTUP_HOME` in the environment. Handling those doesn't need to block this patch though.

::: build/moz.configure/rust.configure:14
(Diff revision 1)
> +    return [expanduser('~/.cargo/bin')]
> +
>  # Rust is required by `rust_compiler` below. We allow_missing here
>  # to propagate failures to the better error message there.
> -rustc = check_prog('RUSTC', ['rustc'], allow_missing=True)
> -cargo = check_prog('CARGO', ['cargo'], allow_missing=True)
> +rustc = check_prog('RUSTC', ['rustc'], allow_missing=True, paths=rustup_bin_path)
> +cargo = check_prog('CARGO', ['cargo'], allow_missing=True, paths=rustup_bin_path)

It looks like you'd have to duplicate the path lookup logic in find_program to add `~/.cargo/bin` to the end of the default PATH.

I'd suggest either abstracting that into a separate function so you can call it here, and in `find_program`, or just check for failure here and call again with `rustup_bin_path`.

Note that we want the check `~/.cargo/bin` *after* everything in PATH so customizations work as expected.
Attachment #8904720 - Flags: review?(giles) → review-
See toolchain_search_path in toolchain.configure.
The way we did it for the llvm-config that gets installed via `mach bootstrap` is to search for:

['llvm-config', ...other variant spellings..., '/home/path/to/.mozbuild/clang/bin']

and that way you don't need to muck around with passing a special path in, since non-absolute filenames get searched along $PATH and absolute filenames just get checked.  Whether that's the Right Way to do things...
(In reply to Nathan Froyd [:froydnj] from comment #5)
> The way we did it for the llvm-config that gets installed via `mach
> bootstrap` is to search for:
> 
> ['llvm-config', ...other variant spellings...,
> '/home/path/to/.mozbuild/clang/bin']

Thanks! That seems very sensible so I copied that approach.
Comment on attachment 8904720 [details]
bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH.

https://reviewboard.mozilla.org/r/176512/#review181932

Thanks for fixing. Looks good to me except for the naming issue.

::: build/moz.configure/rust.configure:8
(Diff revision 2)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +@imports(_from='os.path', _import='expanduser')
> +def rustup_path(what):

Calling this `rustup_path` makes me think it just wraps the command name, not that it tries both. Maybe call this `append_rustup_path` or `add_rustup_path` instead?
Attachment #8904720 - Flags: review?(giles) → review+
Comment on attachment 8904720 [details]
bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH.

https://reviewboard.mozilla.org/r/176512/#review182008
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a93894dd13faa7acafc2c175e6e1ddea32c2599
bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH. r=rillian
Comment on attachment 8904720 [details]
bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH.

https://reviewboard.mozilla.org/r/176512/#review181590

> Does this do the correct thing in the msys environment we build under on windows?
> 
> Also, it looks like `rustup` checks both `CARGO_HOME` and `RUSTUP_HOME` in the environment. Handling those doesn't need to block this patch though.

For the record:
```
>>> os.path.expanduser('~/.cargo/bin')
'c:/Users/ted/.cargo/bin'

$ which rustc
/c/Users/ted/.cargo/bin/rustc.exe
```
Comment on attachment 8904720 [details]
bug 1396993 - Look in ~/.cargo/bin for rustc+cargo even they're not in PATH.

https://reviewboard.mozilla.org/r/176512/#review181932

> Calling this `rustup_path` makes me think it just wraps the command name, not that it tries both. Maybe call this `append_rustup_path` or `add_rustup_path` instead?

I renamed this to `add_rustup_path`.
https://hg.mozilla.org/mozilla-central/rev/4a93894dd13f
https://hg.mozilla.org/mozilla-central/rev/f5e947cf3133
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: