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

Build with feature sets should be deduplicated when using workspace #4048

Closed
ishitatsuyuki opened this issue May 15, 2017 · 9 comments
Closed

Comments

@ishitatsuyuki
Copy link
Contributor

ishitatsuyuki commented May 15, 2017

Let's use the num crate as an example. num is (indirectly) in dependencies with no feature, and (indirectly) in dev-dependencies with some features. What happens is:

  • num is built with no feature if it the crate was reference as in-tree dependency.
  • num is built with features if the crate is directly built/tested.

Thus, there's a rebuild/relink here. That's a huge cost.

I feel Cargo.lock is where it should be, but anyway this doesn't need to be strictly in there.

See also: #3629

@Mark-Simulacrum
Copy link
Member

I'm confused why. Features should never make a crate incompatible... I don't see the huge cost; rebuilding num in your example is expected. We shouldn't be rebuilding a crate that was already built with a greater set of features, though. That is, if crate A is built with a given feature set then rebuilding it with a feature set that is a subset of the first, there is no real need to rebuild.

@ishitatsuyuki
Copy link
Contributor Author

@Mark-Simulacrum try a larger project such as Cargo/RLS... and you will clearly see the rebuild cost. See rust-lang/rust#41639 (comment) for what I'm doing now.

Since we have already resolved the dependencies with the unioned set of required features (in the lock), we should build them all with the same feature set, regardless what type of build we're doing.

@Mark-Simulacrum
Copy link
Member

I guess I'm confused how adding the crate features to Cargo.lock would actually help. I think what you're trying to suggest is that we deduplicate the two builds, and only build the more "featureful" version of the crate (which seems like a good idea!).

@ishitatsuyuki ishitatsuyuki changed the title Cargo.lock should contain the feature set the crate is using Build with feature sets should be deduplicated when using workspace May 15, 2017
@ishitatsuyuki
Copy link
Contributor Author

I thought about Cargo.lock because it was where the dependency resolution is stored, but thanks for pointing out that it's not necessary relevant. I have modified the issue so the intense is clearer.

@alexcrichton
Copy link
Member

Is this the same as #3923?

@ishitatsuyuki
Copy link
Contributor Author

I don't think so. That one is about hashing and rebuild, while this is regarding dependency resolution. I mean, the tree should be identical regardless whether the crate is directly built or treated as dependency, since we have a workspace lockfile for the whole.

@alexcrichton
Copy link
Member

Ah ok, in that case I'm going to close this as this is an intentional design decision that each project gets its own version of the dependency graph wrt feature resolution. If we want to cut down on rebuilds in rust-lang/rust we'll need to make sure that everything has the same set of features.

@ishitatsuyuki
Copy link
Contributor Author

Are you sure? I think unifying dependency resolution is one of the purpose of workspace, and in fact Cargo does honor it at many places.

@alexcrichton
Copy link
Member

Oh sure yeah but this is mostly dependency resolution in the sense of maintaining the same versions, not necessarily the same features. We still provide the guarantee that each view into the crate graph gets its own set of versions when we can.

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

No branches or pull requests

3 participants