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

inline the code from unreachable and void #13

Merged
merged 3 commits into from
Aug 15, 2018
Merged

inline the code from unreachable and void #13

merged 3 commits into from
Aug 15, 2018

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Aug 15, 2018

This bumps the minimal acceptable versions in Cargo.toml to versions that build on modern rust. this gets thread_local working with Cargos -Z minimal-versions. This is part of the process of seeing how hard this is for crates to use in preparation for getting it stabilized for use in CI, specifically upstreaming the changes required to get criterion working with it. It is easy to use if all of your dependencies support it, but much harder if trying to impose it on them.

I would have started with a pr to unreachable but it seems not to be merging pull requests.

If it is compatible with your minimum rustc version policy an alternative is to drop the requirement on unreachable and use std::hint::unreachable_unchecked insted.

@BurntSushi
Copy link
Contributor

regex depends on this crate, which tries to be a bit more conservative with its minimal Rust version, so I'm hoping we can stay conservative here.

@Amanieu What do you think about just dropping the unreachable crate altogether and inlining what you need from there? Sounds like something we should do if it isn't being maintained. If you're amenable to that, I'd be happy to submit a PR for it.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 15, 2018

How did my pr break the 1.9.0 CI job? It should be using the latest version w/out this PR?

@BurntSushi
Copy link
Contributor

@Eh2406 It wasn't your PR. The lazy-static crate hasn't been that great about maintaining their MSRV. It's technically 1.21 now, but it wasn't always.

@Amanieu
Copy link
Owner

Amanieu commented Aug 15, 2018

The minimum rust version for this crate is basically dictated by regex, which is it's biggest user.

@BurntSushi I'm fine with just inlining the unreachable functionality since it is very little code.

@Eh2406 Eh2406 changed the title void 1.0.1 does not build on modern rust inline the code from unreachable and void Aug 15, 2018
@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 15, 2018

I inlined the code from unreachable and void, what do you think?

@BurntSushi
Copy link
Contributor

BurntSushi commented Aug 15, 2018

@Eh2406 LGTM. Thank you! Do you want to just update the CI config to be 1.21 to match lazy static so we can get a green build?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 15, 2018

Network error, CI why do you hate me?

@Eh2406 Eh2406 closed this Aug 15, 2018
@Eh2406 Eh2406 reopened this Aug 15, 2018
@Amanieu
Copy link
Owner

Amanieu commented Aug 15, 2018

You can try git commit --amend --no-edit && git push --force which should re-trigger CI.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 15, 2018

I closed and reopened. We are green!

@Amanieu Amanieu merged commit c1c1ac3 into Amanieu:master Aug 15, 2018
@Amanieu
Copy link
Owner

Amanieu commented Aug 15, 2018

Great work! Thanks!

@Amanieu
Copy link
Owner

Amanieu commented Aug 15, 2018

Published in 0.3.6.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 16, 2018

@BurntSushi While I have your attention, some of your crates are on my queue for this kind of thing, and I was wondering your opinion of minimal-versions?

Specifically, would you be interested in:

  • PR that bump your direct dependencies to work with it? In general I have bean assuming yes from maintainers, but it can meen that I have to work from the bottom up.
  • PR that add synthetic dependencies on things in your dependency tree to work with it? In general I have been assuming no unless they want the third level as well. But if yes, it can save me a lot of work.
  • PR that add it to CI to test that it works?

Your thoughts?

@BurntSushi
Copy link
Contributor

BurntSushi commented Aug 16, 2018

@Eh2406 I'm generally in favor of having a Cargo.toml that doesn't lie, and I have haphazardly attempted to maintain that by hand, but there's no way I've gotten it correct 100% of the time. So in that sense, yes, I would appreciate supporting minimal-versions. I haven't actually used Cargo's support for it yet, so I don't really know how it feels, but I expect I'd get around to it at some point or another anyway. But happy to move forward with it any time. And if we did, yes, it should be added to CI.

I'm not sure how I feel about adding synthetic dependencies though. Doesn't that mean I'm using a dependency that isn't maintained? If so, I'd rather fix that than work-around it. I care a lot about maintaining high standards for which dependencies I bring in, so if there is an unmaintained dependency somewhere, I'm probably willing to do the work to fix that, whatever it takes. Although, I suppose an alternative explanation is that it might be a dependency that is maintained but doesn't want to support minimal-versions, which might be reasonable. I'm not sure what to do in that situation.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 16, 2018

That is entirely reasonable! A major part of this experiment is to find out how Cargo's support feels, and whether it needs to be changed before The Cargo Team recommends it be used widely.

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