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

darwin.libiconv: 50 -> 99 and move to pkgs/by-name #301354

Merged
merged 9 commits into from
Apr 28, 2024

Conversation

reckenrode
Copy link
Contributor

Description of changes

This is another attempt at #299613. The reasoning behind #238993 turned out to be mistaken. In the time between that PR and today, Apple has made several source releases. As it turns out, they switched their libiconv implementation from GNU libiconv to the implementation in FreeBSD’s libc. Updating libiconv proved to be non-trivial for a few reasons.

  • Upstream bugs. Apple’s implementation had bugs that caused libarchive test failures (leading to my adding a test case as I was working on diagnosing before a source release on Monday included a fix);
  • Build system issues. xcbuild cannot build libiconv correctly. The install names are a mess. It doesn’t handle tests are installation. Fixing would require relinking or hacking up the Xcode build project; and
  • Packaging ATF, Kyua, and Lutok. These are requirements for libiconv’s tests. They also mutually depend upon each other and have had releases in years. I opted to pin them to the same commits that FreeBSD’s ports does.

Fortunately, those issues were solvable and should not be an issue for updates going forward. libiconv now has a test suite, and the work to make the other dependencies build should be a one time cost for packaging them. I built for both aarch64-darwin and x86_64-darwin, and I confirmed that libarchive and libgit2 build and pass their tests.

The Linux builds were done on ATF, Kyua, and Lutok. I tried to build libiconv-darwin on Linux, but it failed. It may be possible with effort to make it build, but it seems rather pointless since glibc includes a libiconv implementation.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@toonn
Copy link
Contributor

toonn commented Apr 4, 2024

Building libgit2 and libarchive on macOS 10.13 fails because of kyua having a failing test:

utils/process/executor_pid_test:pid_wrap -> failed: Line 114: current != -1 no t met [0.012s]

@reckenrode
Copy link
Contributor Author

Building libgit2 and libarchive on macOS 10.13 fails because of kyua having a failing test:

utils/process/executor_pid_test:pid_wrap -> failed: Line 114: current != -1 no t met [0.012s]

If you try rebuilding it again (possibly just that drv), does it succeed? I noticed a handful of Kyua tests were flakey on x86_64-darwin, but I was hoping it was a local problem. I may also disable this whole chunk of tests on Darwin since it doesn’t support UF_UNNOUNLINK.

@reckenrode reckenrode force-pushed the libiconv-switch-mk2 branch 4 times, most recently from 192a979 to acf93d9 Compare April 9, 2024 12:12
@reckenrode
Copy link
Contributor Author

I fixed the conflicts and changed the libiconv tests to execute depending on whether a tests option was set (rather than whether it detected ATF).

@reckenrode
Copy link
Contributor Author

Now the conflict should be fixed. I apparently fixed it on a different branch but not the one used for this PR.

@reckenrode
Copy link
Contributor Author

While working on the cctools/ld64 update, I found a few issues with this PR that I want to fix. Temporarily setting this to draft until I can get those commits cherry-picked.

@reckenrode reckenrode marked this pull request as draft April 13, 2024 02:43
@reckenrode reckenrode marked this pull request as ready for review April 26, 2024 12:13
Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

LGTM, mostly have some questions, no issues.
Build of libarchive and libgit2 succeeded on this branch on aarch64-darwin, still running on x86_64-darwin but consider it a formality.

pkgs/top-level/darwin-aliases.nix Show resolved Hide resolved
pkgs/stdenv/darwin/default.nix Show resolved Hide resolved
pkgs/stdenv/darwin/default.nix Show resolved Hide resolved
pkgs/by-name/lu/lutok/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/at/atf/package.nix Show resolved Hide resolved
pkgs/by-name/li/libiconv-darwin/package.nix Show resolved Hide resolved
pkgs/by-name/li/libiconv-darwin/meson.build Show resolved Hide resolved
pkgs/by-name/li/libiconv-darwin/meson.build Show resolved Hide resolved
pkgs/by-name/li/libiconv-darwin/meson.build Show resolved Hide resolved
pkgs/by-name/li/libiconv-darwin/meson.build Show resolved Hide resolved
@reckenrode reckenrode force-pushed the libiconv-switch-mk2 branch 4 times, most recently from e83378d to a1e1f63 Compare April 27, 2024 14:17
Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

Build passed on aarch64-darwin before the last force push, x86_64-darwin is still going. Should I re-run again? I didn't see much change in the last force push.

pkgs/by-name/ky/kyua/kyua-check-hook.sh Outdated Show resolved Hide resolved
pkgs/by-name/lu/lutok/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ky/kyua/package.nix Show resolved Hide resolved
@reckenrode
Copy link
Contributor Author

reckenrode commented Apr 27, 2024

Build passed on aarch64-darwin before the last force push, x86_64-darwin is still going. Should I re-run again? I didn't see much change in the last force push.

Probably not. The last force push changed it to use the setup hooks from libiconvReal, but they’re identical to the existing hooks. (Did that last build include the kyuaSetupHook change? That might be worth testing at least on aarch64-darwin if it gets past the first build of libiconv-darwin.)

@toonn
Copy link
Contributor

toonn commented Apr 27, 2024

That was the intent but apparently it didn't. I'll kick off another build.

Saw you talking to infinisil about role.bash, did you end up finding a good resolution?

@reckenrode
Copy link
Contributor Author

Saw you talking to infinisil about role.bash, did you end up finding a good resolution?

I passthru libiconvReal’s setupHook and use it in libiconv-darwin. libiconvReal isn’t in by-name, so it get to cheat for now. I’ll let whoever ends up moving it deal with how to handle role.bash if/when it’s moved there.

Corresponds to the version of libiconv in macOS 14.4.
libiconv-darwin depends on Meson, which (indirectly) depends on
libiconv. When libiconv-darwin is set as libiconv, it will cause an
infinite recursion. Avoid the infinite recursion by using libiconvReal
in stage 1. Every stage after that can use libiconv-darwin.
Avoid building these packages more than once. Even though they require
linking to dylibs, they’re only used for running tests.
Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

LGTM and builds for libarchive and libgit2 passed on both Darwins.

@toonn toonn merged commit 84df02a into NixOS:staging Apr 28, 2024
24 checks passed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/darwin-updates-news/42249/11

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

Successfully merging this pull request may close these issues.

3 participants