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

treewide: Start to break up static overlay #107238

Merged
merged 3 commits into from Jan 3, 2021

Conversation

Ericson2314
Copy link
Member

Motivation for this change

We can use use stdenv.hostPlatform.isStatic instead, and move the
logic per package. The least opionated benefit of this is that it makes
it much easier to replace packages with modified ones, as there is no
longer any issue of overlay order.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

CC @FRidh @matthewbauer

@FRidh
Copy link
Member

FRidh commented Dec 20, 2020

@matthewbauer suggested

I'm not sure if I exactly like "isStatic". Probably a better name is isStaticLibraries and isStaticBinaries for each of the stdenv adapters.

Any comment on that?

@nh2
Copy link
Contributor

nh2 commented Dec 20, 2020

+1 on having a standard way of querying whether a package should build static/shared libaries and static/dynamic executables, but:

@matthewbauer suggested

I'm not sure if I exactly like "isStatic". Probably a better name is isStaticLibraries and isStaticBinaries for each of the stdenv adapters.

I heavily agree with this.

I think isStatic is not enough, and not a good name. For my use case (building static Haskell executables), I (and some others, see #61575) need packages to build:

  • static libraries (.a files)
  • dynamic libraries as well (.so files) so that TemplateHaskell can load them
  • I do not need them to build static executables (e.g. I don't need a statically linked curl exe; that just makes it more complicated)

Others want only .a (.so being absent), and again others want static executables.

These multiple choices cannot be expressed with a single isStatic bool.

Consequently, I think that the

, enableShared ? !stdenv.hostPlatform.isStatic

proposed in this PR, is not good, and that we should rather have names like buildStaticLibs, buildSharedLibs (which can be used in any combination, except perhaps false/false because then we would not build anything), and buildStaticExes.

@Ericson2314
Copy link
Member Author

@nh2

I do not need them to build static executables (e.g. I don't need a statically linked curl exe; that just makes it more complicated)

remember the hostPlatform part. For pkgsCross, stdenv.buildPlatform.isStatic = false while stdenv.hostPlatform.isStatic = true. A build-time curl won't be statically linked.

But stepping back a bit, stdenv.buildPlatform.isStatic already exists, and the overlay has all the same issues you describe as this PR. I would think actually think this is is closer to what you want, because it's much easier to make more parameters than make more overlays.

@@ -2,7 +2,7 @@
, fetchFromGitHub, fetchurl, zlib, autoreconfHook, gettext
# Enabling all targets increases output size to a multiple.
, withAllTargets ? false, libbfd, libopcodes
, enableShared ? true
, enableShared ? !stdenv.hostPlatform.isStatic
Copy link
Member

Choose a reason for hiding this comment

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

Again, we don't want this at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is really about the libs ld itself uses, not target platform stuff --- hostPlatform from my sed is actually correct --- so I think it's OK?

@matthewbauer
Copy link
Member

matthewbauer commented Dec 21, 2020

So there's definitely a benefit to putting everything in one place - most maintainers don't know about static overlay so having lots of isStatic conditionals in their definitions will make them aware they need to test this. It also means we expect pkgsStatic to really be supported everywhere and it's more than just an after thought. I think we'll need to make sure Nixpkgs maintainers for these packages are okay with that extra burden.

@Ericson2314 Would you consider moving "isStatic" to "stdenv" instead of "stdenv.hostPlatform"? Conceptually, I think this is an important distinction because:

  • "stdenv.hostPlatform" describes what ABI and toolchain we are using. You can never link packages from two different hostPlatform's together. But "static" / "shared" is much more flexible. You can always have some packages that are static and others that aren't but they link together fine. This hasn't been explored much, but I think it's important to leave this open especially with things that need shared linking like GHC / Template Haskell.
  • "stdenv" is where the stdenv adapter is applied, so it's a natural point to determine what our stdenv.mkDerivation is going to do. This would more clearly separate the stdenv toolchain from the crossSystem description.
  • "crossSystem" should be limited to what's needed to make a cross toolchain (arch, vendor, kernel, environment, linker, compilers, etc.). That's the part of the stdenv that's special for cross-compiling. All other static logic can be shared with native compilation.

@Ericson2314
Copy link
Member Author

@matthewbauer I don't think I really believe in a "platform vs rest" of config distinction.

You can never link packages from two different hostPlatform's together.

But you can:

  • Arm hard float with soft-float ABI exists to be compatible
  • i486 -> i586 -> i686, etc.
  • RISC-V's extension modes
  • WASM presumably has feature flags for various draft things
  • Fat binaries / bootloaders / runtime cpuid() type thing

True, nothing in this world has discrete interfaces. There is always a comparability relation.

should be limited to what's needed to make a cross toolchain (arch, vendor, kernel, environment, linker, compilers, etc.).

I don't think there is a firm line where "toolchain" ends. And isStatic could very well influence the toolchain itself by deciding whether to build a dynamic linker.

"stdenv" is where the stdenv adapter is applied, so it's a natural point to determine what our stdenv.mkDerivation is going to do. This would more clearly separate the stdenv toolchain from the crossSystem description.

Well, I also am not sure we we want to stick with a stdenv adapter long-term either. We could use isStatic with the default autoconf / cmake / meson flags just like we do with the rest of *Platform


Finally, it's totally reasonable one would look at stdenv.buildPlatform.isStatic or stdenv.targetPlatform.isStatic, and that's a bit more cumbersome with stdenv.isStatic, especially because targetPackages.stdenv.isStatic is more likely to hit infinite recursion.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 3, 2021

OK if no one says anything son, I think i want to merge it because this rots decently.

  • I did @matthewbauer's change for GCC, though I have some misgivings.
  • The isStatic changes are technically orthogonal, so we can always do them after.

@FRidh
Copy link
Member

FRidh commented Jan 3, 2021

Could you rebase on master instead of having a merge.

We can use use `stdenv.hostPlatform.isStatic` instead, and move the
logic per package. The least opionated benefit of this is that it makes
it much easier to replace packages with modified ones, as there is no
longer any issue of overlay order.

CC @FRidh @matthewbauer
I am actually a bit skeptical about this, but @matthewbauer makes the
case for this in
NixOS#107238 (comment) and
I'm happy to go with it not being as in the loop on static linking stuff
as he is.
@@ -1,4 +1,7 @@
{ stdenv, fetchurl, m4, cxx ? true, withStatic ? true }:
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 think this should have been withStatic ? false all along, without a comment to justify it.

@Ericson2314 Ericson2314 merged commit b3f29f3 into NixOS:master Jan 3, 2021
@Ericson2314 Ericson2314 deleted the no-static-overlay branch January 3, 2021 20:15
Ericson2314 added a commit to Ericson2314/nixpkgs that referenced this pull request Jan 3, 2021
Picking up where NixOS#107238 left off. I think I'll have gotten all the easy
stuff with this.
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.

None yet

4 participants