Skip to content

make cargo install ignore .cargo/config#5874

Merged
bors merged 6 commits into
rust-lang:masterfrom
japaric:revert-revert-gh5606
Sep 2, 2018
Merged

make cargo install ignore .cargo/config#5874
bors merged 6 commits into
rust-lang:masterfrom
japaric:revert-revert-gh5606

Conversation

@japaric

@japaric japaric commented Aug 8, 2018

Copy link
Copy Markdown
Contributor

closes #5850

r? @alexcrichton

@dwijnand dwijnand left a comment

Copy link
Copy Markdown
Contributor

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

ehuss commented Aug 8, 2018

Copy link
Copy Markdown
Contributor

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

@alexcrichton

Copy link
Copy Markdown
Member

r=me with passing tests, thanks @japaric!

@japaric

japaric commented Aug 9, 2018

Copy link
Copy Markdown
Contributor Author

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

dwijnand commented Aug 9, 2018

Copy link
Copy Markdown
Contributor

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

@ehuss

ehuss commented Aug 9, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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

@japaric

japaric commented Sep 1, 2018

Copy link
Copy Markdown
Contributor Author

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

@japaric

japaric commented Sep 1, 2018

Copy link
Copy Markdown
Contributor Author

I simplified the implementation. re-r? @alexcrichton

@dwijnand

dwijnand commented Sep 1, 2018

Copy link
Copy Markdown
Contributor

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 japaric force-pushed the revert-revert-gh5606 branch from f8e6f45 to 942e367 Compare September 1, 2018 16:16
@japaric

japaric commented Sep 1, 2018

Copy link
Copy Markdown
Contributor Author

Rebased and updated / fixed the unit test.

@alexcrichton

Copy link
Copy Markdown
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

bors commented Sep 2, 2018

Copy link
Copy Markdown
Contributor

📌 Commit 942e367 has been approved by alexcrichton

@bors

bors commented Sep 2, 2018

Copy link
Copy Markdown
Contributor

⌛ 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

bors commented Sep 2, 2018

Copy link
Copy Markdown
Contributor

☀️ 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