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

audit: Fix build with pkgsMusl. #61574

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@nh2
Copy link
Contributor

commented May 16, 2019

Motivation for this change

The previous patches no longer applied to the current code.

Also declare necessary autoconf, automake, libtool dependencies.

Without them, the musl build gets:

	/build/audit-2.8.5/missing: line 81: aclocal-1.16: command not found
	configure.ac:16: warning: macro 'AM_PROG_LIBTOOL' not found in library
	sh: autom4te: not found
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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

audit: Fix build with pkgsMusl.
The previous patches no longer applied to the current code.

Also declare necessary autoconf, automake, libtool dependencies.

Without them, the musl build gets:

		/build/audit-2.8.5/missing: line 81: aclocal-1.16: command not found
		configure.ac:16: warning: macro 'AM_PROG_LIBTOOL' not found in library
		sh: autom4te: not found

@nh2 nh2 requested review from matthewbauer and dtzWill May 16, 2019

@endgame
Copy link
Contributor

left a comment

One question, otherwise LGTM.

This command built fine, and seems to make musl-linked binaries:

nix build '(with import (builtins.fetchTarball { url = "https://github.com/nh2/nixpkgs/archive/linxu-audit-fix-musl-build.tar.gz"; }) {}; pkgsMusl.audit)'

I checked that auditctl --help worked but not much else.

@@ -15,7 +17,7 @@ stdenv.mkDerivation rec {

outputs = [ "bin" "dev" "out" "man" ];

depsBuildBuild = [ buildPackages.stdenv.cc ];
depsBuildBuild = [ buildPackages.stdenv.cc autoconf automake libtool ];

This comment has been minimized.

Copy link
@endgame

endgame May 16, 2019

Contributor

Will the autofoo dependencies go away after the next release? IMHO add a comment here if so.

This comment has been minimized.

Copy link
@nh2

nh2 May 16, 2019

Author Contributor

I suppose they will not go away; in fact I'm not quite sure why it works without them on glibc.

Given that the current src is a release tarball, it probably has a pre-generated configure script, which should make them unnecessary; but I'm not sure why on musl that one isn't good enough.

Not relying on the pre-generated configure script may be generally preferrable by the way, because it allows swapping out the src to use e.g. a git repository (which often doesn't have pre-generated configure), and is something people often do in overrides to test newer versions.

In any case, they are depsBuildBuild, so build-only and don't make it into the closure.

This comment has been minimized.

Copy link
@endgame

endgame May 16, 2019

Contributor

That should not be the case. If it is the case, then upstream might be generating configure etc using weird versions of the autotools.

Whether or not every package should depend on autofoo to allow swapping src seems like a higher-level question than this package.

@Infinisil

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Needs to go to staging

@dtzWill

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Alternatively push the patch and autotools bits into a rebuild-avoiding optionalAttrs, but that's pretty ugly. Nevermind, although it's what I did locally to avoid mass-rebuild when poking at this same issue:

https://github.com/dtzWill/nixpkgs/commits/nixos-dtz/pkgs/os-specific/linux/audit

(forgive references to 2.8.6, commit messages were written a bit sleepy-minded 😇)

Actually looks like I did a hack job of what you've done here ;).

I don't think autotools and friends ever really go in depsBuildBuild, honestly don't think it's really "blessed" to put anything there other than buildPackages.stdenv.cc. The oracle of build-fu-and-shenanigans must be consulted! If available... :3.


I'd guess without the tools the build attempts to use the pre-generated configure which works for glibc but since it won't have the changes from the patch it doesn't for musl... does that fit?

As for generating it separately and once regardless of platform details -- in fact, while I don't know it's used much in nixpkgs this is precisely what nix itself does (kinda seen here, details are in plumbing elsewhere: https://github.com/NixOS/nix/blob/master/release.nix#L19 ), it generates a prestine source tarball first (generating the bits) and then uses the result everywhere.

I agree such a migration/change shouldn't be started as part of this PR, but perhaps an RFC or issue or something to discuss? If nothing else it'd be good to fix this before such a discussion concludes ;).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.