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

Use 'cargo check' to build the sysroot and target crate #1136

Merged
merged 4 commits into from
Feb 24, 2020

Conversation

Aaron1011
Copy link
Member

Fixes #1057

I'm using my original approach from PR #1048. Ideally, we would
distinguish between build-deps/dependencies/'final crate' via a
different approach (e.g. the target directory). However, I
haven't been able to get that to work just yet.

However, everything should be working with the approach I'm using. At a
minimum, we can use this PR to verify that everything works as expected
when we don't actually produce native build outputs.

@Aaron1011 Aaron1011 force-pushed the feature/check-libstd branch from b07f3ac to 29b2fad Compare January 1, 2020 09:18
@@ -197,7 +227,7 @@ fn xargo() -> Command {
// Bootstrap tells us where to find xargo
Command::new(val)
} else {
Command::new("xargo")
Command::new("xargo-check")
Copy link
Member Author

Choose a reason for hiding this comment

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

It really is that easy 😄

Copy link
Member

Choose a reason for hiding this comment

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

Well, longer-term we'd ideally also stop creating a Cargo.toml. ;)

@RalfJung
Copy link
Member

RalfJung commented Jan 2, 2020

I remain pretty skeptical of the enormous hacks this introduces, and would prefer something relying on --target, as laid out in #1057 (comment).

src/bin/cargo-miri.rs Outdated Show resolved Hide resolved
@Aaron1011 Aaron1011 force-pushed the feature/check-libstd branch 2 times, most recently from df667ff to d980c36 Compare January 9, 2020 11:17
@Aaron1011
Copy link
Member Author

@RalfJung: I've modified this PR to distinguish between crate types using only the command-line arguments passed by Cargo. Using the target directory turned out to be unnecessary - --emit=link, --crate-type bin, and --test is sufficient.

@RalfJung
Copy link
Member

What about --target? Cargo can already distinguish these things for us, that sounds much less fragile.

@Aaron1011
Copy link
Member Author

Aaron1011 commented Jan 15, 2020

@RalfJung: We need to check for --crate-type bin and --test regardless of whether or not we rely on --target - both normal dependencies and the final crate (binary or test artifact) go in the same directory.

I think using --target is actually much more fragile. We need to keep track of (or determine) the root directory as well as the current target triple, so that we can avoid getting tricked by something like /some/path/with/target/x86_64-unknown-linux/gnu/then/more (e.g. a root directory path that shares names with the subdirs that Cargo generates).

I think checking for the presence of one flag --emit=link is much simpler. If it ever breaks, we can just change the implementation of is_build_dep.

@RalfJung
Copy link
Member

Looks like I'll need to look closer into this and do some experiments myself to fully appreciate what is happening here. That might take a few weeks until I find the time, sorry.

@RalfJung
Copy link
Member

I think using --target is actually much more fragile. We need to keep track of (or determine) the root directory as well as the current target triple, so that we can avoid getting tricked by something like /some/path/with/target/x86_64-unknown-linux/gnu/then/more (e.g. a root directory path that shares names with the subdirs that Cargo generates).

I don't think we need to look at the root directory at all, we can just call rustc to ask it for the default target.

But you are right that this is not really much less hacky than your variant, if at all. So since this works, we can just use it. I did some slight refactoring like moving the test methods down inside inside_cargo_rustc to make that code more local; this should be good to go. Sorry it took me so long!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2020

📌 Commit 7bcf51a has been approved by RalfJung

@bors

This comment has been minimized.

Aaron1011 and others added 2 commits February 24, 2020 14:41
Fixes rust-lang#1057

Since we are no longer using "cargo rustc", we now use the rustc
arguments passed by Cargo to determine whether we are building a
build dependency, normal dependency, or "target" (final binary or test)
crate.
@RalfJung RalfJung force-pushed the feature/check-libstd branch from 7bcf51a to 443163f Compare February 24, 2020 13:41
@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2020

📌 Commit 47f2b12 has been approved by RalfJung

bors added a commit that referenced this pull request Feb 24, 2020
Use 'cargo check' to build the sysroot and target crate

Fixes #1057

I'm using my original approach from PR #1048. Ideally, we would
distinguish between build-deps/dependencies/'final crate' via a
different approach (e.g. the target directory). However, I
haven't been able to get that to work just yet.

However, everything should be working with the approach I'm using. At a
minimum, we can use this PR to verify that everything works as expected
when we don't actually produce native build outputs.
@bors
Copy link
Contributor

bors commented Feb 24, 2020

⌛ Testing commit 47f2b12 with merge fc08cce...

@bors
Copy link
Contributor

bors commented Feb 24, 2020

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2020

📌 Commit faf7bf5 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Feb 24, 2020

⌛ Testing commit faf7bf5 with merge 36305e3...

@bors
Copy link
Contributor

bors commented Feb 24, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 36305e3 to master...

@bors bors merged commit 36305e3 into rust-lang:master Feb 24, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 24, 2020
…c-morse

no more codegen for miri_start_panic

With rust-lang/miri#1136 landed, we don't generate code any more for crates that will be run by Miri. So the LLVM backend does not have to implement the `miri_start_panic` intrinsic any more.

Cc @Aaron1011
}

fn cargo() -> Command {
if let Ok(val) = std::env::var("CARGO") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to use env;:var_os when ever it is possible.

Copy link
Member

Choose a reason for hiding this comment

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

That probably makes sense, if we don't need it to really be UTF-8... do you want to submit a PR?

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.

Build dependencies without codegen ("cargo check") where possible
4 participants