Support gemified set
#4297
Merged
Support gemified set
#4297
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deivid-rodriguez
force-pushed
the
remove_set
branch
2 times, most recently
from
1f9b385
to
7b2a8e0
Compare
|
I submitted upstream PRs: |
deivid-rodriguez
force-pushed
the
remove_set
branch
2 times, most recently
from
507f329
to
a290552
Compare
|
Thor PR was merged, Molinillo's one is waiting on feedback from Samuel. I'll update the comments about the vendored libraries and their differences with their latest released versions later. |
|
Thor 1.1.0 has been released 🎉, I'll update this PR to use that. |
deivid-rodriguez
force-pushed
the
remove_set
branch
from
a290552
to
83fc52d
Compare
|
I updated the PR to use thor 1.1.0. I wouldn't rely on the Molinillo PR to be attended soon, so this should be ready. |
deivid-rodriguez
force-pushed
the
remove_set
branch
2 times, most recently
from
365e387
to
0e6c648
Compare
|
Got some feedback in the Molinillo PR so I updated the PR in Molinillo and this PR to include the modified version. |
deivid-rodriguez
force-pushed
the
remove_set
branch
from
0e6c648
to
2d84a90
Compare
|
Pull request to Molinillo was merged, and now this PR reflects that. |
deivid-rodriguez
force-pushed
the
remove_set
branch
3 times, most recently
from
549b2fa
to
7408d33
Compare
Not sure why but removing usages of the default gem `set` triggers some missing `Bundler::SharedHelpers::OpenSSL` missing constant errors. Fully qualify these usages to avoid it.
Actually making this a set seems to be a very noticiable performance penalty in some cases. It's not necessary so remove it.
Since it has changes to lazily load `set`.
So it can be used by rubygems too.
As long as you use the latest rubygems too.
deivid-rodriguez
force-pushed
the
remove_set
branch
from
7408d33
to
bc53b79
Compare
segiddins
reviewed
Jan 26, 2021
hsbt
approved these changes
Jan 26, 2021
|
Thanks @hsbt! I will review @segiddins comment, and try to get this ready soon. |
|
Let's do this and see how it goes! |
deivid-rodriguez
added a commit
that referenced
this pull request
Feb 2, 2021
Support gemified `set` (cherry picked from commit 25fee8b)
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the end-user or developer problem that led to this PR?
The problem is that users can't currently specify
setin their Gemfiles with the version they want to use, becausebundlerusessetand activates the default version of set unconditionally.To be honest though, the reason that made me work on this was not that one, but that I found that some usages of
Setare actually much slower than using plan arrays. In particular, I was making some improvements to the resolver, and found that due to the nature of my changes (making inclusion checks on the@platformsarray quite frequently), they were slowing down the resolver quite a lot (from 9 seconds to 12 seconds on a Gemfile proved problematic in the past). When switching to plain arrays, there was no performance penalty.What is your fix for the problem, implemented in this PR?
My fix is to replace all usages of
Setpreventing it to be supported as a default gem, fixing both of the above issues.This requires changes in both
thorandMolinillowhich I will propose upstream.Fixes #3912.
Make sure the following tasks are checked