1298600 - Configuration ignores cargo is missing even if --enable-rust

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Akihiko Odaki]

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 1

[Akihiko Odaki]

Attachment: Bug 1298600 - Do not allow_missing cargo;

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

Comment 2

[Akihiko Odaki]

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

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8785635 [details]
Bug 1298600 - Do not allow_missing cargo;

https://reviewboard.mozilla.org/r/74772/#review72966

Thank you for the patch.

Comment 4

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

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.

Comment 5

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 1 - reindent rustc_info

For whatever reason, rustc_info was indented a bit too far.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 2 - check Cargo's version

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?

Comment 9

[Ralph Giles (:rillian)]

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?

Comment 10

[Ralph Giles (:rillian)]

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.

Comment 11

[Ralph Giles (:rillian)]

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

[Mike Hommey [:glandium]]

It's really sad that cargo doesn't have a --version --verbose output like rustc has.

Comment 13

[Akihiko Odaki]

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

Comment 14

[Nathan Froyd [:froydnj]]

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

Comment 16

[Nathan Froyd [:froydnj]]

Attachment: part 1 - reindent rustc_info

Carrying over r+.

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: part 2 - check Cargo's version

Addressed review feedback with a more complete regex and slightly modified how
we obtain the minimum Cargo version.

Comment 18

[Ralph Giles (:rillian)]

Comment on attachment 8836106 [details] [diff] [review]
part 2 - check Cargo's version

Review of attachment 8836106 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

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

https://hg.mozilla.org/mozilla-central/rev/449d0b094ece
https://hg.mozilla.org/mozilla-central/rev/79f23b8d3f2b