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

Add a new input to optionally update RubyGems #271

Merged
merged 5 commits into from
Jan 23, 2022
Merged

Add a new input to optionally update RubyGems #271

merged 5 commits into from
Jan 23, 2022

Conversation

deivid-rodriguez
Copy link
Contributor

Implements #228.

I guess TruffleRuby needs some special handling. Also, I'd like to ask what's the recommended way to try out my changes when working on a PR?

@deivid-rodriguez
Copy link
Contributor Author

I see that CI already exercises everything, awesome!

@eregon
Copy link
Member

eregon commented Jan 21, 2022

Thank you for the PR!

Could you add a single CI job around

testExactBundlerVersion:
name: "Test with an exact Bundler version"
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: ./
with:
ruby-version: 2.6
bundler: 2.2.3
- run: bundle --version | grep -F "Bundler version 2.2.3"
testDependencyOnBundler1:
name: "Test gemfile depending on Bundler 1"
runs-on: ubuntu-latest
env:
BUNDLE_GEMFILE: ${{ github.workspace }}/gemfiles/bundler1.gemfile
steps:
- uses: actions/checkout@v2
- uses: ./
with:
ruby-version: 2.7
bundler: 1
bundler-cache: true
- run: bundle --version | grep -F "Bundler version 1."
testGemfileMatrix:
strategy:
fail-fast: false
matrix:
gemfile: [ rails5, rails6 ]
name: "Test with ${{ matrix.gemfile }} gemfile"
runs-on: ubuntu-latest
env:
BUNDLE_GEMFILE: ${{ github.workspace }}/gemfiles/${{ matrix.gemfile }}.gemfile
steps:
- uses: actions/checkout@v2
- uses: ./
with:
ruby-version: 2.6
bundler-cache: true
- run: bundle exec rails --version
testTruffleRubyNokogiri:
name: "Test installing a Gemfile with nokogiri on TruffleRuby"
runs-on: ubuntu-latest
env:
BUNDLE_GEMFILE: ${{ github.workspace }}/gemfiles/nokogiri.gemfile
steps:
- uses: actions/checkout@v2
- uses: ./
with:
ruby-version: truffleruby-head
bundler-cache: true
- run: bundle list | grep nokogiri
to test updating to latest, so we make sure it works and keeps working?
Maybe also for a specific version.

Since the default remains default then the I think big matrix should still use default.

And could you add a new RubyGems short section section in the README about this new input?

@eregon
Copy link
Member

eregon commented Jan 21, 2022

For TruffleRuby it will just ignore the update, which is probably fine for now, so I think best to treat it like any other Ruby in this action.

@eregon
Copy link
Member

eregon commented Jan 21, 2022

What's your opinion regarding the psych 4 + old Ruby/RubyGems issue? (#264)

I think updating to a given RubyGems version wouldn't work if people have a matrix of both old and new Rubies (it would actually downgrade RubyGems on the newer Rubies).
Maybe a special version psych4 or so would make sense? (feels hacky to me)
Or maybe never downgrade and always skip updating if the installed RubyGems is more recent than the given version (sounds nice to me).
(Updating to latest RubyGems would probably work for all but is adding more moving parts in CI)

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Jan 21, 2022

Thanks, I will make the requested updates 👍.

Regarding the Psych + old RubyGems issue, a custom input value feels too hacky, yeah. I saw you added some custom code sometimes to workaround issues with bundler on different rubies (for example, installing Bundler 1 on JRuby 9.1 because it has issues with Bundler 2). I guess we could similarly automagically update RubyGems to 3.0.3 for example (the same shipped with Ruby 2.6) on older rubies and note that in the "rubygems: default" documentation. Not sure if the issue is spread enough to warrant that kind of handling though. As far as I understand for the issue to happen, the following conditions must be met:

  • Running Ruby 2.4 or 2.5, because older Rubies don't support Psych 4, and newer Rubies come with a recent enough Rubygems.
  • bundler-cache: true (I guess this is common because it's the default?).
  • Psych 4 being in cache, and thus being restored before bundle install is run.

@eregon
Copy link
Member

eregon commented Jan 21, 2022

Yeah, we could e.g. look at the lockfile to see if there is Pysch 4 in there, but that feels hacky.
I think we don't try to address this directly, it's probably not frequent enough to warrant it, and users should probably understand Psych 4 + Ruby 2.4-2.5 is a problematic combination anyway (not just for CI but also for anyone running those locally).

bundler-cache: true (I guess this is common because it's the default?).

It's not default but it's fairly common.

@eregon
Copy link
Member

eregon commented Jan 21, 2022

@deivid-rodriguez What do you think of this:

Or maybe never downgrade and always skip updating if the installed RubyGems is more recent than the given version (sounds nice to me).

I think that seems generally useful, unless there might be a valid reason to downgrade RubyGems lower than the version with Ruby? (I guess not)

@deivid-rodriguez
Copy link
Contributor Author

I think that seems generally useful, unless there might be a valid reason to downgrade RubyGems lower than the version with Ruby? (I guess not)

Oh, we automatically prevent that. If you try to downgrade RubyGems to a version older than the one that comes with each Ruby, it raises an error. Of course, you need to start from a RubyGems version that has this feature, and it's fairly recent.

@deivid-rodriguez
Copy link
Contributor Author

I think downgrading can be useful sometimes, for example, to workaround some regression. But we forbid downgrading past the default version that comes with Ruby because we never test that kind of combination.

@eregon
Copy link
Member

eregon commented Jan 21, 2022

OK, then we should probably check gem --version in this action if the rubygems input is set to a given version as we wouldn't want to fail the workload in such a case.
For example in Puma's CI where the psych 4 issue happens they'd want something like (I imagine):

jobs:
  test:
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-latest, macos-latest]
        ruby: [2.3, 2.4, 2.5, 2.6, 2.7, '3.0', 3.1]
    runs-on: ${{ matrix.os }}
    steps:
    - uses: actions/checkout@v2
    - uses: ruby/setup-ruby@v1
      with:
        ruby-version: ${{ matrix.ruby }}
        rubygems: MINIMUM_RUBYGEMS_VERSION_WHICH_SUPPORTS_PSYCH4
        bundler-cache: true

and for Ruby >= 2.6 it should just no try to update RubyGems as that would error (or downgrade).

BTW, which RubyGems version has Psych 4 support?

@deivid-rodriguez
Copy link
Contributor Author

Oh, you're right. Yeah, that's probably best, if a specific version is given, interpret and document it as a minimum RubyGems version for the matrix, and do nothing if the default one for a particular Ruby is newer.

BTW, which RubyGems version has Psych 4 support?

I think it's 3.0.0.beta2: rubygems/rubygems@b7995ca.

@eregon
Copy link
Member

eregon commented Jan 21, 2022

I think it's 3.0.0.beta2: rubygems/rubygems@b7995ca.

Thanks, so I think we should recommend rubygems: 3.0.0 for the psych issue (probably best to avoid referring to a beta, right?), we could mention that in the README in the RubyGems section.

@deivid-rodriguez
Copy link
Contributor Author

I was thinking of recommending using either 3.0.3 (the version of RubyGems that comes with the next Ruby, 2.6), or 3.0.9 (the latest patch level on that series). I think 3.0.3 should be fine.

When a fixed version is passed, it should be the minimum RubyGems
version for the whole matrix, i.e., RubyGems should not be dowgraded
past the default version of the oldest Ruby in the matrix.
@deivid-rodriguez
Copy link
Contributor Author

I added the requested changes. CI is having some issues on Windows, but it's not this PR's fault.

@deivid-rodriguez
Copy link
Contributor Author

Also added a note recommending rubygems: 3.0.0 for the Psych + RubyGems issue.

@eregon
Copy link
Member

eregon commented Jan 22, 2022

@eregon
Copy link
Member

eregon commented Jan 22, 2022

This is awesome, thank you!

Some things I noticed in https://github.com/ruby/setup-ruby/runs/4905624850?check_suite_focus=true

  • The output of updating RubyGems takes > 1000 lines, I can imagine that to be impractical when looking at the full log. Is there a way to avoid printing the changelog of all versions in between?
  • gem --version gives 3.0.3.1 but then just below it says Default RubyGems version is 3.0.3. Is there a way to have the semver package handle versions with not exactly 3 components?

@MSP-Greg
Copy link
Collaborator

Can you rerun the CI? Not sure what happened, last night something went haywire with MSYS2, and I've never seen it before.

Since I've never seen it before, I didn't have any tests to fail the 'builds', so they replaced the working release assets.

That cascaded into the ruby-loco builds. I fixed things late last night (early this morning), and I'm adding tests now to both.

README.md Outdated Show resolved Hide resolved
@eregon
Copy link
Member

eregon commented Jan 22, 2022

  • gem --version gives 3.0.3.1 but then just below it says Default RubyGems version is 3.0.3. Is there a way to have the semver package handle versions with not exactly 3 components?

It seems the semver package doesn't handle this. Maybe it doesn't matter too much in practice, WDYT?

@eregon
Copy link
Member

eregon commented Jan 22, 2022

@MSP-Greg The rerun is all green indeed, thanks for the reply.

@deivid-rodriguez
Copy link
Contributor Author

Thank you @eregon.

Regarding your first question, we have gem update --system --silent, although we only fixed that recently to not show the changelog of what was installed. Should I use that instead?

Regarding 3.0.3.1 vs 3.0.3, I noticed that too and I'm not 100% sure what's going on there. I can say that 3.0.3.1 is not released to rubygems.org, and in general I don't think we release 4 digit versions to rubygems.org. At least gem update --system does not support 4 digit versions. My best guess is that ruby-core used to ship released versions of RubyGems with some custom patches on top, adding an extra digit to the version every time they did that. I think we can pretend this last digit does not exist.

@eregon
Copy link
Member

eregon commented Jan 23, 2022

I tried --silent locally and it is indeed very silent: there is no output at all.
The rest of the output actually seems useful, notably to show the installer Bundler and RubyGems versions.
Filtering the output manually seems rather messy, so I guess the best is to just keep the verbose output.

@eregon eregon merged commit 4a1b085 into ruby:master Jan 23, 2022
@deivid-rodriguez
Copy link
Contributor Author

Yes, I agree with you, I think we should have some intermediate option between --silent and the default.

Thanks for merging!

@deivid-rodriguez deivid-rodriguez deleted the update-rubygems branch January 23, 2022 11:00
asbjornu added a commit to asbjornu/bitbear.music that referenced this pull request Jan 17, 2023
Require RubyGems version 3.3.22 with the `rubygems` property as
implemented in ruby/setup-ruby#271.
asbjornu added a commit to asbjornu/bitbear.music that referenced this pull request Jan 17, 2023
Require RubyGems version 3.3.22 with the `rubygems` property as
implemented in ruby/setup-ruby#271.
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.

3 participants