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

openssh: fix static build #100906

Merged
merged 2 commits into from Dec 8, 2020
Merged

openssh: fix static build #100906

merged 2 commits into from Dec 8, 2020

Conversation

@KAction
Copy link
Contributor

@KAction KAction commented Oct 18, 2020

This pull requests fixes static build of openssh and its dependency,
keyutils:

$ nix-build -A pkgsStatic.openssh

It seems that this is symptom of more general problem: if library "foo"
depends on library "bar", then to link dynamically program that uses
library "foo" it is enough to pass "-lfoo" to compiler, but it is
necessary to pass "-lfoo -lbar" (in that order) to compiler to link it
statically.

Maybe proper fix would be to add -l{bar} into NIX_LDFLAGS for every
{bar} in build closure when static build is requested
(stdenv.hostPlatform.isStatic)?

  • 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.
@KAction KAction changed the title Openssh openssh: fix static build Oct 18, 2020
@ofborg ofborg bot requested review from edolstra and aneeshusa Oct 18, 2020
#
# ~kaction
+ optionalString stdenv.hostPlatform.isStatic ''
export NIX_LDFLAGS="$NIX_LDFLAGS -lncurses"
Copy link
Contributor

@nh2 nh2 Oct 18, 2020

Fixing static builds is great, but this doesn't seem like the right way to do it.

  • NIX_LDFLAGS is a hack that should only be used as last resort. It sneaks flags past the package's build system, thus often making downstream packages not work as expected.
  • pkg-config should be able to figure that out, if used:
    % nix-shell -A openssh
    
    $ pkg-config --libs libedit
    -L/nix/store/sbfj9lhpfi5v9vwrdf9my8l1m7cwbi37-libedit-20191231-3.1/lib -ledit
    
    $ pkg-config --libs libedit --static
    -L/nix/store/sbfj9lhpfi5v9vwrdf9my8l1m7cwbi37-libedit-20191231-3.1/lib -ledit -lncurses

Notice how when --static is passed to pkg-config, it actually emits the required flags of the recursive dependencies.

The way that works is this:

$ echo $PKG_CONFIG_PATH | tr ':' '\n' | grep libedit | head -n1
/nix/store/wx8xws36cz66gabl9sapgaz9p70h7x1m-libedit-20191231-3.1-dev/lib/pkgconfig

$ cat /nix/store/wx8xws36cz66gabl9sapgaz9p70h7x1m-libedit-20191231-3.1-dev/lib/pkgconfig/libedit.pc
...
Libs: -L${libdir} -ledit
Libs.private: -lncurses 
...

Here we can see that the .pc file for libedit "remembers" its non-dynamic dependencies in Libs.private, so that when asked to be linked in statically by a downstream package, it can emit them.

This is pkg-config's direct way to address your observation:

It seems that this is symptom of more general problem: if library "foo"
depends on library "bar", then to link dynamically program that uses
library "foo" it is enough to pass "-lfoo" to compiler, but it is
necessary to pass "-lfoo -lbar" (in that order) to compiler to link it
statically.

So, the right way that this should work is that you tell openssh's build system to link itself statically, based on that it should invoke pkg-config with the --static flag, and then this manual addition of flags should not be necessary.

Clearly somewhere that process breaks down, and the best way forward is to investigate where.

Copy link
Contributor

@aneeshusa aneeshusa Oct 18, 2020

That's a really great explanation, I learned something new today about how pkg-config works. As openssh maintainer I would agree the current state of the PR doesn't look like the right way to solve things but no idea what the right approach is so thanks for stepping in and posting @nh2!

Copy link
Contributor Author

@KAction KAction Oct 19, 2020

@nh2 Thank you for review.

Unfortunately, openssh build system does not understand concept of static build, so I had to fall back on sed. Libedit was quite simple, since it used pkg-config, so I just replaced all calls to pkg-config with pkg-config --static.

Kerberos was harder. Despite library providing pkg-config files, build system uses either own shell code to figure out compiler flags. I added "kerberos.dev" into build inputs, so configure script control flow into the branch which is simpler to patch.

Copy link
Contributor

@nh2 nh2 Oct 19, 2020

@KAction Yes, that looks good, great!

Please add what you described here as a comment on top of your seds in the nix file though, so that we keep the rationale next to the code.

Your change here might actually help me with nh2/static-haskell-nix#68 which is nice.

Copy link
Contributor

@nh2 nh2 Nov 25, 2020

@KAction with the latest force-push

@KAction KAction force-pushed the KAction:openssh branch from 4879ea9 to 56a01c2 22 hours ago

you're moving back to the NIX_LDFLAGS approach -- just checking that it's intended.

I saw the updated commit message:

openssh: fix static, with-kerberos build

Error messages without this patch are quite misleading: they are
complaining about compilation where root of the problem is configure
stage.

Without these extra linking flags (mostly extracted from pkg-config
database), configure flag interprets undefined reference as missing
header/function error, resulting in problems at compile time.

so probably it's intended, and I think that's fine if there is no other way, but I'd definitely put that into a comment that explains that that is the reason why we're using NIX_LDFLAGS here for now, and also the concrete invocation that made you end up with those specific -l flags, so that we know how to update it.

Also, if you've already investigated that topic/failure a bit further, it would be great to post that info here.

Copy link
Contributor Author

@KAction KAction Nov 28, 2020

Oops, pushed wrong branch. No, it is possible to get things working without NIX_LDFLAGS.

@nh2
Copy link
Contributor

@nh2 nh2 commented Oct 19, 2020

@KAction You'll have to base the PR against staging because it's a mass rebuild (you can do it by rebasing with git and then using Github's Edit button at the top).

@FRidh FRidh added this to WIP in Staging via automation Oct 20, 2020
@FRidh FRidh moved this from WIP to Needs review in Staging Oct 20, 2020
@FRidh FRidh changed the base branch from master to staging Oct 20, 2020
@KAction
Copy link
Contributor Author

@KAction KAction commented Oct 20, 2020

@nh2 Rebased on staging, @FRidh changes target branch for me.

@expipiplus1
Copy link
Contributor

@expipiplus1 expipiplus1 commented Nov 11, 2020

It would be great to get this merged!

@nh2
Copy link
Contributor

@nh2 nh2 commented Nov 25, 2020

It would be great to get this merged!

I just replied #100906 (comment)

@KAction
Copy link
Contributor Author

@KAction KAction commented Nov 28, 2020

@nh2 Sorry for confusion. I believe it is now ready for merging.

@FRidh FRidh requested a review from nh2 Dec 8, 2020
Staging automation moved this from Needs review to Ready Dec 8, 2020
nh2
nh2 approved these changes Dec 8, 2020
@nh2 nh2 merged commit 87413f3 into NixOS:staging Dec 8, 2020
21 checks passed
Staging automation moved this from Ready to Done Dec 8, 2020
@lopsided98
Copy link
Contributor

@lopsided98 lopsided98 commented Dec 25, 2020

This breaks cross-compilation because krb5-config gets taken from the build platform package. *-config scripts are generally problematic for cross-compilation, but in this case it should work if we add host platform kerberos.dev to nativeBuildInputs, or just manually add krb5-config to PATH. This works because krb5-config is just a shell script.

lopsided98 added a commit to lopsided98/nixpkgs that referenced this issue Dec 25, 2020
krb5-config from the host platform needs to be added to PATH so it can be run
during build. This works because krb5-config is a platform independent
shell-script. Before NixOS#100906, krb5-config was not used, so we didn't run into
this problem.
lopsided98 added a commit to lopsided98/nixpkgs that referenced this issue Dec 25, 2020
krb5-config from the host platform needs to be added to PATH so it can be run
during build. This works because krb5-config is a platform independent
shell-script. Before NixOS#100906, krb5-config was not used, so we didn't run into
this problem.
@lopsided98 lopsided98 mentioned this pull request Dec 26, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants