Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo run supports --package argument #3691

Merged
merged 1 commit into from
Feb 14, 2017
Merged

Conversation

matklad
Copy link
Member

@matklad matklad commented Feb 13, 2017

closes #3529

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

@bors: r+

Looks great, thanks!

@bors
Copy link
Collaborator

bors commented Feb 13, 2017

📌 Commit a158958 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 13, 2017

⌛ Testing commit a158958 with merge 31aa5eb...

@bors
Copy link
Collaborator

bors commented Feb 13, 2017

💔 Test failed - status-appveyor

@matklad
Copy link
Member Author

matklad commented Feb 13, 2017

Hm, the test failure seems legitimate, but it only triggers on mvsc... Not sure what's going on there, will try to look closer tomorrow.

tests/run.rs Outdated
.file("d3/src/main.rs", "fn main() { println!(\"d2\"); }");

let cargo_process = || {
let mut process_builder = p.cargo_process("run");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this to cargo instead of cargo_process?

Windows doesn't like blowing away target right after we compile something. By default cargo_process will delete the tree and recreate it, where cargo just creates a process.

This'll then just need a p.build() up front.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then perhaps cargo_process should consume ProjectBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I've tried to add dynamic checks that we don't build project twice, and turns out that some tests do, and relay on that:

and

fn cargo_default_env_metadata_env_var() {
looks somewhat suspicious: it both executes cargo clean and uses .build more than once.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah some of these are flaky sometimes but a default-on dynamic check would be useful!

@alexcrichton
Copy link
Member

@bors: r+

Nah this looks good to me!

@bors
Copy link
Collaborator

bors commented Feb 13, 2017

📌 Commit 261ae46 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Feb 14, 2017

⌛ Testing commit 261ae46 with merge 6a93970...

bors added a commit that referenced this pull request Feb 14, 2017
cargo run supports --package argument

closes #3529
@bors
Copy link
Collaborator

bors commented Feb 14, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 6a93970 to master...

@bors bors merged commit 261ae46 into rust-lang:master Feb 14, 2017
@matklad matklad deleted the run-pkg branch February 14, 2017 09:58
bors added a commit that referenced this pull request Feb 21, 2017
Assert that we don't build test project twice.

Discussed in #3691 (comment).

I've modify the offending tests to be more explicit about recreating projects.
@ehuss ehuss added this to the 1.17.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo run in workspace needs to support --package argument
5 participants