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

Make pkgsStatic set "static" argument to true #96223

Merged
merged 1 commit into from Sep 1, 2020

Conversation

KAction
Copy link
Contributor

@KAction KAction commented Aug 25, 2020

This change allows derivations to define "static ? false" arguments to
have way to distinguish dynamic musl build and static musl build in
cases where upstream build system can't detect it by itself.

Motivation for this change
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.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-to-distinguish-pkgsmusl-and-pkgsstatic/8745/5

@Ericson2314
Copy link
Member

CC @matthewbauer

So far, we've just been overriding individual packages in the overlay. I agree this is rather unsystematic, basically taking on the constraints of a downstream project despite living in the Nixpkgs proper.

That said, we don't add arbitrary configuration to the top-level like this. We would instead probably want to put something in stdenv.*Platform. Take a look at lib/systems/* for how that might be accomplished.

@vcunat
Copy link
Member

vcunat commented Aug 25, 2020

And... the best practice is (generally) as follows?

  • don't build both static and dynamic (in a single build)
  • default to dynamic
  • use parameter called static to switch (some others have been used)

@KAction
Copy link
Contributor Author

KAction commented Aug 26, 2020

@Ericson2314 What about this, updated version of patch?

$ nix repl ~/deve/nixpkgs
nix-repl> stdenv.targetPlatform.isStatic
false
nix-repl> pkgsMusl.stdenv.targetPlatform.isStatic
false
nix-repl> pkgsStatic.stdenv.targetPlatform.isStatic
true

This way, to add something specific to static build, change will be only in one place -- in derivation itself, without need to keep list in pkgs/top-level/static.nix. It will help out-of-tree derivations, too.

@vcunat
Copy link
Member

vcunat commented Aug 26, 2020

And packages would be written like this?

{ stdenv, foo, bar
, static ? stdenv.targetPlatform.isStatic
}:

(So that they can be configured "individually" as well, though I expect most use cases will be fully static builds anyway.)

@KAction
Copy link
Contributor Author

KAction commented Aug 26, 2020

Not sure about this. If you keep static flag, it means that you can request static build with otherwise dynamic stdenv, which may or may not work (most likely not work, since glibc networking code can't be compiled statically, as I recall), but also it means that you can request dynamic build with static stdenv, which definitely will not work, since that flag was added for a reason.

So actually I expect it to be used like this:

{ stdenv, lib }:
stdenv.mkDerivation {
  name = "foo";
  src = { ... };
  patches = lib.optionals stdenv.targetPlatform.isStatic [./patches/fix-for-static-build.patch];
}

@vcunat
Copy link
Member

vcunat commented Aug 26, 2020

Well, for me... static isn't all-or-nothing and the flag mainly means whether the output for that package will contain *.so or *.a. In practice, I believe libc is the last part that you want to make static (in the sense that you want it static usually only when you make all other libraries static a swell). Still, I do expect most of "static" use cases will be fully static builds, i.e. those you apparently have in mind.

@KAction
Copy link
Contributor Author

KAction commented Aug 26, 2020

I do not see reason to not build ".o/.a". Put .so into libs, .a into dev (as it is done in Debian).

@vcunat
Copy link
Member

vcunat commented Aug 26, 2020

I don't think that works very well in practice. Many build tools assume they both reside in the same directory, it's more difficult for you to choose which is used, etc. EDIT: most notably, pkgconfig allows different options for static and non-static but libdir is common.

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 27, 2020

Let's start with

{ stdenv, foo, bar
, static ? stdenv.targetPlatform.isStatic
}:

As @vcunat says as it's a nice small step that nevertheless solves the problem.

@KAction
Copy link
Contributor Author

KAction commented Aug 27, 2020

Am I correct that you want me to remove manual overrides in pkgs/top-level/static.nix and add static ? stdenv.targetPlatform.isStatic to individual derivations into this PR before we can merge it?

@Ericson2314
Copy link
Member

No, sorry, I was just commenting on how we might transition away from having the overlay at all. I didn't realize you had ready pushed new stuff to this. Will review in a moment

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Just a little bit more work left, thanks.

@@ -76,6 +76,7 @@ rec {
# uname -r
release = null;
};
isStatic = false;
Copy link
Member

Choose a reason for hiding this comment

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

It is good to set a default here, but let's make it a bit more nuanced, based on

++ (if (with crossSystem; isWasm || isRedox) then [(import ../../top-level/static.nix)] else []);

Suggested change
isStatic = false;
isStatic = final.isWasm || final.isRedox;

@@ -43,6 +43,10 @@ self: super: let
# ++ optional (super.stdenv.hostPlatform.libc == "glibc") ((flip overrideInStdenv) [ self.stdenv.glibc.static ])
;

exposeStatic = super: super // {
targetPlatform = super.targetPlatform // { isStatic = true; };
Copy link
Member

Choose a reason for hiding this comment

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

So we shouldn't be overriding *Platform in overlays, for a number of obscure reasons.

Instead, we should change the definition of pkgsStatic to include isStatic = true; in the crossSystem argument to Nixpkgs.

That, and the pkgs/stdenv/cross/default.nix line I already pointed out above, and are the only places static.nix is used. The fancier defaulting suggestion I made ensures that latter is already in sync with the use of the overlay, so changing pkgsCross like so is the only other fixup needed.

This change allows derivations to distinguish dynamic musl build and
static musl build in cases where upstream build system can't detect it
by itself.
@KAction
Copy link
Contributor Author

KAction commented Aug 27, 2020

@Ericson2314 Did changes as you requested, works for me as checked in #96223 (comment)

@Ericson2314 Ericson2314 merged commit 5a05601 into NixOS:master Sep 1, 2020
vcunat added a commit that referenced this pull request Sep 1, 2020
Close PR #96012 (thanks).  This "static style" was discussed on:
#96223 (comment)
vcunat pushed a commit that referenced this pull request Sep 1, 2020
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>

Amended by vcunat (isMusl != isStatic).
#96223 (comment)
@KAction KAction deleted the static branch September 1, 2020 14:59
matthewbauer added a commit to matthewbauer/nixpkgs that referenced this pull request Jan 27, 2021
This corrects an issue from
NixOS#96223, where darwin/macOS static
compilation is broken. The issue is that stdenv.hostPlatform.isStatic
is only set on Linux:

https://github.com/NixOS/nixpkgs/blob/916815204eac9e4a41f263dd9855e51380135674/pkgs/top-level/stage.nix#L221

and not macOS or other platforms. This means that we can’t static
compile on platforms platforms that can’t cross-compile to themselves.
To fix this, move isStatic from stdenv.hostPlatform to stdenv. This
avoids the need to have stdenv.hostPlatform != stdenv.buildPlatform.
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