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

extconf.rb: Send RbConfig preferences to #make via env vars #2705

Merged
merged 1 commit into from Apr 17, 2024

Conversation

ParadoxV5
Copy link
Contributor

@ParadoxV5 ParadoxV5 commented Apr 14, 2024

These record the preferences Ruby was built with.
As a library that aims to be canonical, Prism should therefore match those, especially as a part of a C extension gem.

Using RbConfig also enables cross-compiling using https://bugs.ruby-lang.org/issues/20345 or the mentioned rake-compiler.


[P.S.] Do we need tests for these, and if yes, how?

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
# Runs `make` in the root directory of the project. Note that this is the
# `Makefile` for the overall project, not the `Makefile` that is being generated
# by this script.`
def make(target)
Dir.chdir(File.expand_path("../..", __dir__)) do
system(RUBY_PLATFORM.include?("openbsd") ? "gmake" : "make", target, exception: true)
system(
RbConfig::CONFIG.slice(*%w[SOEXT CPPFLAGS CFLAGS CC AR ARFLAGS MAKEDIRS RMALL]), # env
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we extract this operation out so it’s not re-#sliced on every call?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay, it's not a big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, on hindsight, should we send RbConfig::MAKEFILE_CONFIG (unexpanded Makefile includes) over rather than RbConfig::CONFIG (expanded Makefile values)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it wouldn't make a difference right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference would be if the RbConfig::MAKEFILE_CONFIG values were expanded from variables that

  • our hand-written Makefile does not cover / cover differently, or
  • interpret users’ env vars on a finer level

Makefile Show resolved Hide resolved
@ParadoxV5
Copy link
Contributor Author

RbConfig also these entries

{
  "LIBEXT"=>"a",
  "LDSHARED"=>"gcc -shared",
  "DLDSHARED"=>"gcc -shared",
  "OUTFLAG"=>"-o ",
  "COUTFLAG"=>"-o "
}

but I’m uncertain how to integrate them.

@ParadoxV5
Copy link
Contributor Author

(clang-analyzer CI failure)

I suppose they arose because the Ruby RbConfig::CONFIG['CFLAGS'] enabled more diagnostics?
Not sure how it works.

@ParadoxV5 ParadoxV5 marked this pull request as ready for review April 14, 2024 23:27
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, can you just rebase? I fixed the clang-analyzer issues

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
# Runs `make` in the root directory of the project. Note that this is the
# `Makefile` for the overall project, not the `Makefile` that is being generated
# by this script.`
def make(target)
Dir.chdir(File.expand_path("../..", __dir__)) do
system(RUBY_PLATFORM.include?("openbsd") ? "gmake" : "make", target, exception: true)
system(
RbConfig::CONFIG.slice(*%w[SOEXT CPPFLAGS CFLAGS CC AR ARFLAGS MAKEDIRS RMALL]), # env
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay, it's not a big deal

@kddnewton
Copy link
Collaborator

Sorry, if you don't mind rebasing one more time, looks like I had a different version of clang analyzer running locally.

These record the preferences Ruby was built with.
As a library that aims to be canonical, Prism should therefore match those, especially as a part of a C extension gem.
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

3 participants