munge: Fix installation paths #319401
Merged
munge: Fix installation paths #319401
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
Result of 1 package built:
|
|
@GrahamcOfBorg test slurm |
markuskowa
reviewed
Jun 13, 2024
| "--with-pkgconfigdir=${placeholder "out"}/lib/pkgconfig" | ||
| "--with-systemdunitdir=${placeholder "out"}/lib/systemd/system" | ||
|
|
||
| # Cross-compilation fixes | ||
| "--with-libgcrypt-prefix=${libgcrypt.dev}" |
There was a problem hiding this comment.
Suggested change
| "--with-libgcrypt-prefix=${libgcrypt.dev}" | |
| "--with-libgcrypt-prefix=${lib.getDev libgcrypt}" |
There was a problem hiding this comment.
I have no opinion on the cross-compilation issue.
There was a problem hiding this comment.
+1 to getDev, but I'm not sure I understand what issue the comment refers to (even though I suspect I've seen it before)
There was a problem hiding this comment.
Okay, done.
By the way, I managed to reproduce a cross failure with pkgsCross.aarch64-multiplatform.munge when I revert faacc88.
The libgcrypt.dev in nativeBuildInputs is needed for libgcrypt.m4 to be picked up for ACLOCAL_PATH (see also #311773 (comment)). And --with-libgcrypt-prefix is probably needed because the m4 file/libgcrypt-config cannot handle multiple outputs.
It appears possible to force it to use pkg-config but that is no less hacky:
Patch
diff --git a/pkgs/tools/security/munge/default.nix b/pkgs/tools/security/munge/default.nix
index f21a9e17add3..1a7ada38fa95 100644
--- a/pkgs/tools/security/munge/default.nix
+++ b/pkgs/tools/security/munge/default.nix
@@ -1,6 +1,7 @@
{
lib,
stdenv,
+ pkg-config,
fetchFromGitHub,
autoreconfHook,
libgcrypt,
@@ -21,7 +22,7 @@ stdenv.mkDerivation (finalAttrs: {
nativeBuildInputs = [
autoreconfHook
- libgcrypt # provides libgcrypt.m4
+ pkg-config
];
buildInputs = [
@@ -44,7 +45,7 @@ stdenv.mkDerivation (finalAttrs: {
"--with-systemdunitdir=${placeholder "out"}/lib/systemd/system"
# Cross-compilation hacks
- "--with-libgcrypt-prefix=${lib.getDev libgcrypt}"
+ "GPGRT_CONFIG=${stdenv.cc.targetPrefix}pkg-config"
# workaround for cross compilation: https://github.com/dun/munge/issues/103
"ac_cv_file__dev_spx=no"
"x_ac_cv_check_fifo_recvfd=no"
@@ -57,6 +58,11 @@ stdenv.mkDerivation (finalAttrs: {
"sysconfigdir=${placeholder "out"}/etc/default"
];
+ preAutoreconf = ''
+ # For some reason, automake setup hook does not pick it up from buildInputs.
+ addAclocals "${lib.getDev libgcrypt}"
+ '';
+
postInstall = ''
# rmdir will notify us if anything new is installed to the directories.
rmdir "$out"/{var{/{lib,log}{/munge,},},etc/munge}Ideally, all of this would be resolved upstream but m4 is the opposite of exciting to work with.
markuskowa
approved these changes
Jun 19, 2024
- Format & re-order the expression. - Use finalAttrs pattern. - Fix license according to https://github.com/dun/munge/wiki/License-Info. - Use `lib.getDev` instead of directly using `dev` output. This provides loose coupling in case `dev` output is merged back to `out`. - Add comment to segregate cross-configuration hacks from rest of the configuration flags. They were introduced in faacc88 and appear to be still necessary to build `pkgsCross.aarch64-multiplatform.munge`.
Removing `src/etc/Makefile` is overly intrusive, as the file is responsible for installing pkg-config file. The issue when it would try to write to the specified out-of-store paths can be easily resolved by overriding them to ones within `$out` during installation. Also set `sysconfdir` path properly to allow configuring it with `environment.etc`. There should be no problem with missing files, as nothing was installed to `$out/etc` previously either, other than an empty `munge` directory. `localstatedir` is used at runtime and installation creates empty directories in `$out`. But since we do not merge those directories into running system, having them in `$out` serves no purpose. Creation of `StateDir` and logging is handled by systemd anyway. `runstatedir` defaults to `$(localstatedir)/run` but that has long been deprecated in favour of `/run`, so let’s fix that as well. `pkgconfigdir` and `sysconfigdir` (not to be confused with the standard `sysconfdir`) rely on FHS for proper detection and `systemdunitdir` tries to run `systemd --version`, let’s just hardcode them. `sysconfigdir` is mentioned in the newly installed and currently unused `munge.service` – a file within is loaded as `EnvironmentFile` when it exists so let’s override the path for installation. This will again make it so the file can serve as a template in `$out` but the service will load it from `/etc`. The last two options are installation-only so we can directly set them to `$out` subdirectories.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Removing
src/etc/Makefileis overly intrusive, as the file is responsible for installing pkg-config file. The issue when it would try to write to the specified out-of-store paths can be easily resolved by overriding them to ones within$outduring installation.Also set
sysconfdirpath properly to allow configuring it withenvironment.etc. There should be no problem with missing files, as nothing was installed to$out/etcpreviously either, other than an emptymungedirectory.runstatedirdefaults to$(localstatedir)/runbut that has long been deprecated in favour of/run, so let’s fix that as well.pkgconfigdirandsysconfigdir(not to be confused with the standardsysconfdir) rely on FHS for proper detection andsystemdunitdirtries to runsystemd --version, let’s just hardcode them.sysconfigdiris mentioned in the newly installedmunge.serviceand a file within is loaded asEnvironmentFilewhen it exists so let’s override the path for installation so the file can serve as a template in$out. The other two are installation-only so we can directly set them to$outsubdirectories.Also clean up the expression.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.