/cargo

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

Warn about path overrides that won't work #3136

Merged
merged 1 commit into from Oct 6, 2016
+156 −22

Conversation

Reviewers

@larsbergstrom larsbergstrom

Assignees

@brson brson

Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@alexcrichton @rust-highfive @brson @bors @larsbergstrom @sgrif
@alexcrichton
Member

alexcrichton commented Sep 29, 2016

Cargo has a long-standing bug in path overrides where they will cause spurious
rebuilds of crates in the crate graph. This can be very difficult to diagnose
and be incredibly frustrating as well. Unfortunately, though, it's behavior that
fundamentally can't be fixed in Cargo.

The crux of the problem happens when a path replacement changes something
about the list of dependencies of the crate that it's replacing. This alteration
to the list of dependencies cannot be tracked by Cargo as the lock file was
previously emitted. In the best case this ends up causing random recompiles. In
the worst case it cause infinite registry updates that always result in
recompiles.

A solution to this intention, changing the dependency graph of an overridden
dependency, was implemented with the [replace] feature in Cargo awhile back.
With that in mind, this commit implements a warning whenever a bad dependency
replacement is detected. The message here is pretty wordy, but it's intended to
convey that you should switch to using [replace] for a more robust
impelmentation, and it can also give security to anyone using path overrides
that if they get past this warning everything should work as intended.

Closes #2041
@alexcrichton
Warn about path overrides that won't work 
Cargo has a long-standing [bug] in path overrides where they will cause spurious
rebuilds of crates in the crate graph. This can be very difficult to diagnose
and be incredibly frustrating as well. Unfortunately, though, it's behavior that
fundamentally can't be fixed in Cargo.

The crux of the problem happens when a `path` replacement changes something
about the list of dependencies of the crate that it's replacing. This alteration
to the list of dependencies *cannot be tracked by Cargo* as the lock file was
previously emitted. In the best case this ends up causing random recompiles. In
the worst case it cause infinite registry updates that always result in
recompiles.

A solution to this intention, changing the dependency graph of an overridden
dependency, was [implemented] with the `[replace]` feature in Cargo awhile back.
With that in mind, this commit implements a *warning* whenever a bad dependency
replacement is detected. The message here is pretty wordy, but it's intended to
convey that you should switch to using `[replace]` for a more robust
impelmentation, and it can also give security to anyone using `path` overrides
that if they get past this warning everything should work as intended.

[bug]: #2041
[implemented]: http://doc.crates.io/specifying-dependencies.html#overriding-dependencies

Closes #2041
Loading status checks…
fc0e642
@rust-highfive

This comment has been minimized.

Sign in to view

rust-highfive commented Sep 29, 2016

@alexcrichton: no appropriate reviewer found, use r? to override
@alexcrichton

This comment has been minimized.

Sign in to view
Member

alexcrichton commented Sep 29, 2016

r? @brson

@rust-highfive rust-highfive assigned brson Sep 29, 2016

@larsbergstrom

larsbergstrom reviewed Oct 5, 2016
View changes

src/cargo/core/registry.rs
To change the dependency graph via an override it's recommended to use the
`[replace]` feature of Cargo instead of the path override feature. This is
documented online at the url below for more information.

This comment has been minimized.

Sign in to view
Copy link
@larsbergstrom

larsbergstrom Oct 5, 2016

I'm not sure [replace] handles all the secenarios that path overrides do. In particular, Servo and Gecko seem to get hit by some of the version constraints around [replace] that AFAIK do not exist for path.

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Oct 5, 2016

Member

@larsbergstrom if that's the case then that's definitely a bug in [replace], are there open issues for this?

Additionally paths isn't going away, this is just warning about a case where it will never work (e.g. you replace with something that changed dependencies)

This comment has been minimized.

Sign in to view
Copy link
@larsbergstrom

larsbergstrom Oct 5, 2016

Aha, great! That sounds fine, then :-)

We have open issues (e.g., #2649), but I think that's more of a separate discussion than something that should derail this PR!

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Oct 5, 2016

Member

Also feel free to ping me periodically about those kinds of issues if they keep coming up, it's good to always confirm or deny initial assumptions!
@brson

This comment has been minimized.

Sign in to view
Contributor

brson commented Oct 5, 2016

@bors r+
@bors

This comment has been minimized.

Sign in to view
Contributor

bors commented Oct 5, 2016

📌 Commit fc0e642 has been approved by brson
@bors

This comment has been minimized.

Sign in to view
Contributor

bors commented Oct 6, 2016

⌛️ Testing commit fc0e642 with merge a713da1...

bors added a commit that referenced this pull request Oct 6, 2016

@bors
Auto merge of #3136 - alexcrichton:warn-bad-override, r=brson 
Warn about path overrides that won't work

Cargo has a long-standing [bug] in path overrides where they will cause spurious
rebuilds of crates in the crate graph. This can be very difficult to diagnose
and be incredibly frustrating as well. Unfortunately, though, it's behavior that
fundamentally can't be fixed in Cargo.

The crux of the problem happens when a `path` replacement changes something
about the list of dependencies of the crate that it's replacing. This alteration
to the list of dependencies *cannot be tracked by Cargo* as the lock file was
previously emitted. In the best case this ends up causing random recompiles. In
the worst case it cause infinite registry updates that always result in
recompiles.

A solution to this intention, changing the dependency graph of an overridden
dependency, was [implemented] with the `[replace]` feature in Cargo awhile back.
With that in mind, this commit implements a *warning* whenever a bad dependency
replacement is detected. The message here is pretty wordy, but it's intended to
convey that you should switch to using `[replace]` for a more robust
impelmentation, and it can also give security to anyone using `path` overrides
that if they get past this warning everything should work as intended.

[bug]: #2041
[implemented]: http://doc.crates.io/specifying-dependencies.html#overriding-dependencies

Closes #2041
Loading status checks…
a713da1
@bors

This comment has been minimized.

Sign in to view
Contributor

bors commented Oct 6, 2016

☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64
Approved by: brson
Pushing a713da1 to master...

@bors bors merged commit fc0e642 into rust-lang:master Oct 6, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@bors bors referenced this pull request Oct 6, 2016

Merged

upgrade semver #3154

@alexcrichton alexcrichton deleted the alexcrichton:warn-bad-override branch Oct 6, 2016

@sgrif

This comment has been minimized.

Sign in to view
Contributor

sgrif commented on fc0e642 Oct 10, 2016

Is there an equivalent to [replace] that can be used in .cargo/config? The whole reason for using path = is to avoid having to specify the replacement in 20 different places, is it not?

This comment has been minimized.

Sign in to view
Member

alexcrichton replied Oct 10, 2016

@sgrif not currently, no, unfortunately. If there's a workflow to be had here though (one that works), then we can work on developing it though!

(note that it may also be the case that this warning is being emitted in error)

This comment has been minimized.

Sign in to view
Contributor

sgrif replied Oct 10, 2016

diesel-rs/diesel@78080f0 has the change we had to make. Basically we just want to make sure that all of our test suites run against the in-tree version of everything. Using [replace] means we have to juggle it in about a dozen manifest files overall

This comment has been minimized.

Sign in to view
Member

alexcrichton replied Oct 10, 2016

Oh that's actually more correct! The .cargo folder is never intended to be checked into a repo

This comment has been minimized.

Sign in to view
Member

alexcrichton replied Oct 10, 2016

Er, actually, how come path dependencies aren't used there? It's the intention that many crates in one repo all have path dependencies to one another.

This comment has been minimized.

Sign in to view
Contributor

sgrif replied Oct 10, 2016

Because after the 20th time a bug slipped in because I forgot to point to point something at a path dependency, it felt like a repo-wide override made much more sense. The alternative is just a lot of git churn and room for mistakes.

This comment has been minimized.

Sign in to view
Member

alexcrichton replied Oct 10, 2016

If everything is a path dependency always though, would there still be things forgotten?

This comment has been minimized.

Sign in to view
Contributor

sgrif replied Oct 10, 2016

@alexcrichton alexcrichton referenced this pull request Nov 4, 2016

Closed

Spurious rebuilds of paths-overrides #2041

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment