rustc subcommand #1568

sondrele · 2015-05-02T00:04:52Z

Work in progress

I have followed issue #595 for a while now. I hope that this PR can solve it, after it's done of course.

@alexcrichton: on IRC the other day, you suggested adding a Vec<String> to the CompileOptions. I have done this, but wasn't sure what the best way to identify the correct Target would be, so currently everything gets compiled with the additional arguments.
I considered adding a Target structure to the CompileOptions as well, and then in cargo_rustc::build_base_args only append the arguments if the Target matches. What do you think about this? Is there an easier way of figuring out the correct Target?

r? @alexcrichton

- `cargo rustc` is starting out based on `cargo build`

- The new field is a list with arguments to compile the target with. - There should only be one target that gets compiled with these arguments

…tions. - The new tests verifies that the extra arguments gets appended to the command. One is for lib and one is for main - Currently the arguments gets passed on to *every* target that gets built, so the tests only contain one file each

rust-highfive · 2015-05-02T00:05:04Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

alexcrichton · 2015-05-02T00:24:56Z

src/bin/rustc.rs

+Compile a package and all of its dependencies
+
+Usage:
+    cargo rustc [options] [<pkgid>] [--] [<opts>...]


Ah when I originally wrote this spec we didn't have a -p argument, but nowadays this <pkgid> argument should be specified through -p, --package instead.

alexcrichton · 2015-05-02T00:32:26Z

Nice! I left a comment about how I think Profile may be the best place for this to go, but there's a few other things I'd like to see for this:

Can you add some tests where you have some dependencies as well? The dependencies should not be compiled with the custom flags, but the main crate being compiled should work.
Can you add a test where there are multiple top-level targets? I think that cargo rustc should generate an error today if you have, for example, 8 binaries. When you're passing custom flags to the compiler I think we should start out by limiting it to passing it to only one invocation of the compiler.

I'm excited to see progress on this!

sondrele · 2015-05-02T10:22:02Z

Thanks for the quick review!

I'll look into Profile and your other suggestions.
Would this also mean that it should be possible to specify the extra arguments directly in the Cargo.toml?
E.g.

[profile.dev]
opt-level = 0
debug = true
rustc-args = "..."

I have a feeling this could be confusing together with dependencies, since only the final target will be compiled with these flags. Also, that would probably make a new subcommand like this unnecessary.

- This field will be set by the `cargo rustc` command, only if one target is being compiled - The field can not be read from the Cargo.toml

…e that's being compiled instead. - An error will be returned if the length of `targets` is not 1 - The profile of `targets` gets cloned in order to append the extra arguments. - The new test verifies that the build fails due to both `lib` and `main` being compiled

… a dependency. - The test verifies that the trailing arguments only gets passed to the binary being built

…helper functions

- `build_with_args_to_one_of_multiple_binaries`, verify that only one bin gets built - `fails_with_args_to_all_binaries` - `build_with_args_to_one_of_multiple_tests`, same behavious as for the binaries - `build_foo_with_bar_dependency`, verify that bar dependency gets built and only foo gets compiled with args - `build_only_bar_dependency`, build the bar package, that foo depends, on with the extra args

sondrele · 2015-05-05T11:40:04Z

@alexcrichton Have you had a chance to look at this?
What do you think about the extra match and re-assignment to targets in cargo_compile::compile_pkg? Is this how you pictured it?

What do you think about the error message? It can be more descriptive, e.g. by including some more info about the targets that it tries to compile.

Another thing I'm not so sure about concerning the tests, is that they're currently relying on the extra arguments to rustc to come after the -g flag. I'm guessing this is not a safe assumption to make, but currently it works.

alexcrichton · 2015-05-05T17:29:19Z

Ah I will take another look @sondrele! Feel free to ping a PR with a comment as otherwise github doesn't send out notifications. I'd also recommend avoiding editing comments as modifications also don't send out notifications, and a few extra pieces of mail never hurt anyone :)

alexcrichton · 2015-05-05T17:30:21Z

Would this also mean that it should be possible to specify the extra arguments directly in the Cargo.toml?

For now let's avoid adding this step, but it's always a future possibility!

alexcrichton · 2015-05-05T17:31:01Z

Another thing I'm not so sure about concerning the tests, is that they're currently relying on the extra arguments to rustc to come after the -g flag. I'm guessing this is not a safe assumption to make, but currently it works.

Oh don't worry, the tests are super brittle in terms of overhauling the output already, so adding a few more won't harm anyone!

alexcrichton · 2015-05-05T17:34:38Z

src/cargo/ops/cargo_compile.rs

+        &Some(args) => {
+            if targets.len() > 1 {
+                return Err(human("extra arguments to `rustc` can only be \
+                                  invoked for one target"))


Yeah I agree that it would be nice to improve this error message a bit. It would be nice for it to mention the filtering utilities (e.g. --lib and --bin) and then also perhaps provide a suggestion along the lines of "consider passing --lib" (or something like that)

- Dereference arguments before matching - use square brackets for vec! macro

sondrele · 2015-05-06T12:17:00Z

I added an extra line in the error message which hopefully makes it a little better (if the grammar's correct).
It's also possible to add an example to the error message, something like:

...
consider filtering the package by passing e.g. 
`--lib` or `--bin NAME` to specify a single target
example: `cargo rustc --lib -- <opts>...`

alexcrichton · 2015-05-07T02:43:47Z

This all looks fantastic to me, thanks @sondrele! I'm gonna run this past @wycats one last time and then I'll send it off to bors.

alexcrichton · 2015-05-07T19:18:08Z

@bors: r+

bors · 2015-05-07T19:18:09Z

📌 Commit 2cd3c86 has been approved by alexcrichton

bors · 2015-05-07T19:18:10Z

⌛ Testing commit 2cd3c86 with merge 0d75eb6...

@alexcrichton

## Work in progress I have followed issue #595 for a while now. I hope that this PR can solve it, after it's done of course. @alexcrichton: on IRC the other day, you suggested adding a `Vec<String>` to the `CompileOptions`. I have done this, but wasn't sure what the best way to identify the correct `Target` would be, so currently everything gets compiled with the additional arguments. I considered adding a `Target` structure to the `CompileOptions` as well, and then in `cargo_rustc::build_base_args` only append the arguments if the `Target` matches. What do you think about this? Is there an easier way of figuring out the correct `Target`? r? @alexcrichton

bors · 2015-05-07T19:33:57Z

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-32, cargo-win-64

sondrele · 2015-05-07T21:51:53Z

Awesome! Thanks for the thorough reviews! 👍
Looking forward to using it!

mkpankov · 2015-05-08T21:20:04Z

👍

Sondre Lefsaker added 3 commits May 1, 2015 23:19

Add new subcommand rustc.

754938f

- `cargo rustc` is starting out based on `cargo build`

Add a new field to CompileOptions and BuildConfig: target_rustc_args

77bb01e

- The new field is a list with arguments to compile the target with. - There should only be one target that gets compiled with these arguments

rust-highfive assigned alexcrichton May 2, 2015

alexcrichton reviewed May 2, 2015
View reviewed changes

Remove the pkgid arg and replace the profile flag with package.

512b217

Sondre Lefsaker added 5 commits May 2, 2015 16:29

Add new field rustc_args to the Profile

53453c8

- This field will be set by the `cargo rustc` command, only if one target is being compiled - The field can not be read from the Cargo.toml

Modify test to build main like before, but lib also gets built as…

d8cd6f0

… a dependency. - The test verifies that the trailing arguments only gets passed to the binary being built

Cleanup tests - favor more explicit expected result instead of using …

bd4d15f

…helper functions

alexcrichton reviewed May 5, 2015
View reviewed changes

Sondre Lefsaker added 3 commits May 6, 2015 10:29

Update subcommand documentation

dc33bfa

Cleanup:

98c0e2d

- Dereference arguments before matching - use square brackets for vec! macro

Suggest adding flags to filter the package in the error message

2cd3c86

bors merged commit 2cd3c86 into rust-lang:master May 7, 2015

alexcrichton mentioned this pull request May 8, 2015

cargo rustc subcommand #595

Closed

frewsxcv mentioned this pull request Aug 2, 2015

Add a ./mach check servo/servo#5893

Closed

drewcrawford mentioned this pull request Jan 7, 2016

Add additional compiler/linker options in Package.swift swiftlang/swift-package-manager#107

Closed

mkeeter mentioned this pull request Aug 10, 2023

cargo rustc should accept multiple packages #12474

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc subcommand #1568

rustc subcommand #1568

sondrele commented May 2, 2015

rust-highfive commented May 2, 2015

alexcrichton May 2, 2015

alexcrichton commented May 2, 2015

sondrele commented May 2, 2015

sondrele commented May 5, 2015

alexcrichton commented May 5, 2015

alexcrichton commented May 5, 2015

alexcrichton commented May 5, 2015

alexcrichton May 5, 2015

sondrele commented May 6, 2015

alexcrichton commented May 7, 2015

alexcrichton commented May 7, 2015

bors commented May 7, 2015

bors commented May 7, 2015

bors commented May 7, 2015

sondrele commented May 7, 2015

mkpankov commented May 8, 2015

rustc subcommand #1568

rustc subcommand #1568

Conversation

sondrele commented May 2, 2015

Work in progress

rust-highfive commented May 2, 2015

alexcrichton May 2, 2015

Choose a reason for hiding this comment

alexcrichton commented May 2, 2015

sondrele commented May 2, 2015

sondrele commented May 5, 2015

alexcrichton commented May 5, 2015

alexcrichton commented May 5, 2015

alexcrichton commented May 5, 2015

alexcrichton May 5, 2015

Choose a reason for hiding this comment

sondrele commented May 6, 2015

alexcrichton commented May 7, 2015

alexcrichton commented May 7, 2015

bors commented May 7, 2015

bors commented May 7, 2015

bors commented May 7, 2015

sondrele commented May 7, 2015

mkpankov commented May 8, 2015