Skip to content

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

Merged
toonn merged 9 commits intoNixOS:stagingfrom
reckenrode:libiconv-switch-mk2
Apr 28, 2024
Merged

darwin.libiconv: 50 -> 99 and move to pkgs/by-name#301354
toonn merged 9 commits intoNixOS:stagingfrom
reckenrode:libiconv-switch-mk2

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.

@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: vim Advanced text editor 6.topic: stdenv Standard environment labels Apr 3, 2024
@reckenrode reckenrode force-pushed the libiconv-switch-mk2 branch from d534b12 to 17efd23 Compare April 3, 2024 21:14
@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 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-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 3, 2024
@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).

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 9, 2024
@reckenrode reckenrode force-pushed the libiconv-switch-mk2 branch from acf93d9 to c75e3f6 Compare April 9, 2024 12:38
@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.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 9, 2024
@reckenrode reckenrode force-pushed the libiconv-switch-mk2 branch from c75e3f6 to d415113 Compare April 9, 2024 20:37
@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 force-pushed the libiconv-switch-mk2 branch from d415113 to 0ba1ab4 Compare April 26, 2024 12:13
@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.

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

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

@reckenrode reckenrode force-pushed the libiconv-switch-mk2 branch from a1e1f63 to d70ff7b Compare April 27, 2024 21:57
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.
@reckenrode reckenrode force-pushed the libiconv-switch-mk2 branch from d70ff7b to e7b5b44 Compare April 27, 2024 22:10
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
@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

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment 6.topic: vim Advanced text editor 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants