Skip to content
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

Stop copying compiled binaries #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nobu
Copy link

@nobu nobu commented Mar 3, 2022

Considering multi-architecture systems, it is obvious that architecture-independent library directories should not contain architecture-dependent binary files.
Mixing such files often results in trying to load inproper files.

Considering multi-architecture systems, it is obvious that
architecture-independent library directories should not contain
architecture-dependent binary files.  Mixing such files often
results in trying to load inproper files.
@konsolebox
Copy link
Contributor

I'm not sure if I understand this patch properly but is it meant to restrict installing arch-specific or arch-optimizes builds from being installed natively? If a generic build is intended to be made it should be decided on compile time.

@nobu
Copy link
Author

nobu commented Mar 3, 2022

This is to place arch-specific and arch-independent files to the respective directories.
ruby/date for example, this is after rake compile by some versions.

$ find -name \*.bundle
./lib/date_core.bundle
./tmp/x86_64-darwin19/stage/lib/date_core.bundle
./tmp/x86_64-darwin19/date_core/3.1.1/date_core.bundle
./tmp/x86_64-darwin19/date_core/2.7.5/date_core.bundle
./tmp/x86_64-darwin19/date_core/2.6.9/date_core.bundle
./tmp/x86_64-darwin19/date_core/3.0.3/date_core.bundle

The lib/data_core.bundle file is not only just duplicate, but can't to tell for which version/arch.
This PR removes such duplication and confusion.

@nobu
Copy link
Author

nobu commented Mar 3, 2022

As for your question, although I'm not sure if I get your intention correctly, I guess it is unrelated probably.

@konsolebox
Copy link
Contributor

Ok but what would be the generic method for locating the .so file in :test after :compile is finished? And wouldn't this have compatibility issues? If Ruby is changing its strategy of placing .so files to other directories instead of lib just perhaps so multiplatfirm support can be allowed then it should be announced and discussed.

@konsolebox
Copy link
Contributor

Ok I'll look at this thoroughly.

@nobu
Copy link
Author

nobu commented Mar 3, 2022

The version- and arch-specific path is in the load path already, isn't it?
And Ruby has placed .so files separately from .rb files in a quarter of a century at least.

@eregon
Copy link

eregon commented Mar 3, 2022

Agreed this would be a good change, and avoids issues when using Rubies with different ABIs (e.g., 2 CRuby versions and/or ruby-head, or some CRuby version + TruffleRuby for instance).

Of course we'll need the ABI/arch-specific directory to be on $LOAD_PATH for testing locally (rake compile + rake test in gems using rake-compiler), but maybe it's already there?

@konsolebox
Copy link
Contributor

konsolebox commented Mar 3, 2022

The version- and arch-specific path is in the load path already, isn't it?

Yes but only in installed gems. Minitest for example does not specify where the binary is properly located and I only see <SOURCE>/lib in $:. Specifying the so file directly also makes sure test loads the right so file and run test against it instead of some other so files that may already exist in $:.

It also will be difficult to add a Rakefile rule that makes sure the so file exists regardless of how the compilation output worked.

A lazy rule that helps testing gems with differing environment from compile-time environments in making sure the so file isn't recompiled and cause failure like the following:

task :compile_lazy do
  Rake::Task[:compile].invoke
end.instance_eval do
  def needed?
    !File.exist?("lib/digest/xxhash.so")
  end
end

require 'rake/testtask'
Rake::TestTask.new(:test => :compile_lazy) do |t|
  t.test_files = FileList['test/test.rb']
  t.verbose = true
end

In my opinion it will be better to just filter out the so files during install time instead than risk breakage of already existing gems that rely on so files being placed in /lib.

@konsolebox
Copy link
Contributor

Of course we'll need the ABI/arch-specific directory to be on $LOAD_PATH for testing locally (rake compile + rake test in gems using rake-compiler), but maybe it's already there?

No it isn't.

@eregon
Copy link

eregon commented Mar 3, 2022

Right, we'd need to somehow teach Rake::TestTask to add the proper arch/ABI-specific dir in libs.
That doesn't seem so easy in the current version of Rake::TestTask: https://github.com/ruby/rake/blob/5c60da8644a9e4f655e819252e3b6ca77f42b7af/lib/rake/testtask.rb#L88
But I guess it could be done via prepending a module with a initialize that appends the right dirs to libs.

@kou
Copy link
Member

kou commented Mar 4, 2022

This is only related to local test case, right? (This isn't related to gem install.)

The problem is occurred in the following case, right?

$ cd ruby/date
$ ruby2.7 -S rake
$ ruby3.0 -S rake # Load date_core.so for Ruby 2.7

@konsolebox
Copy link
Contributor

Agreed this would be a good change, and avoids issues when using Rubies with different ABIs (e.g., 2 CRuby versions and/or ruby-head, or some CRuby version + TruffleRuby for instance).

Due to my confusion and not being able to recall right away that rake-compiler has got nothing to do with gem installation I wasn't able to pay attention to this, but how is not copying files to /lib going to help prevent or have got anything to do with those issues though? Rake-compiler does not install the gem. Neither is it called by gem install. Some package managers do utilize rake-compiler but the responsibility to not install arch or version specific files is theirs if they choose to use rake-compiler. Otoh an option or task can be added to rake-compiler to prevent copying files to /lib for the convenience of the package managers but the current behavior should not be changed.

@eregon
Copy link

eregon commented Mar 4, 2022

Due to my confusion and not being able to recall right away that rake-compiler has got nothing to do with gem installation I wasn't able to pay attention to this, but how is not copying files to /lib going to help prevent or have got anything to do with those issues though?

I meant for local testing, i.e., bundle exec rake test (and where :test might depend on :compile).

There is a separate issue that RubyGems makes a copy under lib/ in addition to the arch/ABI-specific dir, and that seems unnecessary but it is also AFAIK completely unrelated to rake-compiler.

@nobu Could you clarify the original issue for you? Is it like @kou said above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants