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

Converting Target kind and crate_types to enum representation #258

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

michaelciraci
Copy link
Contributor

Converting Target kind and crate_types to enum representation, along with an Unknown(String) as a fallback.

This is in response to the following issue: #239

src/lib.rs Show resolved Hide resolved
@oli-obk
Copy link
Owner

oli-obk commented Jan 20, 2024

Thanks for working on this.

Please update the changelog, too

@michaelciraci
Copy link
Contributor Author

I was looking into why it failed for nightly, and it looks like the main branch is also failing nightly. I have the fix. Do you want me to add it here to get this nightly pipeline working again, or open a new merge request?

@oli-obk
Copy link
Owner

oli-obk commented Jan 20, 2024

Either is fine with me. Whatever works for you

@michaelciraci
Copy link
Contributor Author

It looks like the latest version of cargo-platform (v0.1.6), has a msrv of 1.70. However v0.1.5 works with 1.56. I put an upper limit on cargo-platform to keep the msrv of 1.56.

@jplatte
Copy link

jplatte commented Jan 21, 2024

I put an upper limit on cargo-platform to keep the msrv of 1.56.

That is generally a very bad idea, as it leads to un-resolvable dependency trees when another dependency in the tree requires cargo-platform = "0.1.6". Instead, I would recommend updating the MSRV CI job to run cargo update -p cargo-platform --precise 0.1.5 before checking compilation. There are also other solutions that don't require touching the CI job (or Cargo.toml) whenever a dependency bumps its MSRV in a patch release, but they're more involved; maybe the maintainers have a preference towards one such solution, I'm just a contributor.

@michaelciraci
Copy link
Contributor Author

Yeah, there are multiple ways to deal with this and I'll defer to the maintainers on this.

The pull request was really for something else, so I can revert the cargo-platform change and that can be dealt with in a different pull request as well.

@oli-obk
Copy link
Owner

oli-obk commented Jan 22, 2024

The pull request was really for something else, so I can revert the cargo-platform change and that can be dealt with in a different pull request as well.

yes please remove that commit

@oli-obk
Copy link
Owner

oli-obk commented Jan 22, 2024

Please remove the commits, instead of using a new commit to revert the previous one

@michaelciraci
Copy link
Contributor Author

Should be good now :)

@oli-obk oli-obk merged commit 6d527db into oli-obk:main Jan 22, 2024
10 of 12 checks passed
@oli-obk
Copy link
Owner

oli-obk commented Jan 22, 2024

Thanks!

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.

3 participants