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.

Sign up for GitHub

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

Do not use values from RbConfig if we are not building a C extension #2774

Merged
merged 1 commit into from
May 6, 2024
Merged

Do not use values from RbConfig if we are not building a C extension #2774

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 27 additions & 23 deletions ext/prism/extconf.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require "rbconfig"

if ARGV.delete("--help")
print(<<~TEXT)
USAGE: ruby #{$PROGRAM_NAME} [options]
Expand Down Expand Up @@ -40,26 +42,6 @@ def generate_templates
end
end

# We're going to need to run `make` using prism's `Makefile`. We want to match
# up as much of the configuration to the configuration that built the current
# version of Ruby as possible.
require "rbconfig"
env = RbConfig::CONFIG.slice("SOEXT", "CPPFLAGS", "CFLAGS", "CC", "AR", "ARFLAGS", "MAKEDIRS", "RMALL")

# It's possible that the Ruby that is being run wasn't actually compiled on this
# machine, in which case the configuration might be incorrect. In this case
# we'll need to do some additional checks and potentially fall back to defaults.
if env.key?("CC") && !File.exist?(env["CC"])
env.delete("CC")
env.delete("CFLAGS")
env.delete("CPPFLAGS")
end

if env.key?("AR") && !File.exist?(env["AR"])
env.delete("AR")
env.delete("ARFLAGS")
end

# 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.`
Expand All @@ -77,15 +59,37 @@ def make(env, target)

# On non-CRuby we only need the shared library since we'll interface with it
# through FFI, so we'll build only that and not the C extension. We also avoid
# `require "mkmf"` as that prepends the LLVM toolchain to PATH on TruffleRuby,
# but we want to use the native toolchain here since libprism is run natively.
# `require "mkmf"` as that prepends the GraalVM LLVM toolchain to PATH on TruffleRuby < 24.0,
# but we want to use the system toolchain here since libprism is run natively.
if RUBY_ENGINE != "ruby"
generate_templates
make(env, "build/libprism.#{RbConfig::CONFIG["SOEXT"]}")
soext = RbConfig::CONFIG["SOEXT"]
eregon marked this conversation as resolved.
Show resolved Hide resolved
# Pass SOEXT to avoid an extra subprocess just to query that
make({ "SOEXT" => soext }, "build/libprism.#{soext}")
File.write("Makefile", "all install clean:\n\t@#{RbConfig::CONFIG["NULLCMD"]}\n")
return
end

# We're going to need to run `make` using prism's `Makefile`.
# We want to use the same toolchain (compiler, flags, etc) to compile libprism.a and
# the C extension since they will be linked together.
# The C extension uses RbConfig, which contains values from the toolchain that built the running Ruby.
env = RbConfig::CONFIG.slice("SOEXT", "CPPFLAGS", "CFLAGS", "CC", "AR", "ARFLAGS", "MAKEDIRS", "RMALL")
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Keep: SOEXT
  • Don’t use: CC & CFLAGS & CPPFLAGS, probably AR & ARFLAGS too
  • What about?: MAKEDIRS, RMALL

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved that code around, I didn't change any logic for CRuby (just trying to clarify the comments a bit), so I would like to not change that in this PR.

FWIW reusing the same CC for building the extension and libprism.a seems best, so I think it makes sense to keep it there.
RbConfig::CONFIG["CC"] not existing seems very weird, how can any gem install of a gem with C extension work? Does mkmf have a fallback if RbConfig::CONFIG["CC"] does not exist?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible that people are using all pre-compiled gems + a Ruby that was built on a different system.

Copy link
Member Author

@eregon eregon May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's possible that people are using all pre-compiled gems + a Ruby that was built on a different system.

Ah right, so in that case Prism happens to be the one to fail because it's the first C extension built from source on that target system? And so any other C extension built from source would fail too (because mkmf would use RbConfig::CONFIG["CC"]).
It seems it's a user error in that case, they should not expect to be able to install native extensions from source without a native toolchain or with RbConfig pointing at a non-existing toolchain.

But, this PR changes nothing about that, I just wondered why the workaround is there.

Copy link
Contributor

@ParadoxV5 ParadoxV5 May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's possible that people are using all pre-compiled gems + a Ruby that was built on a different system.

Is it time for us to start shipping prebuilt gems as well?

Ah right, so in that case Prism happens to be the one to fail because it's the first C extension built from source on that target system? And so any other C extension built from source would fail too (because mkmf would use RbConfig::CONFIG["CC"]). It seems it's a user error in that case, they should not expect to be able to install native extensions from source without a native toolchain or with RbConfig pointing at a non-existing toolchain.

I previously thought about that too.

I just wondered why the workaround is there

To resolve #2716. Perhaps @kddnewton just wanted to make the customer happy ASAP.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this was just to get a release out.

Copy link
Member Author

Choose a reason for hiding this comment

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

One last thought about this is while this code might help with make build/libprism.a I don't see how it could help for mkmf and building the C extension, and so gem install prism should still fail. That's a mystery.
Unless mkmf check for the existance of RbConfig::CONFIG["AR"] but I don't see anything about that in https://github.com/ruby/ruby/blob/v3_3_0/lib/mkmf.rb (CC_WRAPPER is checked but not CC/AR).
I think maybe in #2716 RbConfig::CONFIG["CC"] exists but RbConfig::CONFIG["AR"] does not. And then this workaround would help, since the root Makefile will use ar instead and C extensions very rarely use AR.


# It's possible that the Ruby that is being run wasn't actually compiled on this
# machine, in which case parts of RbConfig might be incorrect. In this case
# we'll need to do some additional checks and potentially fall back to defaults.
if env.key?("CC") && !File.exist?(env["CC"])
env.delete("CC")
env.delete("CFLAGS")
env.delete("CPPFLAGS")
end

if env.key?("AR") && !File.exist?(env["AR"])
env.delete("AR")
env.delete("ARFLAGS")
end

require "mkmf"

# First, ensure that we can find the header for the prism library.
Expand Down
Loading