keep track of crates that are whitelisted to be used even if yanked #6611

Eh2406 · 2019-01-28T21:34:25Z

This is a start on #6609. It definitely needs tests that the bug is fixed, and to reduce the clones. But for now let's see what CI thinks.

src/cargo/ops/cargo_install.rs

alexcrichton · 2019-01-28T22:16:05Z

src/cargo/core/source/source_id.rs

+    pub fn load<'a>(
+        self,
+        config: &'a Config,
+        yanked_whitelist: HashSet<PackageId>,


It might help clean things up to plumb through &HashSet<PackageId> mostly and then only clone when necessary at the bottom

I have been experimenting but plumbing thure the lifetime can be annoying. I will try again.

Oh and to be clear I wouldn't connect the lifetime to anything, only passing around a borrow and then when it needs to be stored cloning it

src/cargo/core/registry.rs

src/cargo/sources/registry/index.rs

bors · 2019-01-29T00:50:41Z

☔ The latest upstream changes (presumably #6591) made this pull request unmergeable. Please resolve the merge conflicts.

Eh2406 · 2019-01-29T22:31:11Z

I think I have addressed your suggestions. Next time I look at this it will be time to add a test, suggestions welcome.

Eh2406 · 2019-01-30T17:28:34Z

Ok that is 2 tests and it seems to not be working. :-(
I don't know why not. @alexcrichton suggestions?

alexcrichton · 2019-01-30T18:27:04Z

Oh so I bet this is related to replaced sources. In src/cargo/sources/config.rs I think that you'll need to apply the config mappings to the whitelisted ids?

alexcrichton · 2019-01-30T18:27:30Z

(the "related to" being that the package ids aren't equal I think because the source ids change as they flow through the config source)

Eh2406 · 2019-01-30T22:48:27Z

Still not passing. Is it something silly? I am worried that it has to do with poisoning all of a registry leading to us whitelisting to little.

alexcrichton · 2019-01-30T22:58:07Z

Hm without digging in too much just yet, it may be that the replacement happens the other way around or something like that? Or maybe something needs to be swapped?

It may be worthwhile to add some debugging prints and such to see if the failing tests can be diagnosed more directly too

Eh2406 · 2019-01-31T01:19:11Z

cargo\sources\registry\index.rs

    pub fn query_inner(
...

+        if !yanked_whitelist.is_empty() {
+            println!("dep.name: {:?} dep.source_id(): {:?}", name, dep.source_id().to_string());
+            println!("yanked_whitelist: {:?}", yanked_whitelist);
+        }

then running yanks_in_lockfiles_are_ok_with_new_dep

dep.name: "bar" dep.source_id(): "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}
dep.name: "baz" dep.source_id(): "registry `C:\\....\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}
dep.name: "bar" dep.source_id(): "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}
dep.name: "bar" dep.source_id(): "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}

So "bar" is not making it to the yanked_whitelist

yanks_in_lockfiles_are_ok_for_other_update

running `C:\Users\...\cargo\target\debug\cargo.exe build`
running `C:\Users\...\cargo\target\debug\cargo.exe build`
dep.name: "bar" dep.source_id(): "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "baz", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "bar", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}
dep.name: "baz" dep.source_id(): "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "baz", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "bar", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}
dep.name: "bar" dep.source_id(): "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "baz", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "bar", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}
dep.name: "baz" dep.source_id(): "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`"
yanked_whitelist: {PackageId { name: "baz", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "bar", version: "0.0.1", source: "registry `C:\\...\\cargo\\target\\cit\\t0\\registry`" }, PackageId { name: "foo", version: "0.0.1", source: "C:\\...\\cargo\\target\\cit\\t0\\foo" }}

So I think we are mapping the source correctly

alexcrichton · 2019-01-31T16:23:09Z

Oh right, turns out this location is too aggressive and is the wrong place for this check.

Let's go back to an explicit method to add an entry to the whitelist, and then regardless of update flags add the entire previous lockfile to the whitelist.

This reverts commit 4fb53e5

Eh2406 · 2019-01-31T17:11:27Z

Ok reverted the relevant commit. I think entire previous lockfile may be two brod. cargo update no longer errores, there is a resolution the old lockfile.

Eh2406 · 2019-01-31T23:01:01Z

Given the number of people who are dealing with broken builds, feel free to push to this PR if that is faster than explaining an idea.

If we're updating a crate then its previous version in the lock file is no longer whitelisted. We may want to perhaps change this in the future!

alexcrichton · 2019-02-01T15:52:22Z

Ok I've pushed up a few commits I found locally, but they don't fix the issue. I have, however, found the issue! The problem is that we're loading a registry when the whitelist is empty, and then afterwards we're adding to the whitelist. The update of the whitelist in the second step isn't getting propagated to the registry source because the registry source is always loaded.

The offending add is here:

cargo/src/cargo/ops/cargo_generate_lockfile.rs

Line 84 in d75d1fb

registry.add_sources(sources)?;

I'm open to possible solutions. I think we'll want some sort of assertion one way or another to ensure this doesn't happen (e.g. if the whitelist is updated the are no loaded sources), but that's somewhat difficult as we add sources in a few places. Otherwise we could try just taking the whitelist a lot sooner, but that may make taking the whitelist a bit larger.

Other solutions could involve passing the whitelist through the query method perhaps

Eh2406 · 2019-02-02T17:40:13Z

I don't think I understand the levels of abstractions well enough to have anything intelligent to say on how this "should" be solved. If you have a more concrete set of steps I can try to follow them.

When sources already exist in a `PackageRegistry` and the whitelist is updated in the package registry, be sure to update all the existing sources to ensure that everyone gets the same view of the intended whitelist.

alexcrichton · 2019-02-03T09:52:48Z

Ok I've pushed a commit which I believe fixes the tests and makes this pass everything now, want to take a look @Eh2406?

Eh2406 · 2019-02-03T14:56:05Z

That is a clever solution. LGTM, for all I understand it.

alexcrichton · 2019-02-04T07:49:21Z

@bors: r+

Alright, let's merge this then!

bors · 2019-02-04T07:49:22Z

📌 Commit 4a2f810 has been approved by alexcrichton

keep track of crates that are whitelisted to be used even if yanked This is a start on #6609. It definitely needs tests that the bug is fixed, and to reduce the clones. But for now let's see what CI thinks.

bors · 2019-02-04T08:14:20Z

⌛ Testing commit 4a2f810 with merge 37ad03f...

bors · 2019-02-04T08:51:10Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 37ad03f to master...

Bump cargo to 865cb70 Merged PRs: * Replace util::without_prefix with Path::strip_prefix rust-lang/cargo#6620 * keep track of crates that are whitelisted to be used even if yanked rust-lang/cargo#6611 * Fix default DYLD_FALLBACK_LIBRARY_PATH on MacOS. rust-lang/cargo#6625 * Bail when trying to run "test --doc --no-run" rust-lang/cargo#6628 * In cargo test's help, add that examples are built rust-lang/cargo#6619 * Extract & re-use filter_targets in cargo_compile rust-lang/cargo#6621 * Test cleanup: remove unnecessary with_status(0) rust-lang/cargo#6630 * Fix run's help message rust-lang/cargo#6631 * Some updates to bash completion. rust-lang/cargo#6644 * Introduce Source::download_now rust-lang/cargo#6637 * Switch from unused_imports to deprecated to test unfixable warnings rust-lang/cargo#6649

[BackPort] Conservative updates This is a back port of #6611 as discussed in #6609

rust-highfive assigned alexcrichton Jan 28, 2019

alexcrichton reviewed Jan 28, 2019

View reviewed changes

Eh2406 force-pushed the conservative-updates branch 2 times, most recently from 7ea62a1 to 3608368 Compare January 29, 2019 21:59

Eh2406 added 2 commits January 29, 2019 17:20

keep track of crates that are whitelisted to be used even if yanked

4400514

only whitelist packages we are keeping from the lockfile

4fb53e5

Eh2406 force-pushed the conservative-updates branch from 85bbda3 to 9a6a382 Compare January 29, 2019 22:23

push the rest of the clones to the bottom

192073b

Eh2406 force-pushed the conservative-updates branch from 9a6a382 to 192073b Compare January 29, 2019 22:33

[WIP] add tests why does this not pass!?

6953ef8

Eh2406 force-pushed the conservative-updates branch from 97609cd to 6953ef8 Compare January 30, 2019 17:26

map_source for ReplacedSource

756f54c

Revert "only whitelist packages we are keeping from the lockfile"

2f7eb42

This reverts commit 4fb53e5

alexcrichton added 2 commits February 1, 2019 07:48

Don't add the entire resolve to the yanked whitelist

2bbce85

If we're updating a crate then its previous version in the lock file is no longer whitelisted. We may want to perhaps change this in the future!

Fix a test assertion

086a454

Update existing sources when whitelist modified

4a2f810

When sources already exist in a `PackageRegistry` and the whitelist is updated in the package registry, be sure to update all the existing sources to ensure that everyone gets the same view of the intended whitelist.

alexcrichton changed the title ~~[WIP] keep track of crates that are whitelisted to be used even if yanked~~ keep track of crates that are whitelisted to be used even if yanked Feb 4, 2019

bors merged commit 4a2f810 into rust-lang:master Feb 4, 2019

Eh2406 deleted the conservative-updates branch February 9, 2019 03:34

pietroalbini mentioned this pull request Feb 10, 2019

Bump cargo to 865cb70 rust-lang/rust#58358

Merged

This was referenced Feb 11, 2019

Conservative updates no longer respect yanked crates since Rust 1.26.0 #6609

Closed

[BackPort] Conservative updates #6655

Merged

bors added a commit that referenced this pull request Feb 12, 2019

Auto merge of #6655 - Eh2406:conservative-updates, r=ehuss

f099fe9

[BackPort] Conservative updates This is a back port of #6611 as discussed in #6609

ehuss modified the milestones: 1.34.0, 1.33.0 Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keep track of crates that are whitelisted to be used even if yanked #6611

keep track of crates that are whitelisted to be used even if yanked #6611

Eh2406 commented Jan 28, 2019 •

edited

Loading

alexcrichton Jan 28, 2019

Eh2406 Jan 28, 2019

alexcrichton Jan 28, 2019

bors commented Jan 29, 2019

Eh2406 commented Jan 29, 2019

Eh2406 commented Jan 30, 2019

alexcrichton commented Jan 30, 2019

alexcrichton commented Jan 30, 2019

Eh2406 commented Jan 30, 2019

alexcrichton commented Jan 30, 2019

Eh2406 commented Jan 31, 2019

alexcrichton commented Jan 31, 2019

Eh2406 commented Jan 31, 2019

Eh2406 commented Jan 31, 2019

alexcrichton commented Feb 1, 2019

Eh2406 commented Feb 2, 2019

alexcrichton commented Feb 3, 2019

Eh2406 commented Feb 3, 2019

alexcrichton commented Feb 4, 2019

bors commented Feb 4, 2019

bors commented Feb 4, 2019

bors commented Feb 4, 2019

keep track of crates that are whitelisted to be used even if yanked #6611

keep track of crates that are whitelisted to be used even if yanked #6611

Conversation

Eh2406 commented Jan 28, 2019 • edited Loading

alexcrichton Jan 28, 2019

Choose a reason for hiding this comment

Eh2406 Jan 28, 2019

Choose a reason for hiding this comment

alexcrichton Jan 28, 2019

Choose a reason for hiding this comment

bors commented Jan 29, 2019

Eh2406 commented Jan 29, 2019

Eh2406 commented Jan 30, 2019

alexcrichton commented Jan 30, 2019

alexcrichton commented Jan 30, 2019

Eh2406 commented Jan 30, 2019

alexcrichton commented Jan 30, 2019

Eh2406 commented Jan 31, 2019

alexcrichton commented Jan 31, 2019

Eh2406 commented Jan 31, 2019

Eh2406 commented Jan 31, 2019

alexcrichton commented Feb 1, 2019

Eh2406 commented Feb 2, 2019

alexcrichton commented Feb 3, 2019

Eh2406 commented Feb 3, 2019

alexcrichton commented Feb 4, 2019

bors commented Feb 4, 2019

bors commented Feb 4, 2019

bors commented Feb 4, 2019

Eh2406 commented Jan 28, 2019 •

edited

Loading