Repository navigation
Do not use values from RbConfig if we are not building a C extension - #2774
Conversation
8aca6e9 to
3707c73
Compare
| # 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. | ||
| require "rbconfig" | ||
| env = RbConfig::CONFIG.slice("SOEXT", "CPPFLAGS", "CFLAGS", "CC", "AR", "ARFLAGS", "MAKEDIRS", "RMALL") |
There was a problem hiding this comment.
- Keep:
SOEXT - Don’t use:
CC&CFLAGS&CPPFLAGS, probablyAR&ARFLAGStoo - What about?:
MAKEDIRS,RMALL
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It's possible that people are using all pre-compiled gems + a Ruby that was built on a different system.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah this was just to get a release out.
There was a problem hiding this comment.
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.
kddnewton
left a comment
kddnewton
left a comment
There was a problem hiding this comment.
Just a small question first!
* Notably, RbConfig::CONFIG["CC"] would be the GraalVM LLVM toolchain on TruffleRuby < 24.0 and we specifically do not want to use that as already explained in comments.
3707c73 to
96fbbe6
Compare
3 participants
Related: #2705 #2725
cc FYI @ParadoxV5