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

Fix new unused patch warning. #6494

Merged
merged 1 commit into from
Dec 28, 2018
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Dec 28, 2018

The unused [patch] warning added in #6470 was misfiring if you build only a subset of a workspace that doesn't include the patch.

I'm about 80% confident this new logic is correct. It addresses the scenario I discovered, and I tested a bunch of commands (generate-lockfile, update, etc.) to make sure they all work. However, the many ways that resolve_with_previous is called is tricky to ensure every scenario works correctly.

@rust-highfive
Copy link

r? @dwijnand

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

@dwijnand
Copy link
Member

r=me with one question: why is it no longer conditional on warn?

@ehuss
Copy link
Contributor Author

ehuss commented Dec 28, 2018

It's a bit complicated. A command like cargo build will run this resolve code twice. In resolve_ws_with_method it runs it first via resolve_with_registry to resolve everything, registering patches, but turns warn OFF. The second time it calls resolve_with_previous directly to resolve with the specific features enabled, turns warn ON and register patches OFF. Presumably the warn flag is done so that warnings only appear once, and this sequence is so that only warnings that apply to the current command (set of enabled features) are shown (I'm only guessing).

There are a variety of other ways resolve_with_previous gets called, each with different settings (update, generate-lockfile, publish, metadata --all-features, etc. all do things differently).

These patch warnings need to be done when method==Everything and patches are being registered. Checking register_patches was the most reliable way I could find to ensure this warning fires at the right time for every command.

@dwijnand
Copy link
Member

Excellent, thanks.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 28, 2018

📌 Commit 7d11280 has been approved by dwijnand

@bors
Copy link
Contributor

bors commented Dec 28, 2018

⌛ Testing commit 7d11280 with merge 9bf8592...

bors added a commit that referenced this pull request Dec 28, 2018
Fix new unused patch warning.

The unused `[patch]` warning added in #6470 was misfiring if you build only a subset of a workspace that doesn't include the patch.

I'm about 80% confident this new logic is correct. It addresses the scenario I discovered, and I tested a bunch of commands (generate-lockfile, update, etc.) to make sure they all work. However, the many ways that `resolve_with_previous` is called is tricky to ensure every scenario works correctly.
@bors
Copy link
Contributor

bors commented Dec 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: dwijnand
Pushing 9bf8592 to master...

@bors bors merged commit 7d11280 into rust-lang:master Dec 28, 2018
bors added a commit to rust-lang/rust that referenced this pull request Jan 4, 2019
Update cargo

24 commits in 0d1f1bbeabd5b43a7f3ecfa16540af8e76d5efb4..34320d212dca8cd27d06ce93c16c6151f46fcf2e
2018-12-19 14:45:14 +0000 to 2019-01-03 19:12:38 +0000
- Display environment variables for rustc commands (rust-lang/cargo#6492)
- Fix a very minor race condition in `cargo fix`. (rust-lang/cargo#6515)
- Add a high-level overview of how `fix` works. (rust-lang/cargo#6516)
- Add dependency `registry` to `cargo metadata`. (rust-lang/cargo#6500)
- Fix fingerprint calculation for patched deps. (rust-lang/cargo#6493)
- serialize version directly (rust-lang/cargo#6512)
- use DYLD_FALLBACK_LIBRARY_PATH for dylib_path_envvar on macOS (rust-lang/cargo#6355)
- Fix error message when resolving dependencies (rust-lang/cargo#6510)
- use PathBuf in cargo metadata (rust-lang/cargo#6511)
- Fixed link to testsuite in CONTRIBUTING.md (rust-lang/cargo#6506)
- Update display of contents of Cargo.toml (rust-lang/cargo#6501)
- Update display of contents of Cargo.toml (rust-lang/cargo#6502)
- Fixup cargo install's help message (rust-lang/cargo#6495)
- testsuite: Require failing commands to check output. (rust-lang/cargo#6497)
- Delete unnecessary 'return' (rust-lang/cargo#6496)
- Fix new unused patch warning. (rust-lang/cargo#6494)
- Some minor documentation changes. (rust-lang/cargo#6481)
- Add `links` to `cargo metadata`. (rust-lang/cargo#6480)
- Salvaged semver work (rust-lang/cargo#6476)
- Warn on unused patches. (rust-lang/cargo#6470)
- don't write a an incorrect rustc version to the fingerprint file (rust-lang/cargo#6473)
- Rewrite `login` and registry cleanups. (rust-lang/cargo#6466)
- [issue#6461] Fix cargo commands list (rust-lang/cargo#6462)
- Restrict registry names to same style as package names. (rust-lang/cargo#6469)
@ehuss ehuss added this to the 1.33.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.

4 participants