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
Conversation
@matthewbauer suggested
Any comment on that? |
+1 on having a standard way of querying whether a package should build static/shared libaries and static/dynamic executables, but:
I heavily agree with this. I think
Others want only These multiple choices cannot be expressed with a single Consequently, I think that the
proposed in this PR, is not good, and that we should rather have names like |
remember the But stepping back a bit, |
pkgs/os-specific/darwin/apple-source-releases/libiconv/default.nix
Outdated
Show resolved
Hide resolved
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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:
|
@matthewbauer I don't think I really believe in a "platform vs rest" of config distinction.
But you can:
True, nothing in this world has discrete interfaces. There is always a comparability relation.
I don't think there is a firm line where "toolchain" ends. And
Well, I also am not sure we we want to stick with a stdenv adapter long-term either. We could use Finally, it's totally reasonable one would look at |
b79147c
to
dc900e5
Compare
OK if no one says anything son, I think i want to merge it because this rots decently.
|
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.
dc900e5
to
8e48232
Compare
@@ -1,4 +1,7 @@ | |||
{ stdenv, fetchurl, m4, cxx ? true, withStatic ? true }: |
There was a problem hiding this comment.
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.
Picking up where NixOS#107238 left off. I think I'll have gotten all the easy stuff with this.
Motivation for this change
We can use use
stdenv.hostPlatform.isStatic
instead, and move thelogic 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
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)CC @FRidh @matthewbauer