Repository navigation
[Content Addressable] Add support for Packaging Single-ABI Gems #62
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,20 @@ | |
|
|
||
| module Cibuildgem | ||
| class CompilationTasks | ||
| attr_reader :gemspec, :native, :create_packaging_task, :extension_task | ||
| CONTENT_ADDRESSABLE_ENV = "CIBUILDGEM_CONTENT_ADDRESSABLE" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you definitely convinced we need this assignment? It feels a bit overcomplicated and hard to quickly understand to me. |
||
|
|
||
| def initialize(create_packaging_task = false, gemspec = nil) | ||
| attr_reader :gemspec, :native, :create_packaging_task, :extension_task, :content_addressable | ||
|
|
||
| def initialize( | ||
| create_packaging_task = false, | ||
| gemspec = nil, | ||
| content_addressable: ENV[CONTENT_ADDRESSABLE_ENV] == "true" | ||
| ) | ||
| @gemspec = Bundler.load_gemspec(gemspec || find_gemspec) | ||
| verify_gemspec! | ||
|
|
||
| @create_packaging_task = create_packaging_task | ||
| @content_addressable = content_addressable | ||
| end | ||
|
|
||
| def setup | ||
|
|
@@ -62,6 +69,14 @@ def define_task(path, binary_name) | |
| @extension_task.cross_platform = normalized_platform | ||
| @extension_task.cross_compile = true | ||
| @extension_task.no_native = true | ||
| if content_addressable | ||
| if @extension_task.respond_to?(:content_addressable=) | ||
| @extension_task.content_addressable = true | ||
| else | ||
| warn("WARNING: content-addressable packaging was requested, but rake-compiler does not support it. " \ | ||
| "Upgrade rake-compiler to build single-ABI gems.") | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in Slack, it would be even better to provide a |
||
| end | ||
| end | ||
|
|
||
| Rake::ExtensionTask.enable! | ||
| @extension_task.define | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ | |
|
|
||
| module Cibuildgem | ||
| class CLITest < Minitest::Test | ||
| TEST_CONTENT_ADDRESSABLE_ENV = "CIBUILDGEM_CONTENT_ADDRESSABLE" | ||
|
|
||
| def setup | ||
| super | ||
|
|
||
|
|
@@ -15,6 +17,7 @@ def setup | |
|
|
||
| def teardown | ||
| ENV.delete("RUBY_CC_VERSION") | ||
| ENV.delete(TEST_CONTENT_ADDRESSABLE_ENV) | ||
|
|
||
| super | ||
| end | ||
|
|
@@ -328,6 +331,116 @@ def test_package_when_a_rakefile_defines_an_extension_task | |
| assert_predicate($CHILD_STATUS, :success?) | ||
| end | ||
|
|
||
| def test_package_single_abi_enables_content_addressable_packaging_for_one_rake_run | ||
| cli = CLI.new([], { "include-single-abi" => true }) | ||
| compilation_task = Struct.new(:ruby_cc_version).new("3.3.8:3.2.8") | ||
| rake_calls = [] | ||
| run_rake_tasks = proc do |*tasks, env: {}| | ||
| rake_calls << [ | ||
| ENV.fetch("RUBY_CC_VERSION"), | ||
| env.fetch(TEST_CONTENT_ADDRESSABLE_ENV), | ||
| tasks, | ||
| ] | ||
| end | ||
|
|
||
| cli.stub(:compilation_task, compilation_task) do | ||
| cli.stub(:run_rake_tasks!, run_rake_tasks) do | ||
| cli.package | ||
| end | ||
| end | ||
|
|
||
| assert_equal( | ||
| [["3.3.8:3.2.8", "true", ["cibuildgem:setup", :cross, :native, :gem]]], | ||
| rake_calls, | ||
| ) | ||
| assert_nil(ENV[TEST_CONTENT_ADDRESSABLE_ENV]) | ||
| end | ||
|
|
||
| def test_package_defaults_to_multi_abi_and_preserves_ruby_cc_version_from_environment | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this naming might be out of date. We want to check if the package call defaults to |
||
| ENV["RUBY_CC_VERSION"] = "3.1.6" | ||
| cli = CLI.new | ||
| compilation_task = Struct.new(:ruby_cc_version).new("3.3.8:3.2.8") | ||
| rake_calls = [] | ||
| run_rake_tasks = proc do |*tasks, env: {}| | ||
| rake_calls << [ | ||
| ENV.fetch("RUBY_CC_VERSION"), | ||
| env.fetch(TEST_CONTENT_ADDRESSABLE_ENV), | ||
| tasks, | ||
| ] | ||
| end | ||
|
|
||
| cli.stub(:compilation_task, compilation_task) do | ||
| cli.stub(:run_rake_tasks!, run_rake_tasks) do | ||
| cli.package | ||
| end | ||
| end | ||
|
|
||
| assert_equal( | ||
| [["3.1.6", "false", ["cibuildgem:setup", :cross, :native, :gem]]], | ||
| rake_calls, | ||
| ) | ||
| end | ||
|
|
||
| def test_compilation_task_enables_content_addressable_extension_task | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a little clunky - can we combine this test with the first test or does it definitely have to be separate? |
||
| require "cibuildgem/extension_patch" | ||
|
|
||
| extension_task = Class.new do | ||
| attr_accessor :config_script, | ||
| :content_addressable, | ||
| :cross_compile, | ||
| :cross_platform, | ||
| :ext_dir, | ||
| :gem_spec, | ||
| :lib_dir, | ||
| :name, | ||
| :no_native | ||
|
|
||
| def define; end | ||
| end.new | ||
| original_current = Rake::ExtensionTask.current | ||
|
|
||
| Dir.chdir("test/fixtures/dummy_gem") do | ||
| Rake::ExtensionTask.current = extension_task | ||
| CompilationTasks.new(false, nil, content_addressable: true).setup | ||
| end | ||
|
|
||
| assert_equal(true, extension_task.content_addressable) | ||
| ensure | ||
| Rake::ExtensionTask.current = original_current if Rake::ExtensionTask.respond_to?(:current=) | ||
| Rake::Task.clear | ||
| end | ||
|
|
||
| def test_compilation_task_warns_when_rake_compiler_does_not_support_content_addressable | ||
| require "cibuildgem/extension_patch" | ||
|
|
||
| extension_task = Class.new do | ||
| attr_accessor :config_script, | ||
| :cross_compile, | ||
| :cross_platform, | ||
| :ext_dir, | ||
| :gem_spec, | ||
| :lib_dir, | ||
| :name, | ||
| :no_native | ||
|
|
||
| def define; end | ||
| end.new | ||
| original_current = Rake::ExtensionTask.current | ||
|
|
||
| _, err = capture_io do | ||
| Dir.chdir("test/fixtures/dummy_gem") do | ||
| Rake::ExtensionTask.current = extension_task | ||
| CompilationTasks.new(false, nil, content_addressable: true).setup | ||
| end | ||
| end | ||
|
|
||
| assert_includes(err, "content-addressable packaging was requested") | ||
| assert_includes(err, "Upgrade rake-compiler") | ||
| ensure | ||
| Rake::ExtensionTask.current = original_current if Rake::ExtensionTask.respond_to?(:current=) | ||
| Rake::Task.clear | ||
| end | ||
|
|
||
| def test_keep_the_extension_task_config_defined_by_the_gem | ||
| Dir.chdir("test/fixtures/with_configured_ext") do | ||
| cli = CLI.new | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
@OughtPuts I know we discussed explicitly running the rake task twice here, once for the fat and then the skinny. But it seems that rake compiler already schedules that order here. So calling run rake task here with the single abi flag we would essentially trigger multi ABI, multi ABI + skinny. Did you have a specific reason in mind as to specifically calling the run rake task twice here? Maybe to make sure multi ABI executes first? I just think it is going to take extra time to run rake task twice for multi ABI twice. 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.
Yeah I had another look at the rake-compiler implementation this morning and we're all good, how we've set things up here will result in the correct gems (single and multi) being built with rake-compiler without any additional iteration. I just couldn't remember how rake-compiler worked when we paired! Sorry for any confusion.