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

Make the gem a noop on Rubies older than 2.6 #9

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

casperisfine
Copy link

Ref: mikel/mail#1439

Some gems depend on io-wait, but still support older rubies,
so they have to chose between droping support or not listing io-wait.

But io-wait could act a a noop on older rubies.

cc @eregon @hsbt @nobu

@simi
Copy link

simi commented Jan 13, 2022

Ruby head fails now on Windows :(. Is that blocking merge?

IMHO running head on PR doesn't make any sense really. It would make sense to move it to daily scheduled run.

@casperisfine
Copy link
Author

IMHO running head on PR doesn't make any sense really.

It does. For instance if a PR start using something that is no longer supported on 3.2, e.g. File.exists?.

The issue is more that GitHub Actions still doesn't really have a concept of allowed failures.

@simi
Copy link

simi commented Jan 13, 2022

🤔 @casperisfine the problem is the same code can pass once and fail another time. Anyway that's really off-topic here.

@eregon
Copy link
Member

eregon commented Jan 13, 2022

head and 3.1 need ucrt support in setup-ruby, which is not merged yet (ruby/setup-ruby#224), maybe that could be the cause. I would think it's more likely to be a transient issue with the test, but hard to know from just the output.
It'd be useful to use fail-fast: false in all workflows so we see other versions results even if one fails.

@eregon eregon requested a review from nobu January 13, 2022 10:49
@MSP-Greg
Copy link

Using the code @eregon mentioned, and very simple changes to the Actions workflow, all three OS's using Ruby 2.6 thru head pass. See:

https://github.com/MSP-Greg/io-wait/runs/4806126260

@dentarg
Copy link

dentarg commented Jan 30, 2022

head and 3.1 need ucrt support in setup-ruby, which is not merged yet (ruby/setup-ruby#224)

That has been merged and released now

Ref: mikel/mail#1439

Some gems depend on io-wait, but still support older rubies,
so they have to chose between droping support or not listing io-wait.

But io-wait could act a a noop on older rubies.
@casperisfine
Copy link
Author

I rebased my PR, hopefully the CI should be passing now.

@nobu nobu merged commit 75fcb74 into ruby:master Feb 1, 2022
@nobu
Copy link
Member

nobu commented Feb 1, 2022

Frankly, I don't want to support/encourage use of ended versions...

@casperisfine
Copy link
Author

That's totally understandable, unfortunately there's still a huge ecosystem of people using ruby scripts with the aws-sdk gem, or even chef on Ubuntu 16.04 which is supported until 2026 and still had ruby 2.3 as system ruby, or ubuntu 18.04 which ash ruby 2.5.

So I can understand why some gem owners might want to keep compatibility.

@simi
Copy link

simi commented Feb 1, 2022

@casperisfine but that is problem of Ubuntu / chef maintainers. Ruby (and RubyGems) can't maintain everything just because other projects decided to make changes breaking movement forward. I do have experience with chef cookbooks. We gave up and moved to different technology because of this. It was impossible to migrate to newer Ruby without rewriting whole project from scratch.

@casperisfine
Copy link
Author

Ruby (and RubyGems) can't maintain everything

Nobody's asking Ruby to maintain anything, just to reasonably accommodate then when it doesn't cost anything. I submitted the change and it cost close to 0 to maintain. If that patch was remotely complex I wouldn't have submitted it.

I know it sucks, but 2.3, 2.4 and 2.5 combined are over 75% of rubygems downloads: https://ecosystem.rubytogether.org/ (most of it being aws related stuff https://ui.honeycomb.io/ruby-together/datasets/rubygems.org/result/Dtq7G7SviU), so it's not surprising that some gems useful in scripting such as mail may want to keep support for older rubies. I see no reason to deliberately prevent them to do so.

@simi
Copy link

simi commented Feb 1, 2022

@casperisfine https://ecosystem.rubytogether.org/ is wrong, we have found out that recently on RG slack. It would be better to not rely on that. Thanks for the honeycomb link, I'll check that out.

so it's not surprising that some gems useful in scripting such as mail may want to keep support for older rubies. I see no reason to deliberately prevent them to do so.

There is no need to support those rubies in main branch. If there are so many usages, it should be easy to find a maintainer for unsupported Rubies in some legacy branch.

There should be no penalty for users upgrading apps to newer Rubies because of users using EOL Rubies. This has happened in this case (started at mail gem, but also affected few others). That's my point.

@eregon
Copy link
Member

eregon commented Feb 1, 2022

This actually improves the experience for Ruby 3.1+ users, as they don't/won't need this hack anymore:
mikel/mail#1439 (comment)
So I think it's a very clear gain here, it removes a hack that Rails & anything else transitively depending on net-* gems needs to accommodate 3.1, and of course it's unreasonable to ask every gem depending on net-* to immediately drop support for Ruby < 2.6 or not support 3.1, if they want to support both it's their choice, and stdlib (or ex-stdlib) gems should not prevent this.

@simi
Copy link

simi commented Feb 1, 2022

I think you're missing my point. mikel/mail#1439 is not even merged now, months after initial PR and cca 35 days after 3.1 release. And if I understand it well, the reason for this is, it will break EOL Rubies. That's it. 3.1+ users need to use "hack" since EOL Ruby compatibility was preferred. That doesn't make any sense to me.

@eregon
Copy link
Member

eregon commented Feb 1, 2022

@simi Now that this is merged, it should now be possible to merge mikel/mail#1439 without also needing to drop support for many Ruby versions.
Yes, this is all done relatively late, I think whenever CRuby removes a default gem or bundled gem we should be careful to always support old Ruby versions (which had those in stdlib) through letting the gem install and e.g. noop like here, cc @hsbt. Otherwise, there is no way to depend on the new extracted gem and still work for older Ruby versions.

@simi
Copy link

simi commented Feb 1, 2022

I think whenever CRuby removes a default gem or bundled gem we should be careful to always support old Ruby versions (which had those in stdlib).

🤔 IMHO That's the whole point of EOL Rubies - to not maintain them anymore. EOL Rubies should not block any changes for supported Rubies. There is no reason to check if any change on Ruby master makes anything on EOL Ruby incompatible.

@eregon
Copy link
Member

eregon commented Feb 1, 2022

It's not reasonable for anyone, including ruby-core, to force an existing gem to choose between dropping support for many Ruby versions (which the gem chooses to support) or support the latest Ruby version, that's just bad. Both should be possible, and both is not hard.
This is also overall a very small change to the gem, so a one-time cost and I think discussing it further is counter-productive.

@simi
Copy link

simi commented Feb 1, 2022

This is also overall a very small change to the gem, so a one-time cost and I think discussing it further is counter-productive.

In theory it is small change, but it blocked mikel/mail#1439 for long time actually. That's the only problem I do see here. If we will be able to find out those problems on time (which IMHO happened in mail gem) and we will be able to provide solution within reasonable time, there is no reason to not do it. But in this case it didn't happened and caused just troubles for Ruby 3.1+ users.

I think we both understand each other. We can end up the discussion in here. Thanks for your point of view @eregon. Btw. I'm available at RubyGems/Bundler Slack for further discussions.

@eregon
Copy link
Member

eregon commented Feb 1, 2022

Not sure why I didn't think of this before, but I wonder why net-smtp, net-imap and net-pop through net-protocol depend on io-wait: https://github.com/ruby/net-protocol/blob/020d825d50fca2d3f559ce4806b134f065f89782/net-protocol.gemspec#L35
io-wait was stdlib and is now a default gem, so I think it shouldn't need to be an explicit dependency, and I think that would avoid the issue too.

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

Successfully merging this pull request may close these issues.

8 participants