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

fix(subplanners): Match aliased dependencies by name and semver #346

Merged
merged 2 commits into from
Jan 19, 2021
Merged

fix(subplanners): Match aliased dependencies by name and semver #346

merged 2 commits into from
Jan 19, 2021

Conversation

Arm1stice
Copy link
Contributor

This PR adds an additional check to ensure that a dependency with an alias is matched by both name and it's version compared to the required semver in the dependency metadata. This allows a situation in which a crate has a dependency listed multiple times with different versions, some with aliases and others without.

Fixes #345

@google-cla
Copy link

google-cla bot commented Jan 15, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@Arm1stice
Copy link
Contributor Author

@googlebot I signed it!

Must have triggered since I'm not currently an employee 😅

@UebelAndre
Copy link
Collaborator

Hey! Glad to see you're around and still making contributions. Do you think you can find a smaller crate that demonstrates this behavior? I was the author of these template files and before I started on #300 I was only dealing with small sets of metadata until I ran into test_plan_build_produces_aliased_dependencies. Would you also be willing to also update that test to use a smaller example and maybe add some comments kinda explaining what's going on in that test?

@Arm1stice
Copy link
Contributor Author

Hey! Glad to see you're around and still making contributions. Do you think you can find a smaller crate that demonstrates this behavior? I was the author of these template files and before I started on #300 I was only dealing with small sets of metadata until I ran into test_plan_build_produces_aliased_dependencies. Would you also be willing to also update that test to use a smaller example and maybe add some comments kinda explaining what's going on in that test?

I can definitely try to do this! The biggest issue is that the crate I'm using in the test is the only one that I know of that has this condition. Unless I upload a custom crate to the registry that recreates it. If I do that I can certainly reduce the size of the test.

@Arm1stice
Copy link
Contributor Author

@UebelAndre Tests should be fixed now, let me know if you think there is more room for improvement

@UebelAndre
Copy link
Collaborator

@UebelAndre Tests should be fixed now, let me know if you think there is more room for improvement

This looks MUCH better, thank you so much!!!

@GregBowyer started working on a way to fix various aliasing bugs in #282 so they might be interested in this as well. Other than that, @acmcarther will have to also review this before it's merged.

But again, thank you! Very excited to finally understand what that test does and I think cargo-raze-alias-test is pretty awesome 😄

Copy link
Member

@acmcarther acmcarther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@acmcarther acmcarther merged commit 0cb6178 into google:master Jan 19, 2021
@UebelAndre UebelAndre mentioned this pull request Jan 20, 2021
4 tasks
@UebelAndre
Copy link
Collaborator

@Arm1stice can you link the source code to cargo-raze-alias-test here? I think we should add it to this repo so we can expand upon it for various aliasing examples.

@UebelAndre
Copy link
Collaborator

re #346 (comment)
@Arm1stice Hey, just pinging this again 😅

@Arm1stice
Copy link
Contributor Author

re #346 (comment)
@Arm1stice Hey, just pinging this again 😅

Sorry, missed this. I have published at https://github.com/Arm1stice/cargo-raze-alias-test

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.

Bug: BUILD file for serenity=0.10.2 is incorrectly generated
3 participants