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

Handle parsing a PackageId that is a raw registry url into a Crate #758

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Skgland
Copy link
Contributor

@Skgland Skgland commented Dec 18, 2024

I noticed that a lot of errors that I would expect to be categorized as DependsOn(_) would end up as Unknown instead.

It looks like cargo sets the package_id field in the json output to registry+https://github.com/rust-lang/crates.io-index#{crate_name}@{crate_version} which wan't handled by Crate::try_from.
As a result no crate would be inserted into the deps set and it wasn't able to differentiate between an empty deps set due to no errors in dependencies and an empty deps set because the package_ids field of failing deps couldn't be parsed into a Crate.

@Skgland Skgland force-pushed the raw-registry-urls branch 2 times, most recently from b57c8b0 to b4bfa7b Compare December 18, 2024 17:59
@Skgland Skgland changed the title Handle parsing PackageIds that are raw registry urls into Crate Handle parsing a PackageId that is a raw registry url into a Crate Dec 18, 2024
@Skgland
Copy link
Contributor Author

Skgland commented Dec 22, 2024

Looking a bit further it looks like in 1.77/1.78 cargo switched to packeg id spec.

Should I add a todo comment for implementing the parsing of the package id spec completly and cleaning up the old package id parsing? Other than that I would consider this ready for review.

src/runner/test.rs Outdated Show resolved Hide resolved
@Skgland
Copy link
Contributor Author

Skgland commented Jan 10, 2025

I have replaced the manual URL parsing code,
initially by using url::Url,
but based on a suggestion by epage rust-lang/cargo#15048 (comment) I replaced it with PackageIdSpec::parse

@Skgland Skgland marked this pull request as ready for review January 10, 2025 21:02
Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

I'm glad you found a package to parse the IDs :D

Left a small comment. Also, as before, I think we should not set the version to "unknown" if it's missing, but error out instead, as I don't know how the rest of Crater would handle it.

src/crates/mod.rs Outdated Show resolved Hide resolved
@Skgland
Copy link
Contributor Author

Skgland commented Jan 12, 2025

I'm glad you found a package to parse the IDs :D

Left a small comment. Also, as before, I think we should not set the version to "unknown" if it's missing, but error out instead, as I don't know how the rest of Crater would handle it.

I think I missed that remark last time, I will change that then.

@Skgland
Copy link
Contributor Author

Skgland commented Jan 12, 2025

Ok, I have rebased to resolve merge conflicts and addressed your remarks regarding turning missing version into unknown and the crate-name repo-name mixup for GitHubRepo crates.

I think how I handle the sha field for GitRepo and GitHubRepo crates needs to be changed as well.
Currently this just puts the GitReference in there whether that's a commit hash or not,
also even if it is a commit hash it currently gets a rev= prefix added to it.

- github strips `.git` suffixes when attempting to create a new repo ending in `.git` so this should be unambiguouse
- for other repos I hope they also don't allow creating repos with names ending in `.git`
@Skgland
Copy link
Contributor Author

Skgland commented Jan 12, 2025

Tags, Branches and non-hex revs now result in sha being None.

Copy link
Member

@pietroalbini pietroalbini left a comment

Choose a reason for hiding this comment

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

Thanks! ❤️

@pietroalbini pietroalbini added this pull request to the merge queue Jan 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 12, 2025
@Skgland
Copy link
Contributor Author

Skgland commented Jan 12, 2025

minicrater failed, it looks like some classifications changed, the new classification looks to be an expected improvement and this changed result just needs to be blessed, though CIs expected to actual diff is a bit hard to read as the coloring ends on linebreak, so I will need to check this locally.

@Skgland
Copy link
Contributor Author

Skgland commented Jan 12, 2025

Ok, I have now updated the minicrater expectations in the latest commit.

@Skgland Skgland requested a review from pietroalbini January 12, 2025 22:53
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.

2 participants