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: respect AR and RANLIB in recipes on darwin #3338

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joshheinrichs-shopify
Copy link

@joshheinrichs-shopify joshheinrichs-shopify commented Nov 15, 2024

What problem is this PR intended to solve?

When using Nix on Darwin, we ideally should be latching onto the Nix-provided AR and RANLIB. Without this, when blocking Xcode via sandbox-exec, I see errors like this while attempting to install the gem, since we end up using the default ar and runlib

xcode-select: note: No developer tools were found, requesting install.
If developer tools are located at a non-default location on disk, use `sudo xcode-select --switch path/to/Xcode.app` to specify the Xcode that you wish to use for command line developer tools, and cancel the installation dialog.
See `man xcode-select` for more details.

Have you included adequate test coverage?

No but this seems relatively harmless?

Does this change affect the behavior of either the C or the Java implementations?

No

When using Nix on Darwin, we ideally should be latching onto the
Nix-provided AR and RANLIB.
Copy link
Member

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

Hi @joshheinrichs-shopify, thanks for opening this.

Do you know if there are any github actions that we could use to test Nix-on-Darwin? (For example, we're using vmactions/freebsd-vm@v1 to test FreeBSD.)

I put in a lot of work over the past few years to thoroughly exercise compilation on supported platforms, and that coverage has allowed me to confidently make changes to extconf.rb. I'd prefer not to merge this without adding some test coverage to prevent regressing, if possible.

@@ -928,7 +928,10 @@ def configure
end

if darwin? && !cross_build_p
recipe.configure_options += ["RANLIB=/usr/bin/ranlib", "AR=/usr/bin/ar"]
recipe.configure_options += [
"RANLIB=#{ENV.fetch("RANLIB", "/usr/bin/ranlib")}",
Copy link
Member

Choose a reason for hiding this comment

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

For libxml2's configure script, if an environment variable like RANLIB is set, then it doesn't need to be passed explicitly to the configure command. Can you restructure this to something like the following

recipe.configure_options << "RANLIB=/usr/bin/ranlib" unless ENV.key?("RANLIB")
recipe.configure_options << "AR=/usr/bin/ar" unless ENV.key?("AR")

@@ -969,7 +972,10 @@ def configure
cflags = concat_flags(ENV["CFLAGS"], "-O2", "-U_FORTIFY_SOURCE", "-g")

if darwin? && !cross_build_p
recipe.configure_options += ["RANLIB=/usr/bin/ranlib", "AR=/usr/bin/ar"]
recipe.configure_options += [
"RANLIB=#{ENV.fetch("RANLIB", "/usr/bin/ranlib")}",
Copy link
Member

Choose a reason for hiding this comment

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

Same here for libxslt's configure script.

@flavorjones
Copy link
Member

I'm also curious: can you share why you're not using precompiled gems?

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.

2 participants