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
Support non-gnu libc linux platforms #4082
Support non-gnu libc linux platforms #4082
Conversation
Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
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.
Hi @lloeki, thanks so much for this!
I'm sorry it took me so long to look at it since it seems like a very interesting PR. As far as I understand, it unblocks sassc-ruby
from providing a precompiled version. And sassc-ruby
is the gem I'm aware of with the extension that takes the most to compile.
No worries :) I'm glad it's being considered! This is a big conundrum, and the patch here presents an "ideal" version of a fix: I feel compelled to tell you that I am wondering how to handle the merge of this patch once merged. Indeed people will eventually start pushing linux-musl gems (they don't currently because it's not working on musl, but once it does...), and non-up-to-date rubygems running on glibc (hence It all depends on what the gem server returns as a resultset and the order in which gem matches and dependencies are processed, but it does happen. You can try it with either
That's one of the blockers, yes.
I assume you have never tried to build |
@deivid-rodriguez seems like my concerns are unwarranted, libv8-node has both
So the fix and its ultimate impact presented here appears to work in all cases. |
48707dd
to
26f3bd9
Compare
@deivid-rodriguez resolved review and rebased onto master. |
@lloeki This is awesome, thanks for your work. I reviewed it and it looks just fine to me! 🎉. Only thing I noticed is that it doesn't seem actually rebased? First commit before your changes is from last November, it seems? https://github.com/rubygems/rubygems/commits/26f3bd99973d97ecadf247f61a2cf0973dbb58dd. Can you rebase it just to make sure it's all still ✔️? And while at it, I'd squash the last commit into the first one. |
Attempting to install a gem published as both *-linux and *-linux-musl results in the incorrect gem being picked up, causing build failures due to binary incompatibility. This is caused by the `nil` wildcard swallowing the libc information upon version comparison. Handle the linux case by performing only non-wildcard equality on the version and asserting 'gnu' and nil equivalence, while preserving the current behaviour for other OSes. Followup to rubygems#2922 Closes rubygems#3174
The symmetry with the "for command line" case is made more apparent.
On past versions there were observed cases of inconsistencies when some platforms were re-parsed. Ensure that a platform's string representation parses again in a platform object equal to the original.
26f3bd9
to
2743af1
Compare
Huh. Not sure what I did there, maybe I forgot to rebase on the correct remote. Go figure, it should be correctly rebased now. |
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.
The squashing I suggested was not a big deal, so I'm merging this. Thanks so much for this fix @lloeki, I expected to release it very soon.
Ah sorry about the squash, I was scratching my head a bit to much about why I messed up the rebase and forgot about it ^^’. Thanks! |
No worries! |
Support non-gnu libc linux platforms (cherry picked from commit ce4e3f5)
Old versions of rubygems (for instance 3.0.3) don't properly detect alternative libc implementations on Linux; in particular for our case, they don't detect musl. (For reference, Rubies older than 2.7 may have shipped with an affected version of rubygems). In such cases, we fall back to use RbConfig::CONFIG['arch'] instead. Why not use `RbConfig::CONFIG['arch']` always? Because `Gem::Platform.local.to_s` does some normalization we want in other situations -- for instance, it turns `x86_64-linux-gnu` to `x86_64-linux`. So for now we only add this workaround in a specific situation where we actually know it is wrong. See also rubygems/rubygems#2922 and rubygems/rubygems#4082 Fixes DataDog/dd-trace-rb#2222
* Fix issue with old rubygems not detecting musl linux properly Old versions of rubygems (for instance 3.0.3) don't properly detect alternative libc implementations on Linux; in particular for our case, they don't detect musl. (For reference, Rubies older than 2.7 may have shipped with an affected version of rubygems). In such cases, we fall back to use RbConfig::CONFIG['arch'] instead. Why not use `RbConfig::CONFIG['arch']` always? Because `Gem::Platform.local.to_s` does some normalization we want in other situations -- for instance, it turns `x86_64-linux-gnu` to `x86_64-linux`. So for now we only add this workaround in a specific situation where we actually know it is wrong. See also rubygems/rubygems#2922 and rubygems/rubygems#4082 Fixes DataDog/dd-trace-rb#2222 * Bump libdatadog minor version in preparation for release I'm planning to release 0.7.0.1.1 with the musl fix. This will be automatically picked up by customers using the currently-released version of dd-trace-rb (1.3.0). * Fix file open permissions / directory creation Otherwise this breaks in Linux due to the rest of the spec flow. * Remove redundant directory creation in spec
I will abide by the code of conduct.
What was the end-user or developer problem that led to this PR?
Attempting to install a gem published as both
*-linux
and*-linux-musl
results in the incorrect gem being picked up, causing build failures due to binary incompatibility. This is caused by thenil
wildcard swallowing the libc information upon version comparison.Followup to #2922
Closes #3174
What is your fix for the problem, implemented in this PR?
Handle the
linux
case by performing only non-wildcard equality on the version and asserting'gnu'
andnil
equivalence, while preserving the current behaviour for other OSes.Impact of this change is expected to be limited to none, as
*-linux
gems working on non-*-linux
platforms such as *-linux-musl
are vanishingly rare due to the stringent requirements and moves required to produce them. Therefore, as codified in the codebase preceding this pull request, it can be assumed that the set of gems published as*-linux
is strictly equal to the set of gems that would be marked-linux-gnu
only (if that were a thing).Alternative considered: implement an additional, optional
@libc
attribute, thus setting the@version
one to be alwaysnil
for an@os
being'linux'
. This would do away with some of the hacks in there, but would significantly alter the existing code for very little immediate value, and would break code currently depending on checking for@version
beingmusl
on recent rubygems.Make sure the following tasks are checked