Skip to content

darwin.libiconv: switch to libiconvReal and add to darwin-aliases#299613

Merged
reckenrode merged 3 commits intoNixOS:stagingfrom
reckenrode:libiconv-switch
Mar 29, 2024
Merged

darwin.libiconv: switch to libiconvReal and add to darwin-aliases#299613
reckenrode merged 3 commits intoNixOS:stagingfrom
reckenrode:libiconv-switch

Conversation

@reckenrode
Copy link
Contributor

@reckenrode reckenrode commented Mar 28, 2024

Description of changes

With #238993 and #299360, it is now possible to deprecate darwin.libiconv and switch Darwin to using upstream libiconv with a compatible ABI. This PR does that and updates packages that were using darwin.libiconv to use just libiconv.

Since this affects the stdenv, I’m targeting staging. I tested by building the stdenv on both platforms and GHC on aarch64-darwin. GHC uses install_name_tool to swap libiconv from the system one to the one in nixpkgs, so that it built successfully is a good test of the ABI compatibility implemented. I also confirmed eval locally using ofborg’s outpaths.nix.

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 labels Mar 28, 2024
@reckenrode reckenrode changed the title Libiconv switch darwin.libiconv: switch to libiconvReal and add to darwin-aliases Mar 28, 2024
@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 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: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 28, 2024
ghost
ghost reviewed Mar 28, 2024
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

looks like this is the first use of the darwin compatibility abi in libiconv. I think that the substituteInPlace should be changed to replace-fail . looking over the headers it seems that defining LIBICONV_PLUG almost has the same effect except that the extensions are not defined.

nixpkgs/pkgs/development/libraries/libiconv/default.nix

Lines 38 to 45 in bc06191

for iconv_h_in in iconv.h.in iconv.h.build.in; do
substituteInPlace "include/$iconv_h_in" \
--replace "#define iconv libiconv" "" \
--replace "#define iconv_close libiconv_close" "" \
--replace "#define iconv_open libiconv_open" "" \
--replace "#define iconv_open_into libiconv_open_into" "" \
--replace "#define iconvctl libiconvctl" "" \
--replace "#define iconvlist libiconvlist" ""

looks good otherwise.

@reckenrode
Copy link
Contributor Author

reckenrode commented Mar 28, 2024

looks like this is the first use of the darwin compatibility abi in libiconv.

I’m not surprised. I added it in anticipation of doing this PR. I originally had it build the compatible ABI unconditionally on Darwin, but it was desired to keep the upstream one the default so that libiconvReal would still correspond to it, so the feature flag was added.

I think that the substituteInPlace should be changed to replace-fail .

I can add it as clean up when I get home.

looking over the headers it seems that defining LIBICONV_PLUG almost has the same effect except that the extensions are not defined.

I think I looked at that, but it didn’t do what we need. Darwin needs a stand alone build with system symbol names (for compatibility with what Apple ships). It also needs to reexport libcharset.dylib, which is a platform peculiarity due to a historical accident.

@reckenrode
Copy link
Contributor Author

reckenrode commented Mar 28, 2024

I didn’t do a full bootstrap, but I did confirm libiconv built after changing it to use --replace-fail.

@reckenrode reckenrode merged commit 1500fe6 into NixOS:staging Mar 29, 2024
@reckenrode reckenrode deleted the libiconv-switch branch March 29, 2024 03:35
@nixos-discourse
Copy link

nixos-discourse commented Mar 29, 2024

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

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

This was referenced Mar 31, 2024
Merged
Merged
@straight-shoota straight-shoota mentioned this pull request Nov 21, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: vim Advanced text editor 8.has: clean-up This PR removes packages or removes other cruft 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: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@reckenrode @nixos-discourse