Skip to content

libiconvReal: make default on Darwin#511070

Closed
reckenrode wants to merge 5 commits intoNixOS:stagingfrom
reckenrode:push-zrltqnunnoww
Closed

libiconvReal: make default on Darwin#511070
reckenrode wants to merge 5 commits intoNixOS:stagingfrom
reckenrode:push-zrltqnunnoww

Conversation

@reckenrode
Copy link
Copy Markdown
Contributor

@reckenrode reckenrode commented Apr 18, 2026

The Darwin libiconv tries to be compatible with GNU libiconv, but it’s not. Recent versions of Autoconf and gnulib include checks for issues in Darwin’s libiconv implementation, which has effectively turned autoreconfHook into autoBreakDarwinHook due to failing to link libiconv. Instead of continuing to work around it, make GNU libiconv the default. With the UTF-8-MAC patch, it should be a drop-in replacement.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

@reckenrode
Copy link
Copy Markdown
Contributor Author

Oh, right. I forgot to split the stdenv changes out for this PR. I’ve added them to the last commit making GNU libiconv the default.

@ofborg ofborg Bot added the 6.topic: darwin Running or building packages on Darwin label Apr 18, 2026
@reckenrode reckenrode force-pushed the push-zrltqnunnoww branch 2 times, most recently from 3ff1260 to 9d7c94d Compare April 18, 2026 04:39
@reckenrode
Copy link
Copy Markdown
Contributor Author

I fixed the eval error in pkgsStatic and rebasing on current staging.

@reckenrode reckenrode changed the title libiconv: make default on Darwin libiconvReal: make default on Darwin Apr 18, 2026
@nixpkgs-ci nixpkgs-ci Bot requested review from a team, DimitarNestorov, adevress, balsoft, bjornfor, fpletz, kashw2, me-and, philiptaron, prusnak, thiagokokada, thoughtpolice, wmertens and zivarah and removed request for a team April 18, 2026 04:57
@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. labels Apr 18, 2026
@balsoft

This comment was marked as resolved.

@balsoft balsoft moved this from In Review to Reviewed in Nixpkgs security review Apr 19, 2026
Copy link
Copy Markdown
Member

@Eveeifyeve Eveeifyeve left a comment

Choose a reason for hiding this comment

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

I see no issues with this from diff and homebrew/macports concerns mentioned to have this change be merged. The nodejs-slim issue is related to openssl which should be fixed in a completely different pr IMO.

@nixpkgs-ci nixpkgs-ci Bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 19, 2026
@balsoft balsoft moved this from Reviewed to In Review in Nixpkgs security review Apr 19, 2026
@reckenrode
Copy link
Copy Markdown
Contributor Author

My pushes have been rebasing the PR on current staging.

@reckenrode reckenrode force-pushed the push-zrltqnunnoww branch 2 times, most recently from c532be4 to d620df1 Compare April 19, 2026 19:46
@reckenrode
Copy link
Copy Markdown
Contributor Author

reckenrode commented Apr 19, 2026

There appears to be a (trivial) diff between the patch source and the vendored patch:

I changed it to use fetchurl. If I’m going to be setting patchFlags to -p0 anyway, then I might as well do that. The original reason for vendoring was that I can’t use fetchpatch2 (e.g., with extraPrefix) because it causes an infinite recursion in the Darwin stdenv bootstrap.

@reckenrode
Copy link
Copy Markdown
Contributor Author

reckenrode commented Apr 19, 2026

Once @emilazy weighs in, and she approves, I’ll merge and give #511329 a heads up.

@reckenrode
Copy link
Copy Markdown
Contributor Author

reckenrode commented Apr 21, 2026

The nodejs-slim issue is related to openssl which should be fixed in a completely different pr IMO.

I agree the nodejs-slim issues should be addressed in another PR. I was noting that problem because it the only thing in my testing that failed to build. The OpenSSL problem (fixed in #510554) is not the only one though. I ended up also having to disable the following tests. I’m not opening a PR for that because I don’t know whether it’s the right fix. My goal was only getting to the point I can build .NET and mpv.

"test-esm-import-meta-main-eval"
"test-worker-debug"
"test-worker-track-unmanaged-fds"

After that, everything except for python313Packages.aiohttp built, which is failing due to the CVE-2026-3644 fix in #508075. There is an open PR upstream to fix it at aio-libs/aiohttp#12395, but I’ll leave it for the maintainers to cherry-pick or for upstream to commit a fix. I’m not touching something sensitive outside my usual area that just to fix a build.

This mirrors the Darwin libiconv package, which does not provide a setup
hook. It is expected that libiconv will be linked explicitly on Darwin.
The Darwin libiconv tries to be compatible with GNU libiconv, but it’s
not. Recent versions of Autoconf and gnulib include checks for issues in
Darwin’s libiconv implementation, which has effectively turned
`autoreconfHook` into `autoBreakDarwinHook` due to failing to link
libiconv. Instead of continuing to work around it, make GNU libiconv the
default. With the UTF-8-MAC patch, it should be a drop-in replacement.
@balsoft balsoft moved this from Reviewed to Needs Re-review in Nixpkgs security review Apr 22, 2026
@balsoft balsoft moved this from Needs Re-review to Reviewed in Nixpkgs security review Apr 24, 2026
Copy link
Copy Markdown
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

So, I definitely don’t feel great about this, if we can avoid it. I don’t want to hard‐block it, if it’s really going to be an immense headache, but here are my thoughts:

We generally try to provide a compilation environment close to the standard Xcode one. Sure, we remove some cruft and redundant stuff from the SDK, we have a slightly different LLVM, some stuff we build from source using newer source releases, etc. – but fundamentally we try to give a build environment that is broadly structured like a standard macOS build environment with the macOS APIs and behaviour. We rely on the fact that lots of people use macOS and so upstream software tends to build successfully for macOS when given a build environment enough like a standard one. Hence the move to packaging the SDK as a whole and making xcrun work, dropping the pkg-config files from libGL, and so on. When we do diverge from the macOS standard, e.g. libc++ in the past, it often ends up causing us pain and causing interoperability problems.

GNU libiconv clearly isn’t quite compatible with the macOS one – we have to patch it up a fair bit here to get it to mimic the system one, which I already don’t love (we could presumably drop that ABI‐compatibility flag entirely as unused otherwise, and keep libiconvReal a stock GNU libiconv). Since iconv is a standard interface, it’s reasonable to say that the result is hopefully functionally compatible… but it’s definitely not bug‐compatible: it behaves differently in the face of that specific check.

That means that you can develop a program for macOS using Nix, have everything be fine, and then end up surprised that it behaves differently when built and run with a standard macOS build environment. It doesn’t seem impossible that there might be some macOS‐centric software that specifically expects Apple’s behaviour, either, even if it’s non‐compliant. (After all, there’s apparently enough stuff expecting the UTF-8-MAC encoding that we need to patch that in?)

There’s of course some other ways this kind of thing can already happen, but we mostly remove things rather than add them, and when we do change things it’s usually more along the lines of “using a newer version of a library Apple bundles in the SDK” or “patching them newer source releases to support older macOS versions” than “using an entirely different codebase to implement a standard system interface”.

So I’m wondering just how prevalent this issue is, and if it’s really worth taking this step to work around it?

(FWIW, I don’t think the Autoconf version itself matters here? I believe it’s the AM_ICONV macro shipped with GNU gettext. I couldn’t find any change relating to iconv in Autoconf 2.73.)

There’s only six files in Nixpkgs master that have am_cv_func_iconv_works in them, and one of them is actually pkgs/stdenv/linux/default.nix. Admittedly, that’s probably not all of the workarounds – there’s also pkgs/tools/security/pinentry/gettext-0.25.patch, which patches configure.ac to pull in gettext explicitly as a dependency, which we set am_cv_func_iconv_works in the build environment for, which I guess means that it propagates down when you pull in gettext, which… sure. Even with a pretty generous search query, it doesn’t seem like there’s that many of these workarounds. Some of them, like 5cbaaed and d7ebb03, were actually added for Linux.

I realize that this could get worse over time, and that not every build failure will necessarily have a workaround in tree already. But since Homebrew and MacPorts don’t and most likely won’t ever use GNU libiconv by default, if this ends up being a widespread problem, they’ll be incentivized to find maintainable solutions. I realize they don’t necessarily habitually regenerate ./configure scripts, but they’ll have to sometimes, and upstream projects will gradually migrate to the newer versions too.

It seems like this is one of those things where we have a bunch of vendored copies of something that impede our ability to do a fix in one central place. Things that pull in GNU gettext explicitly to get AM_ICONV already work out of the box with autoreconfHook; if we had to do something somewhere other than leaf packages to mitigate this issue, perhaps we could just teach autoreconfHook to more reliably use our packaged gettext for AM_ICONV. (Since Homebrew pass the same environment variable in their gettext build, and have only two other occurrences of am_cv_func_iconv_works in tree, maybe they’re just doing better than us at making sure the packaged gettext is present when regenerating stuff.)

Ultimately, I’m inclined to wait and see if this actually becomes a substantial problem, likely favour adding an explicit dependency on GNU gettext rather than overriding the cache variable directly where possible, and try to match how Homebrew and MacPorts approach the problem if it becomes more significant – ideally, we can get fixes as far upstream as possible and match our peers, since we’re going to be far from the only people running into this if it gets bad. For now, I fear the cure here is worse than the disease.

But if you strongly disagree and really want this to land in 26.05 and we can’t hash it out before the freeze, it’s okay to not treat this as a strict veto on merging :)

@reckenrode
Copy link
Copy Markdown
Contributor Author

I think that’s a reasonable take. I’m going to close this for now. If it turns out to be a widespread problem, we can reconsider. I’m currently building libiconv-115.100.1, which shows some changes related to invalid characters. Maybe we’ll get lucky, and Apple will have fixed the issue for us, but let’s take a wait and see approach.

@reckenrode reckenrode closed this Apr 26, 2026
@github-project-automation github-project-automation Bot moved this to Done in Stdenv Apr 26, 2026
@reckenrode
Copy link
Copy Markdown
Contributor Author

Alas, no luck. The issue is still there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

Status: Reviewed
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants