Rework planning to use resolve tree and not packages (updated) #425
Conversation
There are a range of subtle and irritating bugs that stem from attempting to work with renames from the package set provided by cargo metadata. Fortunately, recent versions of cargo metadata provide a limited version of the resolve nodes, both as the original node ids and as a newer set providing both rename and targeting information. As such we can (hopefully) simplify crate -> dep node resolution, remove a bunch of subtle aliasing bugs. Fixes #241, fixes #269, fixes #270, resolves #144, resolves #187
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
I ran this patch on a large repo, and it seems to produce identical output to #282. |
There was a problem hiding this comment.
LGTM, one comment and associated question. Thanks for following up on this.
The google CLA bot will require both you and @GregBowyer to sign off, since it requires consent from all commit authors. Squashing it down may be a relevant option here.
| @@ -92,7 +95,7 @@ pub struct CrateDependencyContext { | |||
| // build_data_dependencies can only be set when using cargo-raze as a library at the moment. | |||
| pub build_data_dependencies: Vec<BuildableDependency>, | |||
| pub dev_dependencies: Vec<BuildableDependency>, | |||
| pub aliased_dependencies: Vec<DependencyAlias>, | |||
| pub aliased_dependencies: BTreeMap<String, DependencyAlias>, | |||
There was a problem hiding this comment.
Can you comment on the meaning of the string here?
Not necessary, but a struct, holding a string may provide more meaning long term if that's fairly straight-forward.
There was a problem hiding this comment.
I'm happy to work on this a bit more, but I might suggest that it would be easier to commit this change and iterate, or just let you change my patch to your satisfaction in a followup. That way, we don't get delayed by asynchronous conversations. Your comments are reasonable, but the back and forth conversation seems too costly for things that don't impact correctness.
The pattern here is something I would do if I reviewed a coworker's code on a daily basis, and I wanted to make that process smoother going forward. But I think I'll only be sending cargo-raze patches here and there.
As for the CLAs, mine is signed. There was also a warning about this in #282 regarding @GregBowyer's sign-off.
There was a problem hiding this comment.
It was mostly to make sure I understood intent correctly, and I assumed adding a comment would be a pretty quick change on your end.
| pub aliased_dependencies: BTreeMap<String, DependencyAlias>, | |
| /// Aliased dependencies, sorted/keyed by their `target` name in the `DependencyAlias` struct. | |
| pub aliased_dependencies: BTreeMap<String, DependencyAlias>, |
|
@googlebot I consent. |
| @@ -92,7 +95,7 @@ pub struct CrateDependencyContext { | |||
| // build_data_dependencies can only be set when using cargo-raze as a library at the moment. | |||
| pub build_data_dependencies: Vec<BuildableDependency>, | |||
| pub dev_dependencies: Vec<BuildableDependency>, | |||
| pub aliased_dependencies: Vec<DependencyAlias>, | |||
| pub aliased_dependencies: BTreeMap<String, DependencyAlias>, | |||
There was a problem hiding this comment.
It was mostly to make sure I understood intent correctly, and I assumed adding a comment would be a pretty quick change on your end.
| pub aliased_dependencies: BTreeMap<String, DependencyAlias>, | |
| /// Aliased dependencies, sorted/keyed by their `target` name in the `DependencyAlias` struct. | |
| pub aliased_dependencies: BTreeMap<String, DependencyAlias>, |
This fixes `crates.bzl` never having any crates defined in the `_PROC_MACRO_DEPENDENCIES` map. This fixes a regression from #425 (or #282) and regenerates cargo-raze outputs (which should have been done on the merged commit). This also fixes a regression in how BUILD files were rendered, resulting in buildifier defects. I cherry picked a change I had an another branch which is ahead of `main` and contains the fix. This change includes #393 Finally, this fixes another regression where `extra_aliased_targets` was being ignored.
This addresses @dfreese's comments in #282 and preserves @GregBowyer's commit in the history.