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

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

joshheinrichs-shopify
Copy link
Contributor

@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

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.

ext/nokogiri/extconf.rb Outdated Show resolved Hide resolved
ext/nokogiri/extconf.rb Outdated Show resolved Hide resolved
@flavorjones
Copy link
Member

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

@joshheinrichs-shopify joshheinrichs-shopify force-pushed the nix-darwin branch 2 times, most recently from e8f5a4a to 99eb909 Compare November 25, 2024 23:16
@joshheinrichs-shopify
Copy link
Contributor Author

thanks for taking a look at this! sorry for the slow response -- was on vacation last week.

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.)

there look to be at least a few options:

both appear to support darwin. the latter as far as i understand is a downstream enterprisey distribution of nix so i went with the former.

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

this appears to be a consequence of us being on a preview version of ruby (3.4.0-preview2).

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.

Looks good! I kicked off CI.

@flavorjones flavorjones self-requested a review November 26, 2024 14:59
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.

CI is failing, see my suggestions inline.

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

Kicked off CI again.

@flavorjones
Copy link
Member

@joshheinrichs-shopify CI is failing. Looks like a tricky one. My guess is that there's inconsistent header usage: xmlSwitchEncodingName was added in libxml2 v2.13.0, and so the extconf is checking for its existence using a "modern" libxml2 (maybe Nokogiri's own vendored headers?), but then compiling against something older than that (maybe nix's system headers?). I have no idea what nix package is being linked- and compiled-against so I can't verify that hunch, but something like that is likely the cause.

@joshheinrichs-shopify
Copy link
Contributor Author

hmm looks like that failing test is latching onto xcode for some reason. taking a look...

@@ -209,10 +209,14 @@ def aix?
RbConfig::CONFIG["target_os"].include?("aix")
end

def nix?
def unix?
Copy link
Member

Choose a reason for hiding this comment

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

LOL, naming things is hard

@flavorjones
Copy link
Member

Just kicked CI and it looks like the darwin-nix-enabled build is green! If everything goes green I'll merge.

@flavorjones flavorjones enabled auto-merge (squash) November 28, 2024 14:07
@flavorjones flavorjones merged commit 6af2385 into sparklemotion:main Nov 28, 2024
132 checks passed
@flavorjones
Copy link
Member

Thank you! This will be in v1.17.0 when it ships (hopefully very very soon).

flavorjones added a commit that referenced this pull request Nov 28, 2024
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