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

use cargo_util_schemas::{PackageName, FeatureName} #260

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

aatifsyed
Copy link

Testing the waters for how you'd feel about this change

cargo_metadata has validated strings for these usecases.

  • this would be a breaking change with a new public dependency

Copy link
Contributor

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

cargo_metadata basically consumes the output JSON from cargo metadata, which should be validated by cargo itself already. I am not sure if I see the value of extra validations in this crate. Could you elaborate why this is needed?

@oli-obk
Copy link
Owner

oli-obk commented Apr 10, 2024

I don't care about the validation, but sharing data types with cargo may be a useful way forward to slowly making cargo_metadata mostly a set of convenience functionality

@aatifsyed
Copy link
Author

CI is failing because

[package]
name = "cargo-util-schemas"
version = "0.2.0"
rust-version = "1.75.0"  # MSRV:1

https://docs.rs/crate/cargo-util-schemas/0.2.0/source/Cargo.toml.orig

So this would be an MSRV bump as well as an API breaking change

@aatifsyed
Copy link
Author

Just a gentle bump on this - under consideration or should it be closed?

I will say that having HashMap<PackageName, semver::Version> is fantastic UX ootb in my own experiments on this fork :)

@oli-obk
Copy link
Owner

oli-obk commented Apr 26, 2024

That's quite an MSRV bump. What's the earliest rustc version that supports rust-version in Cargo.toml? Maybe we can first bump to that and add an MSRV to cargo_metadata. Then when we bump it again in the future, the cargo version resolution code can at least figure out which versions it can use.

@weihanglo
Copy link
Contributor

What's the earliest rustc version that supports rust-version in Cargo.toml?

1.56, though Cargo's MSRV-aware resolver recognizes it even when the version is older than 1.56.
So maybe use cargo-msrv to find the current MSRV?

@oli-obk
Copy link
Owner

oli-obk commented Apr 26, 2024

Oh neat, thanks!

edit: lol we already use rust-version here.

@oli-obk
Copy link
Owner

oli-obk commented Apr 26, 2024

Oh well, let's bump the rust-version. It's a major version bump, and anyone using an old cargo to compile cargo_metadata to use it on a newer crate is already asking for trouble anyway.

@aatifsyed
Copy link
Author

Interestingly, the "real" MSRV is 1.65:

$ cargo msrv -- cargo check --ignore-rust-version --config='registries.crates-io.protocol="git"'
...
Check for toolchain '1.65.0-x86_64-unknown-linux-gnu' succeeded
   Finished The MSRV is: 1.65.0   

(which is when let ... else was stabilised) - cargo_util_schemas is being overly conservative here.

I'll push a commit that changes CI and rust-version

Copy link
Author

@aatifsyed aatifsyed left a comment

Choose a reason for hiding this comment

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

That leaves two TODOs, introducing PackageName and RegistryName, which I'm not confident about - what do you feel is the best course of action there?

@@ -357,7 +358,7 @@ pub struct Package {
pub id: PackageId,
/// The source of the package, e.g.
/// crates.io or `None` for local projects.
pub source: Option<Source>,
pub source: Option<Source>, // TODO(aatifsyed): should this be RegistryName?
Copy link
Author

Choose a reason for hiding this comment

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

here

@@ -303,7 +304,7 @@ pub struct Node {
pub struct NodeDep {
/// The name of the dependency's library target.
/// If the crate was renamed, it is the new name.
pub name: String,
pub name: String, // TODO(aatifsyed): should this be PackageName?
Copy link
Author

Choose a reason for hiding this comment

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

and here

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.

None yet

3 participants