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

Fix the build for ruby 3.0.x (but not 3.1.0-dev yet) #262

Merged
merged 4 commits into from Jun 26, 2021

Conversation

rgarner
Copy link
Contributor

@rgarner rgarner commented Jun 22, 2021

Fix:

  1. dev dependency for webrick
  2. keyword arguments are stricter with respect to options hashes (double-splat fixes)
  3. Proc.new requires an empty block where a block is not given

Does not fix:

  • gem as-notifications will need updating as per point 3. above (though I'm not sure we actually call this in such a way that it would use the default Proc.new for the argument, so it's probably just a ticking time bomb)
  • 3.1.0-dev is broken as Time::RFC_* constants aren't there right now

webrick is no longer a bundled dependency for Ruby 3 and up.
@rgarner rgarner changed the title Fix build for ruby 3.0.0 Start fixing the build for ruby 3.0.0 Jun 22, 2021
@bethesque
Copy link
Contributor

🎉 So glad to see someone pick this up!

@rgarner
Copy link
Contributor Author

rgarner commented Jun 22, 2021

@bethesque heh, we tried doing this curiously on a local project and it was kind of a "hey, while this is still fresh in my head". It's still failing on ruby-head – I'm a bit stumped with that one ☹️

@rgarner
Copy link
Contributor Author

rgarner commented Jun 22, 2021

@bethesque I've seen other projects allow failures on ruby-head, I wonder if that's acceptable here

@rgarner
Copy link
Contributor Author

rgarner commented Jun 22, 2021

It looks like our cookie.rb has this in it, and the constants RFC2822_DAY_NAME and RFC2822_MONTH_NAME are, for whatever reason, not in Ruby 3.1.0-dev:

    def rfc2822(time)
      wday = Time::RFC2822_DAY_NAME[time.wday]
      mon = Time::RFC2822_MONTH_NAME[time.mon - 1]
      time.strftime("#{wday}, %d-#{mon}-%Y %H:%M:%S GMT")
    end

Since I don't know the reason for the dropping of these constants (and maybe it's in error, maybe they'll come back?), we can either drop down to the RFC822 equivalents in CGI::Util::RFC822_DAYS and CGI::Util::RFC822_MONTHS or we can make failures in ruby-head not an error condition.

@bethesque
Copy link
Contributor

Hm. That's a strange one.

irb(main):002:0> require 'time'
=> true
irb(main):003:0> Time::RFC2822_DAY_NAME
=> ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"]

Given the nature of a constant is, well, constant, perhaps just create a Webmachine constant with that value? Maybe they've dropped support for RFC2822_DAY_NAME in ruby-head, but haven't documented it yet. I can't find anything in the googles either.

@bethesque
Copy link
Contributor

I'm ok with failing ruby-head personally. We can deal with it when it becomes an official release.

@bethesque
Copy link
Contributor

The ruby-head build is already marked as "experimental" in test.yml. There's just nothing that runs after the tests to show that the failure is ignored.

@rgarner
Copy link
Contributor Author

rgarner commented Jun 22, 2021

@bethesque ah, thanks. I'm quite unfamiliar with the setup of these kinds of matrix builds! I've just cloned the ruby source to try and work out why the constants have gone missing. Current hypothesis is that they're fiddling with the shareable_constant_value pragma in the C and some constants are going missing (there's been a reverted commit around it, I wonder if 3.1.0-dev's current release is prior to the revert). I think this might be too big a yak for me to shave today, so I'll just open this for review.

@rgarner rgarner marked this pull request as ready for review June 22, 2021 08:10
@bernd
Copy link
Contributor

bernd commented Jun 22, 2021

@rgarner @bethesque I created a PR in as-notifications to make it compatible with Ruby 3.0. bernd/as-notifications#3

It would be awesome if one of you could take a quick look. My Ruby got quite rusty. 😅 The tests work, so I guess it's fine. I can push a new as-notifications gem release once it has been merged.

@rgarner
Copy link
Contributor Author

rgarner commented Jun 22, 2021

@bernd have commented over there. Also, hi, and thanks 😁

@rgarner rgarner changed the title Start fixing the build for ruby 3.0.0 Fix the build for ruby 3.0.x (but not 3.1.0-dev yet) Jun 22, 2021
@seancribbs
Copy link
Member

You all are the best. ❤️ 💜 💙 💚 💛

@bernd
Copy link
Contributor

bernd commented Jun 22, 2021

@rgarner I pushed as-notifications 1.0.2 to rubygems.

@rgarner
Copy link
Contributor Author

rgarner commented Jun 24, 2021

@bethesque hugely grateful for the thoughtful review*

*spotting my terrible mistakes

Ruby 3 is stricter about hashes being used interchangeably with
keyword arguments, and will give errors like:

ArgumentError: wrong number of arguments (given 2, expected 0..1)

Explicitly map options hash to positional argument with ** to fix
Ruby 3 no longer supports Proc.new without a block to mean "the block
that was passed to this method". Use an explicit &block instead
This commit does not fix as-notifications, on this line:

https://github.com/bernd/as-notifications/blob/v1.0.0/lib/as/notifications/fanout.rb#L18
@bethesque
Copy link
Contributor

Is this good to merge now from your perspective @rgarner? It looks fine to me.

webmachine.gemspec Outdated Show resolved Hide resolved
Since 1.0.2 is now compatible with Ruby 3.0, we have one spec
we no longer have to use the Proc.new { } form with.

Thanks Bernd!
@bethesque bethesque merged commit a17691c into master Jun 26, 2021
@bethesque bethesque deleted the fix-build-for-ruby-3.0.0 branch June 26, 2021 23:34
@bethesque
Copy link
Contributor

Thanks @rgarner!

@bethesque
Copy link
Contributor

I'll put out a release when I get a moment. Not sure if adding ruby 3.0 support is a "bug fix" (patch) or a "feature" (minor) 🤔.

@rgarner
Copy link
Contributor Author

rgarner commented Jul 1, 2021

@bethesque It's tricky, for sure. I'd err on the side of "minor". My reasoning is that prior to Ruby 3.0's release, there was no "bug" (and there still isn't so long as you don't upgrade Ruby), and so adding support is a sort of "feature". I think if I were wondering why something at version 1.0 didn't support Ruby 3.0 and I bundle updated I'd have more reassurance that 1.1 might have added new Ruby support than if I saw my Gemfile.lock go to 1.0.1. For that reason I think minor sends the right message (the semver guidelines in respect of our particular case here are perhaps ambivalent depending on your reading of them).

@Asmod4n
Copy link
Member

Asmod4n commented Jul 2, 2021

Hm, that’s a tricky one.
I would follow the road of least surprise and treat it like a Bugfix. Shipping something that depends on a interpreter which might get updated to another major release at any time should still work after the major Update of the interpreter.

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.

None yet

5 participants