Upgrade Bundler to 2.3.22 #5509
Conversation
|
Mmm, CI is red, I'll have a look! |
|
I this ends up being another issue in Bundler, I think I'm going to need to start running dependabot tests before Bundler releases, since it's so effective at caching bugs 😅. |
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
2 times, most recently
from
a3325c7 to
96a8be1
Compare
|
Unfortunately there's another regression that it's going to affect Dependabot so let's postpone the upgrade one more release... :( |
|
041415e426ef0f1a91987aa05b5b8da1ca5d23bc and f64b5cfa5b3daa76fba2db2128e26c9b742404ca could be merged straight away though. |
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
2 times, most recently
from
14af4e3 to
dece10e
Compare
|
And of course, this time that I decided to try the upgrade before actually releasing, no regressions were found 😄. |
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
from
b257322 to
c3f09e7
Compare
|
This one should be ready now! Just noting that in the particular case that caused us to downgrade in the first place (#5420), Bundler will now create the update successfully BUT will include removing the "ruby" platform from the I understand this may cause some confusion to some users, but I think there's no much that can be done. Maybe include a note in the pull request? |
@landongrindheim So the above is not a blocker for this? Do you think it's worth doing though? |
@deivid-rodriguez I think I misread this question at first. Are you suggesting a temporary message to bundler PRs? I think that would be a nice touch, and it'd be a bonus if you have a good idea of how to implement it 🙂 I don't know that I'd consider it a blocker if we aren't breaking builds. I'd welcome the change, especially if we can do so just when the Ruby platform is removed from |
|
Yes, that was my suggestion. Add a note to the PR in case Bundler removes a platform like here. That said, I think I have to block this PR again due to another regression: rubygems/rubygems#5871. Oh boy. |
|
Technically is a musl (alpine) specific issue, so it shouldn't affect us. But I want to have a better understanding of the issue before moving this forward. |
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
from
c3f09e7 to
042bb5f
Compare
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
4 times, most recently
from
04f74b2 to
de28e39
Compare
|
That's a good question, because since dependabot currently does not respect "BUNDLED WITH", but uses its own version, users are indeed likely to run an older version locally! The answer is no, it's a one time change, and older versions should deal with it fine and stick with the new platform 👍. |
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
from
de28e39 to
eaf6fb5
Compare
|
I can do that, and I'm a big fan of keeping git history clean and meaningful but I think this may be fine. It shows the real history of this PR, getting little issues on each version I tried. The advantage of leaving this as is (in addition to "I don't have to do anything" 😆) is that if some problem is found on a specific version, it can be more easily bisected and reverted. |
|
By the way, I haven't merged this because I wanted to use this as an excuse to have a look at how PRs are built and try to add a note about the potential platform removal. But not a blocker at all as discussed above many times :) |
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
from
eaf6fb5 to
6121218
Compare
There was a problem hiding this comment.
I think we should move forward with this, without spending any time on adding a temp note to the PR about the platform change... in general, most users won't notice/care, and the few do may file an issue in core and it's trivial to point them to this PR for details.
This way we land this and keep moving forward, and don't wonder "is that because of an older version of Bundler or something else??"
deivid-rodriguez
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
from
6121218 to
a159fa9
Compare
|
Ok, just gave this a fresh rebase! |
During the migration from Ruby 2.6 to Ruby 2.7 (first Ruby which includes Bundler 2.x by default), this spec caused some issues because dependabot would issue a call to the RubyGems dependency API to figure out information about Bundler. But this is not needed these days and it probably wasn't since the beginning, because dependabot should always use the running Bundler version information in this case, no need to query the API.
Some changes in Bundler 2.3.20 [1] have created a little inconsistency, because `Bundler::Definition#resolve` will now inconsistently include the Bundler gem itself. If the resolve is taken exclusively from the lockfile, then it's not included, because `Gemfile.lock` files don't lock the Bundler gem itself even if it's a dependency (only in the BUNDLED WITH section). However, when resolving from scratch, then `Bundler::Definition#resolve` will include Bundler if Bundler is a Gemfile dependency. Dependabot was making the assumption that the Bundler gem itself is always included in `Bundler::Definition#resolve`, and thus the outcome of one spec now changes, because the way we short-circuit logic for Bundler no longer works. I'm not a fan of the inconsistency created on the Bundler side (the mentioned commit was meant to reduce inconsistencies, not the opposite). However, this can be easily fixed in dependabot itself by short-circuiting earlier in the Bundler case, before even looking at the definition. [1] rubygems/rubygems@2777e79
This should be fixed upstream now.
jeffwidman
force-pushed
the
deivid-rodriguez/bundler-2.3.20
branch
from
a159fa9 to
b424345
Compare
I noticed the following log lines in [the output of building the `updater`](https://github.com/dependabot/dependabot-core/actions/runs/3212248230/jobs/5250946524) Docker image: ``` 32 sha256:563a023320753425444761bd82772c641463b33ea716db65998c70bdc931dd95 32 1.167 Bundler 2.3.22 is running, but your lockfile was generated with 2.2.20. Installing Bundler 2.2.20 and restarting using that version. 32 2.192 Fetching gem metadata from https://rubygems.org/. 32 2.217 Fetching bundler 2.2.20 32 2.297 Installing bundler 2.2.20 32 2.511 Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead. 32 3.308 Fetching gem metadata from https://rubygems.org/......... ``` That's probably because of #5509 which updated `bundler` to `2.3.22` which doesn't match the version of Bundler specified in `Gemfile.lock`: * https://bundler.io/blog/2022/01/23/bundler-v2-3.html * https://github.com/dependabot/dependabot-core/blob/2ba96fcc09564675d94140b7f8ee6fefa32de935/updater/Gemfile.lock#L333 So I built the docker updater image (couldn't use the core image unfortunately because it doesn't mount `/updater`, and for good reason). And then within that docker image ran: ``` $ bundler update --bundler ```
I noticed the following log lines in [the output of building the `updater`](https://github.com/dependabot/dependabot-core/actions/runs/3212248230/jobs/5250946524) Docker image: ``` 32 sha256:563a023320753425444761bd82772c641463b33ea716db65998c70bdc931dd95 32 1.167 Bundler 2.3.22 is running, but your lockfile was generated with 2.2.20. Installing Bundler 2.2.20 and restarting using that version. 32 2.192 Fetching gem metadata from https://rubygems.org/. 32 2.217 Fetching bundler 2.2.20 32 2.297 Installing bundler 2.2.20 32 2.511 Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead. 32 3.308 Fetching gem metadata from https://rubygems.org/......... ``` That's probably because of #5509 which updated `bundler` to `2.3.22` which doesn't match the version of Bundler specified in `Gemfile.lock`: * https://bundler.io/blog/2022/01/23/bundler-v2-3.html * https://github.com/dependabot/dependabot-core/blob/2ba96fcc09564675d94140b7f8ee6fefa32de935/updater/Gemfile.lock#L333 So I built the docker updater image (couldn't use the core image unfortunately because it doesn't mount `/updater`, and for good reason). And then within that docker image ran: ``` $ bundler update --bundler ```
We had to downgrade it recently due to #5420.
Now this is fixed upstream, although with the slight difference that if Dependabot follows the package manager behavior here strictly (I think it does), PRs will also remove the "ruby" platform from the
Gemfile.lockfile in these cases. I understand that this might surprise some users, but these lockfiles can no longer by bundled in newer Bundler versions otherwise, so I think it's a good thing after all.