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

Bug7346/transitive patches #7452

Merged
merged 3 commits into from
Sep 27, 2019
Merged

Bug7346/transitive patches #7452

merged 3 commits into from
Sep 27, 2019

Conversation

pyrrho
Copy link
Contributor

@pyrrho pyrrho commented Sep 27, 2019

Fixes #7346.

A cursory comparison between current stable and nightly shows that projects with this topology resolve similarly. If there are other behaviors I should test, I'd be happy to expand that section. This is a pretty focused change, though, so I'm not sure what else there is to break.

Sorry about the delay in putting this PR together. Good news is I know more than I did last week.

@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 @Eh2406 (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 Sep 27, 2019
@Eh2406
Copy link
Contributor

Eh2406 commented Sep 27, 2019

This looks like the change I had in mind! Thank you!
@bors r+

There is a lot of context to understand to make this change. What worked well to "know more"? What did not? Where could we document more? What would you like more explanation on?

@bors
Copy link
Contributor

bors commented Sep 27, 2019

📌 Commit f7bfd9d has been approved by Eh2406

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2019
@bors
Copy link
Contributor

bors commented Sep 27, 2019

⌛ Testing commit f7bfd9d with merge fdcc73f...

bors added a commit that referenced this pull request Sep 27, 2019
Bug7346/transitive patches

Fixes #7346.

A cursory comparison between current stable and nightly shows that projects with this topology resolve similarly. If there are other behaviors I should test, I'd be happy to expand that section. This is a pretty focused change, though, so I'm not sure what else there is to break.

Sorry about the delay in putting this PR together. Good news is I know more than I did last week.
@bors
Copy link
Contributor

bors commented Sep 27, 2019

☀️ Test successful - checks-azure
Approved by: Eh2406
Pushing fdcc73f to master...

@bors bors merged commit f7bfd9d into rust-lang:master Sep 27, 2019
@pyrrho
Copy link
Contributor Author

pyrrho commented Oct 3, 2019

Okay so. Sorry about the delay on getting back to you, @Eh2406. It's been hard to tease my thoughts into a form that will (hopefully) be immediately valuable, and things were getting muddy. Rather than write a novel, I'm going to just clean up my outline of impressions, and hand that to you. We can drill down into specific pieces if more detail would be useful.

  • The code is very organic. A bit sprawling in places.
    • Might be unavoidable. Between finding/walking registries, handling the semantics of Cargo.toml, and building the dep graphs? That's a lot of complexity.
    • Separation of concerns seems to be loose. A fair bit of tight coupling between modules seems to be in place.
    • The state of the program after every major call was hard to discern; after core::resolver::activate_deps_loop is called, do we have a linearized set of crates to build, or a dep-graph? Both? How does it affect the mut cx member? (I think I understand it, and know where to look in the code, but it's still hard to keep in my head.)
  • Comments that help guide readers or describe the flow are rare.
    • Most of the code has good reference-style doc comments for the "What" of each bit of code.
    • Some files have really good prosaic overviews (e.g. core/resolver/mod.rs, sources/registry/mod.rs) that get into the big-picture "Why".
    • Fewer have strong descriptions of how the pieces fit together. Examples;
      • struct Manifest has a member links: Option<String> and struct Summary has a member links: Option<InternedString>. I get it now, but I had read a lot of code/infer a few relationships to grok what was going on.
      • PackageRegistry is a cache of known core::source::SourceIds each of which owns boxed core::source::Source trait object, which itself is a type-erased sources::*::<Kind>Source (GitSource, RegistrySource, etc.). I get that now, but bouncing between core::source and sources::registry was real tricky.
  • Tests aren't what I expected.
    • There seem to be very few unit/regression tests per module. I often lean on those to understand micro-level semantics.
    • The testsuite is amazing.
      • Might be my favorite (domain-specific) integration test framework I've added tests to.
      • Super easy to separate concerns. I didn't bother looking into the implementation of Package::new or git::repo; their use was obvious and immediately helpful.
      • Organization of individual tests was... daunting. Documenting what each file in tests/testsuite/ is for (either in the file, or in a table of contents) might be helpful.
  • Using a debugger in the context of testsuite/ was suboptimal
    • Triggering breakpoints in the cargo library as part of the testsuite execution may be impossible?
    • Relatively simple to re-creatae the packages generated by testsuite tests as a local workspace, but that was less expressive than the test suite.
    • LLDB in Rust is far from mature. General improvements would be awesome, but out of scope.

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 3, 2019

This is grate and it all rings true to my experience! Let me give some inline feedback, in the hope it will keep the conversation going.

  • The code is very organic. A bit sprawling in places.
    • Separation of concerns seems to be loose. A fair bit of tight coupling between modules seems to be in place.

So true, not sure what to do. We have discussed working on splitting out subcrates, but it is a lot of work and slow going. Entropy and all.

    • The state of the program after every major call was hard to discern; after core::resolver::activate_deps_loop is called, do we have a linearized set of crates to build, or a dep-graph? Both? How does it affect the mut cx member? (I think I understand it, and know where to look in the code, but it's still hard to keep in my head.)

Hmm... good point. Maybe we can add comments. The hard part is guessing what state reader will want explained. Do you feel up to a PR with those kinds of questions as comments? If so I can work to replace your questions with the answers.

  • Comments that help guide readers or describe the flow are rare.

This rings true, but not sure I know how to proceed without descending into the particular.

    • Fewer have strong descriptions of how the pieces fit together. Examples;
      • struct Manifest has a member links: Option<String> and struct Summary has a member links: Option<InternedString>. I get it now, but I had read a lot of code/infer a few relationships to grok what was going on.

InternedString has not fully spread throughout the code base. For that matter, I don't know that we even have a good description of when it should be used. That can be improved... I can try to write up a guide, so someone can do an review. Also the codebase is "organic", sometimes the duplication is removable. (#7324)

      • PackageRegistry is a cache of known core::source::SourceIds each of which owns boxed core::source::Source trait object, which itself is a type-erased sources::*::<Kind>Source (GitSource, RegistrySource, etc.). I get that now, but bouncing between core::source and sources::registry was real tricky.

This gets me every time too. How to make it better I wunder.

  • Tests aren't what I expected.
    • There seem to be very few unit/regression tests per module. I often lean on those to understand micro-level semantics.

This is mostly historical. As I understand it @alexcrichton personality does not find this kind of open box testing helpful for him but is open to adding it when others find it helpful. (sorry Alex for putting words in your mouth.) I would be open to adding some to the Resolver, making this more testable may help with the "tight coupling".

    • The testsuite is amazing.
      • Might be my favorite (domain-specific) integration test framework I've added tests to.
      • Super easy to separate concerns. I didn't bother looking into the implementation of Package::new or git::repo; their use was obvious and immediately helpful.

Big thanks to all those that made this happen! As I recall @dwijnand did a lot of work on this, but the bonese predate me.

      • Organization of individual tests was... daunting. Documenting what each file in tests/testsuite/ is for (either in the file, or in a table of contents) might be helpful.

Agread. I get the impression we don't spend much time organizing them. As long as a test gets run on CI, we don't pay too much attention to were it is. Part of the problem is that loats of the test have cross cutting interactions that make it hard to categorize.

  • Using a debugger in the context of testsuite/ was suboptimal

I have never gotten a debugger to to anything useful on rust code in general. So it is possible that we are not making it eazy.

Which of these threads of conversation do you want to pursue? How can I help?

@alexcrichton
Copy link
Member

This is mostly historical. As I understand it @alexcrichton personality does not find this kind of open box testing helpful for him but is open to adding it when others find it helpful. (sorry Alex for putting words in your mouth.)

Heh no worries but this sounds right to me! I haven't personally written a ton of these tests but I don't want to stop anyone from writing tests, and I agree that especially in the context of the resolver having unit tests is extremely useful!

@pyrrho pyrrho deleted the bug7346/transitive_patches branch October 14, 2019 18:45
@ehuss ehuss added this to the 1.40.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when patching dependency
6 participants