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

make cargo install ignore .cargo/config #5874

Merged
merged 6 commits into from
Sep 2, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Aug 8, 2018

closes #5850

r? @alexcrichton

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Hehe, nice - so simple :) LGTM

@ehuss
Copy link
Contributor

ehuss commented Aug 8, 2018

May want to add a small note in the documentation that it is ignored during install (such as here).

@alexcrichton
Copy link
Member

r=me with passing tests, thanks @japaric!

@japaric
Copy link
Member Author

japaric commented Aug 9, 2018

With this implementation I'm seeing that cargo build --release; cargo install builds the crate twice, which didn't happen before. Can someone point me to the place where Cargo decides whether to recompile artifacts or not? Making CompileMode::Build and CompileMode::Install hash to the same value would probably fix this but I'd like to understand better how this mechanism works.

@dwijnand
Copy link
Member

dwijnand commented Aug 9, 2018

Not sure exactly where, but (in case it helps) src/cargo/core/compiler/fingerprint.rs is involved AFAIK.

@ehuss
Copy link
Contributor

ehuss commented Aug 9, 2018

I'm not sure a new CompileMode is necessary. IIUC, the only change you want to make is to ensure BuildConfig::requested_target is set correctly? If so, then you can just pass in a new flag to BuildConfig::new or just mutate the value in install:exec where it is already munging the build_config values. I think that would simplify things.

@alexcrichton
Copy link
Member

ping @japaric, wanted to circle back to see if you've had a chance to work on this?

@japaric
Copy link
Member Author

japaric commented Sep 1, 2018

Sorry, I've been occupied with other WG stuff; I'll revisit this and #5946 today.

@japaric
Copy link
Member Author

japaric commented Sep 1, 2018

I simplified the implementation. re-r? @alexcrichton

@dwijnand
Copy link
Member

dwijnand commented Sep 1, 2018

Some of the testing utilities have changed, so you'll have to adapt the test for that. (I can help if you get stuck, but it should be easy)

@japaric
Copy link
Member Author

japaric commented Sep 1, 2018

Rebased and updated / fixed the unit test.

@alexcrichton
Copy link
Member

@bors: r+

Wow that... is a surprising fix! That seems like something we'll probably fix at some point in Cargo, but there's a test to verify it works so sounds good to me!

Thanks @japaric!

@bors
Copy link
Contributor

bors commented Sep 2, 2018

📌 Commit 942e367 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 2, 2018

⌛ Testing commit 942e367 with merge 78bae45...

bors added a commit that referenced this pull request Sep 2, 2018
make `cargo install` ignore .cargo/config

closes #5850

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Sep 2, 2018

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

@bors bors merged commit 942e367 into rust-lang:master Sep 2, 2018
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.

[RFC] cargo install should ignore the default build target
5 participants