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

nixStatic: fix darwin build #356983

Closed
wants to merge 1 commit into from
Closed

nixStatic: fix darwin build #356983

wants to merge 1 commit into from

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Nov 18, 2024

Things done

Depends on #355350

  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

@Mic92 Mic92 requested a review from a team November 18, 2024 15:12
Comment on lines +192 to +193
NIX_LDFLAGS = lib.optionalString (stdenv.hostPlatform.isDarwin && stdenv.hostPlatform.isStatic)
"-framework CoreFoundation -framework SystemConfiguration";
Copy link
Member

Choose a reason for hiding this comment

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

Other packages use buildInputs for this, and actual CoreFoundation (etc.) packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think adding the package will actually cause the build to link against the framework. The issue I am adding these is that I see missing symbols when linking libcurl.

Copy link
Member

Choose a reason for hiding this comment

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

Generally these forced linkages mean something is broken with the build upstream, I think.

(Also the separate framework packages are deprecated compatibility stubs now and do nothing at all.)

Comment on lines +169 to +179
(pkgs.runCommand "iconv-pkgconfig" {} ''
mkdir -p $out/lib/pkgconfig
cat > $out/lib/pkgconfig/iconv.pc <<EOF
prefix=${iconv}

Name: iconv
Description: GNU iconv
Version: ${lib.getVersion iconv}
Libs: -liconv
EOF
'')
Copy link
Member

Choose a reason for hiding this comment

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

Surely there's a better way to fix this, but if this gets the job done, I'm not opposed.

I guess you could use writeTextFile here, but that's obviously not the problem.

@Ericson2314
Copy link
Member

Yeah if we can just make sure iconv has a pkg-config file on staging after this, to replace this, that would be much better.

Copy link
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.

Apple doesn’t ship a .pc file for libiconv so it seems like an issue of the Nix build system if it’s requiring it on macOS.

Comment on lines +192 to +193
NIX_LDFLAGS = lib.optionalString (stdenv.hostPlatform.isDarwin && stdenv.hostPlatform.isStatic)
"-framework CoreFoundation -framework SystemConfiguration";
Copy link
Member

Choose a reason for hiding this comment

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

Generally these forced linkages mean something is broken with the build upstream, I think.

(Also the separate framework packages are deprecated compatibility stubs now and do nothing at all.)

Libs: -liconv
EOF
'')
iconv
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want this? This is the iconv(1) binary. libiconv is already provided by the base SDK.

@emilazy
Copy link
Member

emilazy commented Nov 19, 2024

This is an upstream non‐portable brokenness that causes problems with other platforms like musl too: libarchive/libarchive#1819 libarchive/libarchive#1766. AFAICT it’s wrong on glibc too, it just happens to work by accident somehow(?). It seems it was a regression introduced for MSYS2 support or something. I don’t think any major libiconv (Apple’s, glibc’s, …) ships a .pc file, so we probably shouldn’t ship one ourselves.

We should fix it by fixing the .pc file in libarchive as Homebrew does, invariant of platform: https://github.com/Homebrew/homebrew-core/blob/f8e9e8d4f30979dc99146b5877fce76be6d35124/Formula/lib/libarchive.rb.

The -framework stuff I’m not sure about, but it’s almost certainly something broken in Nix’s build system or in the .pc files of libraries it’s linking to.

@emilazy
Copy link
Member

emilazy commented Nov 19, 2024

BTW this package has been broken since 2023 so I don’t think it’s any kind of blocker. Static builds are pretty niche on macOS. If we fix it, we should fix it the right way, not by hacking around bugs in other packages and Nix’s build system like this.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Nov 19, 2024
@ofborg ofborg bot requested review from flokli, Artturin and lovesegfault November 19, 2024 03:18
@Mic92
Copy link
Member Author

Mic92 commented Nov 19, 2024

BTW this package has been broken since 2023 so I don’t think it’s any kind of blocker. Static builds are pretty niche on macOS. If we fix it, we should fix it the right way, not by hacking around bugs in other packages and Nix’s build system like this.

Well. Should we just disable it those libarchive fixes are in. This would take another staging cycle? It would make my life as a nix maintainer easier.

@Mic92 Mic92 mentioned this pull request Nov 19, 2024
13 tasks
@Mic92
Copy link
Member Author

Mic92 commented Nov 19, 2024

In the near time, I don't think I have the capacity to do the suggested refactorings. My goal was just to get a clean nixpkgs review for nix upgrade. In any case as a proper fix would take a staging cycle, I would suggest to disable static nix darwin builds for now so it doesn't waste build cycles on peoples machines / hydra.

@Mic92 Mic92 closed this Nov 19, 2024
@Mic92 Mic92 deleted the nix-static branch November 19, 2024 09:15
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.

4 participants