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

pkgsStatic: improve usability #65213

Merged
merged 3 commits into from Jul 30, 2019
Merged

Conversation

tobim
Copy link
Contributor

@tobim tobim commented Jul 21, 2019

Motivation for this change

Defining packages that require an extra static parameter to work with pkgsStatic in overlays is rather awkward right now. Passing static within stdenv removes the need to explicitly add most packages to the pkgsStatic attrset. The flag can be queried by attrByPath ["static"] false stdenv.

stdenvs with alternative compilers are now overridden in pkgsStatic.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

This flag can replace the extra static parameter to packages that
don't work automatically with this adapter.
@tobim
Copy link
Contributor Author

tobim commented Jul 21, 2019

cc @matthewbauer

@symphorien
Copy link
Member

this collides with the derivations which have a static output. (but there are only 3 of them https://search.nix.gsc.io/?q=outputs.*static&i=nope&files=&repos=)

@matthewbauer
Copy link
Member

How about isStatic or something like that instead.

@@ -53,6 +53,14 @@ self: super: let

in {
stdenv = foldl (flip id) super.stdenv staticAdapters;
gcc49Stdenv = foldl (flip id) super.gcc49Stdenv staticAdapters;
Copy link
Member

Choose a reason for hiding this comment

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

I'll have to test this, but I had hoped that these would get the static flags as well. They should all just extend the super.stdenv attr, but their may be some issue with. There might be some late binding that needs to happen though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expected that as well, but after adding the first patch the packages from my overlay didn't pick up the static attribute. I had to add these to get a correct build.

@tobim
Copy link
Contributor Author

tobim commented Jul 21, 2019

this collides with the derivations which have a static output. (but there are only 3 of them https://search.nix.gsc.io/?q=outputs.*static&i=nope&files=&repos=)

I don't follow, why would an attribute in stdenv collide with an output of a derivation? If this is the case, why don't we get collisions from stdenv.lib?

@tobim
Copy link
Contributor Author

tobim commented Jul 21, 2019

I'd like to add another commit to this PR: Turn buildInputs into propagatedBuildInputs, so all .as can be found when linking binaries.

@symphorien
Copy link
Member

I don't follow, why would an attribute in stdenv collide with an output of a derivation?

Since glibc has a static output, glibc.static already exists (and is a derivation, not a boolean).

@tobim
Copy link
Contributor Author

tobim commented Jul 21, 2019

Since glibc has a static output, glibc.static already exists (and is a derivation, not a boolean).

It is not my intention the add a static flag to every derivation, the flag should only be part of stdenv. If the former does happen nonetheless, I need to think of another approach.

I actually tried to force this collision by modifying zlib like so:

diff --git a/pkgs/development/libraries/zlib/default.nix b/pkgs/development/libraries/zlib/default.nix
index 7fb5be1c343..ca3b31525de 100644
--- a/pkgs/development/libraries/zlib/default.nix
+++ b/pkgs/development/libraries/zlib/default.nix
@@ -26,14 +26,14 @@ stdenv.mkDerivation (rec {
   '';
 
   outputs = [ "out" "dev" ]
-    ++ stdenv.lib.optional (shared && static) "static";
+    ++ stdenv.lib.optional static "static";
   setOutputFlags = false;
   outputDoc = "dev"; # single tiny man3 page
 
   configureFlags = stdenv.lib.optional shared "--shared"
                    ++ stdenv.lib.optional (static && !shared) "--static";
 
-  postInstall = stdenv.lib.optionalString (shared && static) ''
+  postInstall = stdenv.lib.optionalString static ''
     moveToOutput lib/libz.a "$static"
   ''
     # jww (2015-01-06): Sometimes this library install as a .so, even on

The extra stdenv.static did not cause a problem in this case.

@symphorien
Copy link
Member

It is not my intention the add a static flag to every derivation, the flag should only be part of stdenv

Ah sorry I had not understood. This is fine then.

@tobim
Copy link
Contributor Author

tobim commented Jul 22, 2019

I wasn't aware of the uses of makeStaticLibraries in the default stdenv. I dropped that change for now. Maybe the propagation should be done in an additional adapter?

@tobim
Copy link
Contributor Author

tobim commented Jul 24, 2019

@matthewbauer: Can we put the workaround for the alternate stdenvs in place until we can figure out why they don't get the overrides automatically?

Propagating all buildInputs unconditionally might be a bit too much. Technically it is only necessary if the output contains archive libraries. Should I devise a heuristic for that?

@matthewbauer
Copy link
Member

@tobim I'm not sure if a heuristic is possible, but might be interesting to look at

@matthewbauer
Copy link
Member

@GrahamcOfBorg build pkgsStatic.proot pkgsStatic.nano

@tobim
Copy link
Contributor Author

tobim commented Jul 25, 2019

The naive approach would be to simply look for any '.a' files in all outputs. It would contains lots of false positives, but it would be an improvement nonetheless.

@tobim
Copy link
Contributor Author

tobim commented Jul 25, 2019

I just noticed that the existing placeholder builtin can't be used for that, as it only works with outputs. Don't know how to achieve this then.

Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

Looks good! The main ones that I care about working are pkgsStatic.proot and pkgsStatic.nix (still requires #56281 unfortunately). Eventually we need to add a release-static.nix to make sure these are not breaking between changes (or piggyback off release-cross.nix).

@tobim
Copy link
Contributor Author

tobim commented Jul 28, 2019

@matthewbauer there is nothing left to do in this PR, can you merge?

@tobim
Copy link
Contributor Author

tobim commented Jul 28, 2019

My use case is a proprietary tool that comes with support for arrow. I will open a PR for static arrow soon.

@FRidh
Copy link
Member

FRidh commented Mar 29, 2020

I think we should get rid of propagating build inputs by using propagatedBuildInputs. The problem with propagatedBuildInputs is that it creates a file, nix-support/propagated-build-inputs, with the references causing them to become run-time dependencies, defying the purpose. Now, one could move out the generated binaries, but in my opinion that should not be necessary for small closure size.

@tobim
Copy link
Contributor Author

tobim commented Mar 29, 2020

@FRidh for any package that has transitive dependencies outside of the stuff that is always present (libc, libgcc...), buildInputs have to be propagated in order for linking to succeed. I admit the fix implemented here is not perfect, but I haven't been able to come up with a better solution yet.

We could think about automatically splitting the outputs, but I'm not sure how to deal with packages that have an explicit lib in that case.

@FRidh
Copy link
Member

FRidh commented Mar 29, 2020

Yes I understand why its done. I've opened #83667 for the discussion.

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