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

Raise the min version requirement because of the ? operator #412

Closed
wants to merge 2 commits into from

Conversation

dns2utf8
Copy link
Contributor

@dns2utf8 dns2utf8 commented Sep 6, 2017

I fixed the tests for the latest nightly compiler in #411 but since the ? operator is in the main code the minimal required version must be raised.

TODO

  • Update README
  • Update CHANGELOG

@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Sep 6, 2017

I updated the readme to 1.13

@nikomatsakis
Copy link
Member

Yeah, so I should probably not have merged that PR that used ? -- however, I do think it may be worth raising the minimum requirement so that we can use pub(crate), in which case we could also use ?.

(I know that @cuviper and I have differing points of view on this matter, however, as I tend to view the minimum version requirement as more of a courtesy than a hard semver requirement, so I'd like to get his take.)

@cuviper
Copy link
Member

cuviper commented Sep 9, 2017

I should probably not have merged that PR that used ?

Which one was that?

AFAICS the only problem here is actually in futures, where 0.1.15 uses ? and 0.1.14 didn't. If I manually run cargo update -p futures --precise 0.1.14, rust-1.12 can build rayon again. Maybe we can just do that for CI, and eventually we're going to split rayon-futures anyway.

But this highlights my real gripe, that such updated requirements are viral, and there's no tooling to deal with it. Any crate that tries to maintain compatibility is at the mercy of its dependencies to do the same. I'll be less grumpy about it when cargo learns how to ignore too-new crates, e.g. RFC 1953.

I do think it may be worth raising the minimum requirement so that we can use pub(crate)

That would be rust-1.18, which is a big jump to me. And IMO pub(crate) is a relatively minor benefit, that doesn't really enable us to do anything new. Some of our hokey "free pub fn in private module" constructors could be pub(crate) methods instead, but this is just style, not functionality. Do you have a more compelling use case?

@dns2utf8
Copy link
Contributor Author

dns2utf8 commented Sep 9, 2017

You are correct. I grepped the rayon code and it contains no ? operator.
I set the crate requirement to =0.1.14 for testing now.

Imho the futures crate has about the same age as 1.12 and does not guarantee any compatibility with older compilers. Sure one could ask @alexcrichton to yank 0.1.15 and release it as 0.2.0 again.
Moving forward I assume the futures crate will use more features in upcoming releases.
So raising the min version will be required at some point.

From my point of view it boils down to the questions

  • who relies on 1.12?
  • or more general what versions are currently used?

For my projects, I check the debian repo where the current stable release is 1.14.0

@alexcrichton
Copy link

I would highly discourage dependencies like =1.1.14 as it tends to be pretty hostile to downstream dependencies (foces everyone to use an older version), and I don't think I'll be yanking 1.1.15 because Rust with ? is pretty old at this point

@cuviper
Copy link
Member

cuviper commented Sep 9, 2017

We don't really know what compiler folks may be stuck with, and that's part of the problem. The 2017 survey showed most respondents with the current version through rustup, but there were still a tail of people behind, even "1.10 or older".

I wouldn't suggest that futures should yank 0.1.15. I just wish that their compatibility choice wasn't so viral. And I also agree that pinning an older version in rayon has its own problems. I suggest just stepping down the version in the CI scripts for now.

@cuviper cuviper mentioned this pull request Sep 11, 2017
@cuviper
Copy link
Member

cuviper commented Sep 16, 2017

#436 separated rayon-futures, so now we don't need to bump the min version for rayon.

@cuviper cuviper closed this Sep 16, 2017
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.

4 participants