Closed
Bug 1298600
Opened 8 years ago
Closed 8 years ago
Configuration ignores cargo is missing even if --enable-rust
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: akihiko.odaki, Assigned: froydnj)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160823070410
Steps to reproduce:
Tried to build with --enable-rust but not having cargo.
Actual results:
Configuration ignores that cargo is missing.
INFO: checking for cargo...
DEBUG: cargo: Trying cargo
INFO: not found
Building fails if it's missing:
0:07.71 force-cargo-build
0:07.71 env: ':': No such file or directory
0:07.71 make[6]: *** [/home/root3/mozilla-central/config/rules.mk:939: force-cargo-build] Error 127
(snip)
0:07.71 make[5]: *** [/home/root3/mozilla-central/config/recurse.mk:71: toolkit/library/rust/target] Error 2
0:07.73 make[1]: *** [client.mk:168: build] Error 2
0:07.78 849 compiler warnings present.
make: *** [GNUmakefile:9: build] Error 2
Expected results:
Configuration fails when it finds cargo is missing.
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Component: Untriaged → Build Config
Product: Firefox → Core
Reporter | ||
Comment 2•8 years ago
|
||
The build always depends on 'gkrust' and 'gkrust-gtest' if --enable-rust
library/gtest/moz.build:
if CONFIG['MOZ_RUST']:
USE_LIBS += [
'gkrust-gtest',
]
library/moz.build
# The above library needs to be last for C++ purposes. This library,
# however, is entirely composed of Rust code, and needs to come after
# all the C++ code so any possible C++ -> Rust calls can be resolved.
if CONFIG['MOZ_RUST']:
USE_LIBS += ['gkrust']
So it shouldn't allow missing cargo. The review request has the change to alter the value of allow_missing.
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8785635 [details]
Bug 1298600 - Do not allow_missing cargo;
https://reviewboard.mozilla.org/r/74772/#review72966
Thank you for the patch.
Attachment #8785635 -
Flags: review?(cmanchester) → review+
I found this becomes annoying since rust enabled by default.
We will need this in m-c, or a better message that tells user to install cargo.
Assignee | ||
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8785635 [details]
Bug 1298600 - Do not allow_missing cargo;
https://reviewboard.mozilla.org/r/74772/#review99838
::: build/moz.configure/rust.configure:20
(Diff revision 1)
> def cargo_binary_names(value):
> if value:
> return ['cargo']
>
> rustc = check_prog('RUSTC', rust_compiler_names, allow_missing=True)
> -cargo = check_prog('CARGO', cargo_binary_names, allow_missing=True)
> +cargo = check_prog('CARGO', cargo_binary_names, allow_missing=False)
This doesn't seem like the correct fix, since we still (nominally) support `--disable-rust`. The better fix would be to keep `allow_missing=True` here, and complain in `rust_compiler` later on in this file, similar to how that function already complains if `rustc` isn't found.
Attachment #8785635 -
Flags: review-
Assignee | ||
Comment 7•8 years ago
|
||
For whatever reason, rustc_info was indented a bit too far.
Attachment #8828871 -
Flags: review?(giles)
Assignee | ||
Updated•8 years ago
|
Attachment #8785635 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
We need a particular version of Cargo, and we can't assume that because
the user has Rust 1.X, they necessarily have Cargo 0.(X+1).
I am ambivalent on how to best keep Rust and Cargo versions in sync, or even if
we want to do that. If this way is too cryptic, maybe we should just have:
rustc_min_version = Version('1.13')
cargo_min_version = Version('0.14')
and trust that people will keep those in sync if they need to?
Attachment #8828872 -
Flags: review?(giles)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nfroyd
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 9•8 years ago
|
||
Comment on attachment 8828871 [details] [diff] [review]
part 1 - reindent rustc_info
Review of attachment 8828871 [details] [diff] [review]:
-----------------------------------------------------------------
::: build/moz.configure/rust.configure
@@ +46,5 @@
> die('Failed to call cargo: %s', e.message)
>
> set_config('MOZ_CARGO_SUPPORTS_FROZEN', cargo_supports_frozen)
>
> +@depends('--enable-rust', rustc, cargo, rustc_info)
This change should be in the other patch?
Attachment #8828871 -
Flags: review?(giles) → review+
Comment 10•8 years ago
|
||
Comment on attachment 8828872 [details] [diff] [review]
part 2 - check Cargo's version
Review of attachment 8828872 [details] [diff] [review]:
-----------------------------------------------------------------
::: build/moz.configure/rust.configure
@@ +35,5 @@
> +def cargo_info(cargo):
> + # |cargo --version| doesn't have pleasant-to-parse output like rustc
> + # does. So we have to resort to regexes.
> + out = check_cmd_output(cargo, '--version').splitlines()[0]
> + VERSION_FORMAT = r'^cargo (\d\.\d+\.\d).*'
Although it's unlikely, any of the version subfields could be multiple digits. I'd at least allow patch version > 9.
@@ +86,5 @@
> +
> + rustc_min_version = Version('{}.{}'.format(rustc_major_version,
> + rustc_minor_version))
> + cargo_min_version = Version('{}.{}'.format(cargo_major_version,
> + cargo_minor_version))
How about:
rustc_min_version = Version('1.13')
cargo_min_version = Version('{}.{}'.format(rustc_min_version.major - 1,
rustc_min_version.minor + 1))
Shorter and easier to update.
Please also add a comment explaining that we're requiring the version of cargo corresponding to the minimum rustc version, since the release numbering offset isn't obvious.
Attachment #8828872 -
Flags: review?(giles)
Comment 11•8 years ago
|
||
Come to think of it, I imagine cargo will start making 1.x releases before rust gets to 2.x. This check will still work when that happens, but it means we could go with the simpler, semantically-equivalent `cargo_min_version = Version('0.{}'.format(rust_min_version.minor + 1))`. I don't feel strongly though.
Comment 12•8 years ago
|
||
It's really sad that cargo doesn't have a --version --verbose output like rustc has.
Reporter | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8785635 [details]
Bug 1298600 - Do not allow_missing cargo;
https://reviewboard.mozilla.org/r/74772/#review99838
> This doesn't seem like the correct fix, since we still (nominally) support `--disable-rust`. The better fix would be to keep `allow_missing=True` here, and complain in `rust_compiler` later on in this file, similar to how that function already complains if `rustc` isn't found.
It is OK because the `check_prog` call depends on `cargo_binary_names`, and `cargo_binary_names` depends on `'--enable-rust'`.
It is the same logic with `rustc` variable. It is defined with `check_prog` call, and it depends on `rust_compiler_names`. `rust_compiler_names` depends on `'--enable-rust'`.
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #12)
> It's really sad that cargo doesn't have a --version --verbose output like
> rustc has.
This is https://github.com/rust-lang/cargo/issues/3584 if folks are interested.
Assignee | ||
Updated•8 years ago
|
Attachment #8828871 -
Attachment is obsolete: true
Assignee | ||
Comment 17•8 years ago
|
||
Addressed review feedback with a more complete regex and slightly modified how
we obtain the minimum Cargo version.
Attachment #8836106 -
Flags: review?(giles)
Assignee | ||
Updated•8 years ago
|
Attachment #8828872 -
Attachment is obsolete: true
Comment 18•8 years ago
|
||
Comment on attachment 8836106 [details] [diff] [review]
part 2 - check Cargo's version
Review of attachment 8836106 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. Thanks!
Attachment #8836106 -
Flags: review?(giles) → review+
Comment 20•8 years ago
|
||
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/449d0b094ece
part 1 - reindent rustc_info; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/79f23b8d3f2b
part 2 - check Cargo's version; r=rillian
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/449d0b094ece
https://hg.mozilla.org/mozilla-central/rev/79f23b8d3f2b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•