Skip to content

Conversation

@jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented May 12, 2021

Previously, we added a dependency for all targets in a package to the package's library target. This is correct for most targets, except build scripts, which run before the library crate is built. This PR removes the incorrect dependency on the library target.

We also used to treat all dependencies the same, which led to build scripts being able to use regular dependencies as well as dev-dependencies. This is also fixed by this PR, and build scripts only depend on build-dependencies.

Incorrect dependency graph:

screenshot-2021-05-11-23:35:01

Graph after removing incorrect dependency on the library target:

screenshot-2021-05-12-14:29:31

Graph after removing non-build-dependencies (final state after this PR):

screenshot-2021-05-12-16:53:00

@jonas-schievink
Copy link
Contributor Author

Seems to be working fine after some local testing.

bors r+

@bors
Copy link
Contributor

bors bot commented May 12, 2021

@bors bors bot merged commit a5b5582 into rust-lang:master May 12, 2021
@jonas-schievink jonas-schievink deleted the track-dep-kinds branch May 12, 2021 14:52
@lnicola
Copy link
Member

lnicola commented May 24, 2021

I'm not sure why, but this increased the number of unknown types in ripgrep:

image

@jonas-schievink
Copy link
Contributor Author

hmm, its build script imports clap::Shell, we seem to be missing the clap dependency here. I'll take a look.

bors bot added a commit that referenced this pull request May 24, 2021
8970: fix: duplicate dependencies that have multiple DepKinds r=jonas-schievink a=jonas-schievink

Cargo collapses identical dependencies that are listed under `[dependencies]` and `[build-dependencies]` into a single `NodeDep`. We have to undo that by duplicating the dependency for each of its listed `DepKind`s.

Not doing that would incorrectly treat a dependency as `DepKind::Normal`, even though it is *also* meant to be a `DepKind::Build`.

Fixes #8812 (comment)

bors r+

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
@lnicola
Copy link
Member

lnicola commented May 25, 2021

Not to complain, but there still are 4 more unknown types (64 vs. 60). I'm not sure of a way to enumerate them, though.

@jonas-schievink
Copy link
Contributor Author

webrender's numbers also increased slightly when the chalk unification work happened, so I'm assuming it's caused by that

@lnicola
Copy link
Member

lnicola commented May 25, 2021

Yeah, good point.

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