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.

Sign up for GitHub

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

Jump to bottom

libiconvReal: implement ABI compatibility on Darwin #238993

Merged
merged 1 commit into from
Aug 5, 2023
Merged

libiconvReal: implement ABI compatibility on Darwin #238993

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions pkgs/development/libraries/libiconv/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{ fetchurl, stdenv, lib
, enableStatic ? stdenv.hostPlatform.isStatic
, enableShared ? !stdenv.hostPlatform.isStatic
, enableDarwinABICompat ? false
}:

# assert !stdenv.hostPlatform.isLinux || stdenv.hostPlatform != stdenv.buildPlatform; # TODO: improve on cross
Expand Down Expand Up @@ -28,8 +29,35 @@ stdenv.mkDerivation rec {
''
+ lib.optionalString (!enableShared) ''
sed -i -e '/preload/d' Makefile.in
''
# The system libiconv is based on libiconv 1.11 with some ABI differences. The following changes
# build a compatible libiconv on Darwin, allowing it to be sustituted in place of the system one
# using `install_name_tool`. This removes the need to for a separate, Darwin-specific libiconv
# derivation and allows Darwin to benefit from upstream updates and fixes.
+ lib.optionalString enableDarwinABICompat ''
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" ""
Comment on lines +39 to +45
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it might not be a good idea to export both versions of symbols to increase compatibility with the Linux version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both glibc and Musl use the iconv prefix. The reason for the libiconv prefix in GNU libiconv is to allow it to be used on systems that provide their own implementations in libc. However, on Darwin, the system implementation is GNU libiconv, and it’s being (or will be) replaced with this version.

As far as I can tell, (almost?) all Linux packages in nixpkgs use iconv from libc (glibc or Musl). It’s possible a Darwin app might link both the system and the separate libiconv, but I’d like to see an actual example before attempting to support that use case. I’d had to add to Darwin’s maintenance load to support something no one actually does.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, makes sense. I just worry about having the same pkgs.libiconvReal with different behaviour across platforms, even though it's probably not going to come up. How would you feel about adding a forDarwinStdenv (or whatever) option to the package to conditionalize this logic on, and setting that in the pkgs.libiconv logic rather than changing pkgs.libiconvReal directly?

Copy link
Contributor Author

@reckenrode reckenrode Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe call it enableDarwinABICompat and default it to stdenv.hostplatform.isDarwin? If a package wants to do something wacky, it can override the compatibility and use both. I’ve pushed an updated branch with that change. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion is to call it e.g. useLibcAbi, default it to false, and then adjust all-packages.nix in this manner:

diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index cca76287015..69426ff28d1 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -22372,9 +22372,7 @@ with pkgs;
       then libcIconv (if stdenv.hostPlatform != stdenv.buildPlatform
         then libcCross
         else stdenv.cc.libc)
-    else if stdenv.hostPlatform.isDarwin
-      then darwin.libiconv
-    else libiconvReal;
+    else libiconvReal.override { useLibcAbi = true; };
 
   libcIconv = libc: let
     inherit (libc) pname version;

Advantages: one fewer Darwin-specific conditional, might fix issues on other platforms that currently use libiconvReal here with probably the wrong ABI for the use-case, keeps the comment above it accurate ("We also provide libiconvReal, which will always be a standalone libiconv, just in case you want it regardless of platform." – I think if we provide something called libiconvReal with this documentation it should match the API/ABI of the actual standalone libiconv, so if we do something other than this I think we need to change the name and comment. But it seems fine to me to just do the override in libiconv's definition, because it expresses what we want from it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on Matrix, I’d like to keep the change conservative and targeted at Darwin for now. Other platforms may have their own iconv implementation, so it would be a misnomer to call that the “libc ABI”. I also don’t want to propagate Darwin’s quirk in reexporting libcharset. At least for now, I’ll be keeping the name but defaulting it to false.

When it comes time to switch Darwin to this implementation, the definition of libiconv can be updated to libiconvReal.override { enableDarwinABICompat = true; }. That will require updates to a few other packages that reference darwin.libiconv directly and the implementation of a deprecation mechanism for the darwin attrset.

Thanks for the feedback! I’ve pushed a commit that defaults this to off.

done
'';

# This is hacky, but `libiconv.dylib` needs to reexport `libcharset.dylib` to match the behavior
# of the system libiconv on Darwin. Trying to do this by modifying the `Makefile` results in an
# error linking `iconv` because `libcharset.dylib` is not at its final path yet. Avoid the error
# by building without the reexport then clean and rebuild `libiconv.dylib` with the reexport.
#
# For an explanation why `libcharset.dylib` is reexported, see:
# https://github.com/apple-oss-distributions/libiconv/blob/a167071feb7a83a01b27ec8d238590c14eb6faff/xcodeconfig/libiconv.xcconfig
postBuild = lib.optionalString enableDarwinABICompat ''
make clean -C lib
NIX_CFLAGS_COMPILE+=" -Wl,-reexport-lcharset -L. " make -C lib -j$NIX_BUILD_CORES SHELL=$SHELL
'';

configureFlags = [
(lib.enableFeature enableStatic "static")
(lib.enableFeature enableShared "shared")
Expand Down