-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Make the setup script generated by bundle install --standalone
multiplatform
#6635
base: master
Are you sure you want to change the base?
Make the setup script generated by bundle install --standalone
multiplatform
#6635
Conversation
'/#{Gem.local_platform}/#{Gem.extension_api_version}/very_simple_binary-1.0")') | ||
end | ||
|
||
# Our CI has steps for several platforms, so each step Gem::Platform.local will return a different |
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'm wrong? Is it better to write tests for each platform?
I'll work on the CI failures. |
5f3504d
to
6ecd620
Compare
Is it an exact copy or do you need any changes? Indeed it's quite unfortunate, but I don't have any better ideas? I guess we should at least add a note to Not sure if anyone has any better ideas. |
I only copied this case. I agree to the addition of a note. |
6ecd620
to
7d7d385
Compare
I see! But it also has some logic from |
Right! I forgot this code from I think we can move |
9284bda
to
76ee89c
Compare
I think that makes sense! |
718379d
to
1462183
Compare
@deivid-rodriguez I think this one is ready to go 😁 |
@gustavothecoder It looks good but looking at the changed resolver specs, this now sounds backwards incompatible? 😬 |
It seems it is 😞 I think it's possible to do something like this: def self.local
arch = RbConfig::CONFIG["arch"]
force_mswin_version = true
@local ||= new(arch, force_mswin_version)
end
def initialize(arch, force_mswin_version = false)
case arch
when Array then
@cpu, @os, @version = arch
when String then # Update Bundler::Standalone#add_arch_formatter whenever this case is changed.
arch = "#{arch}_60" if force_mswin_version && /mswin(?:32|64)$/.match?(arch)
# ...
end
end Personally, I don't really like this solution. Maybe we shouldn't refactor |
You're probably right. Let's let this settle for a few days in case we come up with better ideas or other maintainers chime in. |
Doesn't necessarily have to be this PR, but another case to consider is prebuilt native gems such as $:.unshift File.expand_path("#{__dir__}/../#{RUBY_ENGINE}/#{Gem.ruby_api_version}/gems/nokogiri-1.15.5-x86_64-darwin/lib") The exact formatting of the suffix varies from gem to gem unfortunately. Older sorbet-static for example use: However, when generating the setup script in Bundler, we do know the list of possible suffixes given we store all of them in the lockfile so it's possibly a case of writing that list into the setup file and making it select the most appropriate one for the running platform. |
Just to brainstorm an idea here while I'm passing: perhaps we could have the shared code in its own standalone (no Anyway, as for the mswin problem: I guess what we could do is have the shared code like: def platform_for_arch(arch) # or whatever you want to call it
arch = arch.split "-"
# as usual...
end then in gem/platform we have: def initialize(arch)
case arch
when Array then
@cpu, @os, @version = arch
when String then
@cpu, @os, @version = platform_for_arch(arch) and in setup.rb we have: def local_platform
arch = RbConfig::CONFIG["arch"]
arch = "#{arch}_60" if arch =~ /mswin(?:32|64)$/
platform_for_arch(arch).compact.join "-"
end if that makes things any easier and/or clearer? |
1462183
to
3c9fbf4
Compare
The string parser has its own file because we need to reuse it in Bundler::Standalone.
3c9fbf4
to
ab125dc
Compare
Thank for contributing to the discussion @Bo98. I tried to implement your approach, and with some tweaks, I think it works. I already updated this PR with it, so I'll be grateful if you can give me feedback 😄 About the prebuilt native gems, certainly we need to handle this case to truly have a multiplatform setup script, but I can't solve this problem right now, so I would like not to increase the scope of this PR. |
What was the end-user or developer problem that led to this PR?
As discussed in #3186, removing the hardcoded platform from
setup.rb
allows--standalone
mode to support native extensions for multiple platforms, which is an improvement to the feature.What is your fix for the problem, implemented in this PR?
I created a path helper that formats
RbConfig::CONFIG['arch']
to be Rubygems standard. The helper I created is basically a copy ofGem::Platform#initialize
.My first thought was to copy
Gem::Platform#initialize
at runtime so that the arch's formatting logic remains centralized, but I haven't been able to figure out a simple way to do this.Make sure the following tasks are checked