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
Re-bundle gems with Bundler 2.2.0 #1059
Conversation
Bundler 2.2.0 contains a fix for an issue with optional groups and the `.bundle/config` file. The issue resulted in the file being touched every time we'd run a `BUNDLE_WITH=screenshots bundle install` with a change that, if tracked, would have required all developers to set up all the screenshots automation tooling while that's only needed by platforms developers. See rubygems/rubygems#3708
5441314
to
4299f0a
Compare
You can trigger an installable build for these changes by visiting CircleCI here. |
@@ -284,6 +284,7 @@ GEM | |||
|
|||
PLATFORMS | |||
ruby | |||
x86_64-darwin-19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change happened because Bundler 2.2.0 enables the specific_platform
option by default to "properly support gems providing native platform versions with different dependencies than their pure ruby counterparts."
As far as I understand, this should make it easier/faster to run bundle install
by not requiring that gems like nokogiri
be compiled from source.
I'd expect if someone with an older Mac or an Apple Silicon one were to run bundle install
, they'd update the file with their platform, too. This makes me wary that we might get a confused message every now and again by a developer that set up the project and resulted in changes they didn't expect, exactly what this PR is trying to solve.
We can prevent this at the .bundle/config
level by adding BUNDLE_FORCE_RUBY_PLATFORM: "true"
.
But then I wonder if we'd be leaving precious seconds or minutes on the table when running bundle install
just to avoid an occasional change to the file 🤔
Then again, we don't often run bundle install
, so maybe the time penalty of always building from source is an acceptable compromise for easier maintenance? 🤔 🤔 CI caches the gems so we shouldn't worry about performances there, either.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll proceed to update the other repos once we'll make a decision on this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a win to avoid recompiling a lot on developer machines.
I don't see an issue with occasionally dirtying this file when running on ARM (I don't see anyone bundling this with Windows or Linux anytime soon...)
This seems ok to me as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok now, but we'll regret it as soon as Apple Silicon macs will start to be used as development machines.
Right now, the few Apple Silicon macs around are basically used for testing/investigating/poking, so it's ok to have a dirty repository after bundle install
in this case.
But when they'll become standard development machines for someone, I don't think it's a great experience to always have a dirty repository, or to have to remember to manually edit the file if you had to add a gem.
And I image we'll have a mix of Intel and Apple Silicon machines around for quite a long time 😰
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just bundle install
'd on WordPressAuthenticator
, using bundler 2.2.1
(since that's the version its Gemfile.lock
was already locked on) and it added additional darwin platform to the ones that was already there in this repo.
So at least it means that Bundler seems to support multiple binaries platforms in parallel, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, in this case it looks good 👍
@@ -284,6 +284,7 @@ GEM | |||
|
|||
PLATFORMS | |||
ruby | |||
x86_64-darwin-19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a win to avoid recompiling a lot on developer machines.
I don't see an issue with occasionally dirtying this file when running on ARM (I don't see anyone bundling this with Windows or Linux anytime soon...)
This seems ok to me as-is?
On WC, one of our user who happened to now use
Then got errors when installing; the solution was to make them switch back to For more details see this ref: p1608040117493700/1608037157.471800-slack-CC7L49W13 Worth taking a deeper look to ensure this won't cause any trouble in other's setup as well |
@AliSoftware it's possible that Bundler 2.2.1 addresses that kind of problem. There is an "ad hoc fix for platform regression" PR (rubygems/rubygems#4127) in there. One of the linked issues reports and error with the "Found changes from the lockfile, re-resolving dependencies because..." message. I tried to run In light of that, though, I guess we might want to proceed with caution with this update, though, maybe do it one repo at a time over a period of time so that we can get feedback on whether it works for people or not? I'll merge this to begin with. |
2.2.1 has a fix for some platform specific stuff which seems worth adopting ASAP: rubygems/rubygems#4127
Note that as mentioned in another above comment I've noticed that other repos have already migrated — especially see WordPressAuthenticator-iOS. Could be a good indicator to check if people already have issues with this one and bundler 2.2 for example since it's already landed since… well I don't know when but probably some time now |
@jleandroperez @eshurakov as per discussion above, this PR updates the repo to expect Bundler 2.2.1 to be used to manage our Ruby dependencies. Now that I merged this, can I ask you to checkout
Does it work smooth for you or do you get errors? Also, is the repo still clean afterwards or has Thanks 🙇 |
Bundler 2.2.0 contains a fix for an issue with optional groups and the
.bundle/config
file.The issue resulted in the file being touched every time we'd run a
BUNDLE_WITH=screenshots bundle install
with a change that, if tracked, would have required all developers to set up all the screenshots automation tooling while that's only needed by platforms developers. See rubygems/rubygems#3708.Test
1) Checkout this branch.
2) Check your version of Bundler with
bundle version
, if it's less than 2.2.0, take a note of what version it is (e.g. 2.1.4) then update bundlergem update bundler
. If your Bundler already is at the latest version, you can usegem info bundler
to see all the installed versions. Ideally, you'll have an older one too, e.g.:If not, you can always install an older one with
gem install bundler:<version>
, e.g.:gem install bundler:2.1.4
.3) Run the command resulting in the undesired behavior on the old version of Bundler. E.g:
BUNDLE_WITH=screenshots bundle _2.1.4_ install
. (TIL you can pass the version of Bundler to use tobundle
like that). You should see that:gem install bundler:2.2.0
." That's just default Bundler behavior, not our custom logic, but it's nice to see it because hopefully it'll help people runningbundle install
being on the expected version of the tool..bundle/config
file changed with the following diffDiscard those changes.
3) Run the command that resulted in the undesired behavior on Bundler 2.2.0.
BUNDLE_WITH=screenshots bundle install
Verify that neither the warning nor the
.bundle/config
change happened.Review
Only one platform developer required to review these changes, but anyone can perform the review.
Release
These changes do not require release notes.