ncurses: Build standard terminfo dirs into static ncurses

[NorfairKing]

Description of changes

This builds terminfo directories into ncurses when building statically, even on non-static variants of nixpkgs.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[midchildan]

I think it can be simplified to this:

Suggested change

	] ++ lib.optionals (stdenv.hostPlatform.isUnix && (enableStatic || stdenv.hostPlatform.isStatic)) [
	] ++ lib.optionals (stdenv.hostPlatform.isUnix && enableStatic) [

I don't think the package would build if stdenv.hostPlatform.isStatic == true && enableStatic == false.

[NorfairKing]

@midchildan Does enableStatic get activated automatically on stdenv.hostPlatform.isStatic?
Or do I need to change the default value for enableStatic to stdenv.hostPlatform.isStatic?

[midchildan]

The default value for enableStatic already is stdenv.hostPlatform.isStatic, so no further changes should be necessary.

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

Line 8 in 742eb48

, enableStatic ? stdenv.hostPlatform.isStatic

[NorfairKing]

Done

[NorfairKing]

@midchildan Anything else I need to do before we can merge this? I'd like to get it into 24.05.

[midchildan]

passthru.tests is failing, but this would probably be fixed with #311069

[midchildan]

@NorfairKing You'll need to get a committer to merge it. Since I reviewed this PR, you can post a link to the "PRs already reviewed" Discourse thread to give it some attention.

https://discourse.nixos.org/t/prs-already-reviewed/2617

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/1637

[JohnRTitor]

I'd like to get it into 24.05.

Probably won't happen unless backported. The last staging-next iteration was merged a few days ago.

I wonder if this should be targeting master branch though, it doesn't cause that many rebuilds.

[NorfairKing]

I could backport it once this is merged.

[doronbehar]

I don't understand, how did you find out you need to apply this change?

[NorfairKing]

@doronbehar I was statically linking against musl with pkgsMusl.

[doronbehar]

@doronbehar I was statically linking against musl with pkgsMusl.

Exactly what expression did you try to evaluate? Could you share it please?

[NorfairKing]

@doronbehar you can check here: https://github.com/NorfairKing/smos/blob/8c7f97b3aba304e6d0ac6d36493f9c22600a4572/nix/overlay.nix#L54
and also the terminfo section here: https://cs-syd.eu/posts/2024-04-20-static-linking-haskell-nix

[NorfairKing]

This PR has already been approved. I just need someone to press merge, and I don't know what anyone is waiting for .. ?
This process could use some transparency :(

[doronbehar]

@doronbehar you can check here: NorfairKing/smos@8c7f97b/nix/overlay.nix#L54 and also the terminfo section here: cs-syd.eu/posts/2024-04-20-static-linking-haskell-nix

It's a bit hard for me to dive into the context there, but I'm taking from here that basically the issue with the terminfo dirs is reproducible with (if you are inside a Nixpkgs branch):

nix build -L --impure --expr 'with (builtins.getFlake "'$PWD'").legacyPackages.${builtins.currentSystem}; ncurses.override {enableStatic = true;}'

I asked that because I wanted to claim that if you'd run instead:

nix build -L --impure --expr 'with (builtins.getFlake "'$PWD'").legacyPackages.${builtins.currentSystem}; pkgsStatic.pkgsMusl.ncurses'

You'd get the package you are interested in, without the terminfo dirs issue.

This case makes me think that in general we shouldn't add enableStatic arguments to packages, but instruct people to use pkgsStatic.${pkg}, because they really mean the same.

This PR has already been approved. I just need someone to press merge, and I don't know what anyone is waiting for .. ? This process could use some transparency :(

I might merge it, I just want to make sure that my observations are correct. I'm sorry for you were waiting so long. Your PR is not trivial although it seems simple, which makes people afraid to break packages due to it. I'm opening a discourse thread about this issue in general.

I'm sorry I wasn't transparent in my intentions when I commented here for the first time, I hope now that my concerns are clear.

[nixos-discourse]

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

https://discourse.nixos.org/t/remove-all-enablestatic-arguments-in-nixpkgs-and-instruct-people-to-use-pkgsstatic/46092/1

[doronbehar]

BTW this doesn't have to target staging branch, as the number of rebuilds is small because it touches only the static ncurses.

[NorfairKing]

CC @nh2 who's done all a lot of the work you're suggesting removing.
IIRC he knows about why we use pkgsMusl instead of pkgsStatic for static linking.

[doronbehar]

CC @nh2 who's done all the work you're suggesting removing.

The work that I suggest removing? Are you talking about removing the enableStatic arguments?

[NorfairKing]

The work that I suggest removing? Are you talking about removing the enableStatic arguments?

Yes, the discourse page https://discourse.nixos.org/t/remove-all-enablestatic-arguments-in-nixpkgs-and-instruct-people-to-use-pkgsstatic/46092 seems to suggest removing enableStatic, right?
Did I read that wrong?

This would make impossible to link statically in a nixpkgs "version" like pkgsMusl, which is what @nh2 has made possible in many cases, see #43795

This PR is just following up on the years of effort that went into that, so I'm pinging him to weigh in.

[doronbehar]

seems to suggest removing enableStatic, right?
Did I read that wrong?

Partly wrong yes. I'm suggesting to remove enableStatic, but rely instead upon stdenv.hostPlatform.isStatic - so that pkgsStatic.ncurses and/or pkgsStatic.pkgsMusl.ncurses will make the expression add the configureFlags you are missing.

This would make impossible to link statically in a nixpkgs "version" like pkgsMusl, which is what @nh2 has made possible in many cases, see #43795

OK I wasn't aware of this effort. I had in the past good experience with pkgsStatic.pkgsMusl and pkgsMusl.pkgsStatic, so that's why I suggested you to try out pkgsStatic.pkgsMusl.ncurses, and perhaps also try (it could be equivalent) pkgsMusl.pkgsStatic.ncurses, as I'm curious to know how will it work for you - that's what I asked you to try to use in my long comment. Please try out these attribute as an alternative 🙏.

[NorfairKing]

@doronbehar I'm all for improvements but can we please make them after merging this fix ?

[doronbehar]

OK I understand now the subtle difference between enableStatic and using pkgsStatic :) Sorry for holding this up, and thank you for insisting you are correct 🚀 .

[doronbehar]

Aish you could have target branch master, and could have get this update faster to your channels / flakes.

[NorfairKing]

@doronbehar thank you for the kind communication 💛

[NorfairKing]

@doronbehar Some more background info:
#61575

[github-actions]

Successfully created backport PR for staging-24.05:

• [Backport staging-24.05] ncurses: Build standard terminfo dirs into static ncurses #322169