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

resolve build dependencies independently [WIP] #7761

Conversation

boringcactus
Copy link

motivation

the unification of build dependencies with runtime dependencies creates a lot of counterintuitive behavior (#5237 and #4866 are two areas where this has been a long-standing issue). the most straightforward approach to fixing this, as far as i can tell, is to not unify build dependencies with runtime dependencies.

approach

unfortunately, everything about cargo is built around a monolithic dependency resolution DAG.

there are a lot of ways that this could be implemented, but what i've (tentatively) chosen is to add a field build_graphs: HashMap<PackageId, Resolve> to the existing Resolve, and when a package with at least one build dependency is encountered, the resolution process recurses to generate the build dependency DAG for that package. this approach seems good to me - it preserves existing code's assumption that all dependency resolution information is in the Resolve, and it allows for reuse of most of the existing dependency resolution code - but i'm certainly open to suggestions if there's a better approach.

status

right now, the resolver tests are broken and i don't understand enough about proptest to fix them, and a few dozen integration tests are failing for reasons that i don't quite understand. my intuition is that there's some gap in the translation from the cargo-internal Resolve to the rustc-attached UnitGraph, but i need to do more poking around to be confident of that.

pending actions i could use some help with:

  • confirm that this approach is reasonable
  • fix resolver tests
  • fix integration tests
  • write tests for feature selection

this whole approach probably isn't great, and it
breaks a few dozen tests for reasons i don't understand,
but the code is more complicated than i probably realize
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2020
@boringcactus
Copy link
Author

@ehuss, you mentioned on discord that you were planning to work on a new feature resolver; is there an approach that i could take here to make that work easier? this change is distinct but adjacent, and i don't want to step on your toes.

@boringcactus
Copy link
Author

i don't actually think i have the energy to chase this down.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 14, 2020

First off, I am sorry we did not get back to you sooner! I did not notice this PR come in, I should have. Sorry for the radio silence.

Second this is impressive. Getting anything working in the resolver requires a large amount of dedication. This is an interesting and creative approach to a problem we have been struggling with for a long time.

However, I am having a hard time thinking thru all the implications that this would have. For example, this would mean that we can have a 1.0.0 and a 1.0.1 and a 1.02 ... all in the same project as long as they are each dependencies of different build scripts. That may not be a problem, but it is new behavior that I have not seen asked for. For another example, if we have b 1.0.0 in more than one Resolve does it get built twice? If so how do we make sure that it doesn't stomp on the output of the other.

@boringcactus
Copy link
Author

well, having different bindgen versions for different build scripts is the exact thing that #5237 requires. my intuition is that b 1.0.0 appearing in more than one Resolve doesn't cause it to be built twice because it should only appear once in the UnitGraph, but i am nowhere near confident in that belief. if you the approach i'm taking here is worth looking into, i can take another stab at some of the tests.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 24, 2020

I think that #5237 requires allowing multiple Major versions even tho they share a links flag. I was trying to express that this would also allow multiple Minor versions.

The Team's capacity for major resolver related changes is very small at the moment. (For example the appallingly slow response time I have been giving you on this thread. I am very sorry.) Now that Eric has made his months long project publick in #7820, I think we need to see that through before we figure out what happens next. When the dust settles on that we should keep this radically simple approach in mined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants