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 the Cargo.toml version of the check-cfg squelching #577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

glandium
Copy link
Contributor

@alexcrichton
Copy link
Owner

Thanks! Mind adding the MSRV you're interested in to .github/actions/main.yml as well?

@glandium
Copy link
Contributor Author

My own MSRV is probably much more recent than what other people might be relying on, but 1.80 is really too recent. It would be great to figure out the real practical MSRV and decide what the real supported version should be.

@alexcrichton
Copy link
Owner

I've got no preference myself, I mostly figure that if you don't want this to regress again it'd be good to test in CI. No guarantee has been given up to now, but now's better late than never!

Basically I think this should be tested in CI. As to exactly what version is tested, iunno what was the practical MSRV before so I'm fine if you just want to fill in what you're interested in. If someone else is so interested they can send a PR to reduce it further.

@glandium
Copy link
Contributor Author

This is actually more tricky than in looks: only versions of cargo >= 1.73.0 and < 1.80.0 complain about rustc-check-cfg output from build.rs. Versions older than cargo 1.73.0 still build curl-sys just fine... I'm not super convinced it's worth testing a version between 1.73.0 and 1.80 just for check-cfg now it's there in a way supported by these versions.

@glandium
Copy link
Contributor Author

The practical MSRV looks like it is 1.63.0, FWIW (from socket2 and cc).

@glandium
Copy link
Contributor Author

Could you merge this and do a release?

@alexcrichton
Copy link
Owner

Can you add a CI entry for the MSRV you're interested in?

@glandium
Copy link
Contributor Author

See #577 (comment)

@alexcrichton
Copy link
Owner

I still don't really understand the consequences of that comment? Why would that mean that more tests can't be added to CI? Is the crate as-is broken on only that range of rustc versions? Does your desired MSRV not fall in that range?

@glandium
Copy link
Contributor Author

Is the crate as-is broken on only that range of rustc versions?

Yes.

Does your desired MSRV not fall in that range?

Yes, but that's kind of incidental. And once this is fixed, there isn't really a reason it would break (except if the MSRV bump abruptly, but that would be better served with a test with 1.63, which doesn't anything for this issue (it works).

@alexcrichton
Copy link
Owner

Can you add testing for your MSRV then? At the very least something needs to be added here if you'd like a guarantee it doesn't regress. If other older verisons are broken then others can send PRs to fix those if they're interested.

@glandium
Copy link
Contributor Author

How would you expect it to regress? Someone explicitly reverting the change?

@alexcrichton
Copy link
Owner

I think it's a good idea to have CI regardless of if it's likely to break or not. It serves as a good documentation reminder for future contributors and additionally doubles-up about being extra-sure that mistakes aren't made.

@alexcrichton
Copy link
Owner

I've merged and published #581 and I've double-checked it works for 1.63. If you're interested I think it'd be reasonable to add to CI here (perhaps as a second entry since as you were saying it behaves differently than 73-80).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants