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

Revert #1501 in a really roundabout way #1607

Closed
wants to merge 1 commit into from

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Jan 26, 2019

The author of ring has chosen to yank all versions of that crate other
than the most recent. This has had the effect of making it so that we
cannot modify our Cargo.toml without breaking the build. The long term
fix will be to get us on the latest version of cookie (which requires
updating some unmaintained dependencies). I haven't looked into how much
work that will be beyond confirming that it is not as simple as just
changing the version number.

Right now, this is blocking anything that requires updating an existing
dependency, or adding a new dependency. So this is a quick fix to get
our builds working again until we have the time to sort out updating
everything between us and ring.

Unfortunately, this isn't just as simple as reverting #1501, since
[patch] doesn't force cargo to resolve to yanked versions. It'll
instead resolve to really old versions of cookie in an attempt to
remove ring from our dependency tree. So I've instead had to fork
cookie, and change its dependency on Ring to use what we had prior to
#1501.

The author of ring has chosen to yank all versions of that crate other
than the most recent. This has had the effect of making it so that we
cannot modify our `Cargo.toml` without breaking the build. The long term
fix will be to get us on the latest version of cookie (which requires
updating some unmaintained dependencies). I haven't looked into how much
work that will be beyond confirming that it is not as simple as just
changing the version number.

Right now, this is blocking anything that requires updating an existing
dependency, or adding a new dependency. So this is a quick fix to get
our builds working again until we have the time to sort out updating
everything between us and ring.

Unfortunately, this isn't just as simple as reverting rust-lang#1501, since
`[patch]` doesn't force cargo to resolve to yanked versions. It'll
instead resolve to really old versions of `cookie` in an attempt to
remove `ring` from our dependency tree. So I've instead had to fork
cookie, and change its dependency on Ring to use what we had prior to
 rust-lang#1501.
@carols10cents
Copy link
Member

Unfortunately, this isn't just as simple as reverting #1501, since
[patch] doesn't force cargo to resolve to yanked versions. It'll
instead resolve to really old versions of cookie in an attempt to
remove ring from our dependency tree.

I don't understand, reverting #1501 should revert Cargo.lock too, which should resolve yanked versions because they're in Cargo.lock? Can you clarify what you mean by "revert"? If I run git revert 41af25d7310fe990dfd79fe8e0f72fa2801e893f -m 1 and build, I see:

...
Compiling ring v0.11.1 (https://github.com/SergioBenitez/ring?branch=v0.11#ca9fe986)
...
Compiling cookie v0.9.2
...

And it builds successfully for me; I've opened #1608 with that change to see if it passes travis...

Am I missing something?

@sgrif
Copy link
Contributor Author

sgrif commented Jan 27, 2019

Try adding a new dependency

@sgrif
Copy link
Contributor Author

sgrif commented Jan 27, 2019

Just revert it doesn't really change the situation. Our build will still pass right now, since we have a Cargo.lock with the older version. As soon as you do anything to Cargo.toml though, you will cause cargo to re-resolve dependencies and will refuse to resolve Ring.

@carols10cents
Copy link
Member

You're right that I can't update itertools on that branch, but the patch to ring was added a year ago and we've change Cargo.toml/Cargo.lock since then. So what changed now?

@carols10cents
Copy link
Member

I think ring/cookie/etc is a red herring. I think we're hitting rust-lang/cargo#5702.

@sgrif
Copy link
Contributor Author

sgrif commented Jan 27, 2019

So what changed now?

All versions of Ring except 0.13.5 and 0.14.x were yanked.

@carols10cents
Copy link
Member

That still is a cargo bug. We had a lockfile.

@sgrif
Copy link
Contributor Author

sgrif commented Jan 28, 2019

Yes, I agree. But right now cargo will refuse to resolve if we touch Cargo.toml in any way. As I said in the PR description:

Right now, this is blocking anything that requires updating an existing dependency, or adding a new dependency. So this is a quick fix to get our builds working again

@carols10cents
Copy link
Member

A different quick fix would be to only commit those changes to the lockfile that we want, and checkout the changes having to do with ring/cookie/etc. Can you elaborate on why adding a different patch is better than that strategy?

@sgrif
Copy link
Contributor Author

sgrif commented Jan 28, 2019

This fix doesn't require people to edit the lockfile by hand in order to add a dependency. It puts us back in a state that we can develop as normal.

@sgrif
Copy link
Contributor Author

sgrif commented Jan 28, 2019

Actually this PR will need to go a tad further, since we can't use patch to force it to resolve this version of cookie either :\

@sgrif
Copy link
Contributor Author

sgrif commented Jan 28, 2019

I'm going to look into just updating to the latest version of cookie. I'll re-open either this with more fixes or that PR shortly.

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.

2 participants