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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
Thanks! |
closes: #202
Instead of creating
.rake-compiler-siteconf.rb
to overrideRbConfig::MAKEFILE_CONFIG
andRbConfig::CONFIG
, set environment variables tomake install
command. SinceRbConfig
is 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-compiler
to 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.