Use ENV variables to set conf paths #222
Conversation
|
Hmm... I think that users should run |
|
Hello @kou ! I have updated the PR, and this will also solve my original issue :) |
lib/rake/extensiontask.rb
Outdated
| # copy binary from temporary location to final lib | ||
| # tmp/extension_name/extension_name.{so,bundle} => lib/ | ||
| task "copy:#{@name}:#{platf}:#{ruby_ver}" => [lib_path, tmp_binary_path, "#{tmp_path}/Makefile"] do | ||
| # install in lib for native platform only | ||
| unless for_platform | ||
| sh "#{make} install target_prefix=", chdir: tmp_path | ||
| expaned_lib_path = mkintpath(File.expand_path(lib_path)) |
There was a problem hiding this comment.
Can we use relative path instead like RubyGems did rubygems/rubygems#5626 ?
| expaned_lib_path = mkintpath(File.expand_path(lib_path)) | |
| relative_lib_path = Pathname(lib_path).relative_path_from(tmp_path) |
I don't want to require mkmf.
There was a problem hiding this comment.
I have update the sitearchdir and sitelibdir to use the expanded path to be more debuggable :)
expaned_lib_path = File.expand_path(lib_path)ps. Thank you for reviewing the PR @kou !
lib/rake/extensiontask.rb
Outdated
| "target_prefix=", | ||
| ] | ||
|
|
||
| sh(*make_command, chdir: tmp_path) |
There was a problem hiding this comment.
We can't use this because make may have multiple shell components such as make -j4.
lib/rake/extensiontask.rb
Outdated
| # copy binary from temporary location to final lib | ||
| # tmp/extension_name/extension_name.{so,bundle} => lib/ | ||
| task "copy:#{@name}:#{platf}:#{ruby_ver}" => [lib_path, tmp_binary_path, "#{tmp_path}/Makefile"] do | ||
| # install in lib for native platform only | ||
| unless for_platform | ||
| sh "#{make} install target_prefix=", chdir: tmp_path | ||
| expaned_lib_path = File.expand_path(lib_path) |
There was a problem hiding this comment.
If we use absolute path, we need a special conversion on Windows that did by mkintpath:
https://github.com/ruby/ruby/blob/b5c74d548872388921402ff2db36be15e924a89b/lib/mkmf.rb#L1944-L1958
We don't need it when we use relative path.
There was a problem hiding this comment.
We also need to escape this path because this path may include a special character such as .
Hmm. We may want to use sh(*make_command_line, chdir: tmp_path) with Shellwords.shellsplit:
relative_lib_path = Pathname(lib_path).relative_path_from(tmp_path)
make_command_line = Shellwords.shellsplit(make)
make_command_line << "install"
make_command_line << "sitearchdir=#{relative_lib_path}"
make_command_line << "sitelibdir=#{relative_lib_path}"
make_command_line << "target_prefix="
sh(*make_command_line, chdir: tmp_path)There was a problem hiding this comment.
My initial thought of using relative path is that it would be easier to debug. However, the relative path is still debuggable and it will be better to avoid requiring mkmkf. (I just checked the mkmkf's file size 😅 )
Also, thank you for pointing out the Shellwords.shellsplit, and it will be much more safer to use it!
|
Could you update the PR title and description before we merge this? |
ggmichaelgo
changed the title
ggmichaelgo
changed the title
kou
changed the title
|
Thanks! |
closes: #202
Instead of creating
.rake-compiler-siteconf.rbto overrideRbConfig::MAKEFILE_CONFIGandRbConfig::CONFIG, set environment variables tomake installcommand. SinceRbConfigis suppose to be readonly, we shouldn't overwrite its values.Additionally, this update addresses a bug caused by
.rake-compiler-siteconf.rb. The file's hardcoded destination path causesrake-compilerto consistently create files in that location, even if the project is renamed or moved. Consequently, changes to the gem project's path do not trigger the file task to update the file. This fix resolves this issue.