lima-bin: Fix completion generation

[YorikSar]

Description of changes

It was running unpatched binary which was failing and thus generating empty output. After #289517 installShellCompletion errors out because of this, which lead to broken build.

Add a call to autoPatchelf to patch limactl binary before generating completion files.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[tricktron]

@YorikSar Thank you for catching that. I'll still have to reproduce this.

Is autopatchelf not run in the fixup phase as default? If so, then I'd move the install shell completion code to the postFixup phase. That seems cleaner to me because then we can profit from the default and don't have to manually patch.

[tricktron]

@YorikSar I could reproduce it 😃. Unfortunately, the installCheck phase fails with a go runtime error:

Running phase: installCheckPhase
SIGSEGV: segmentation violation
PC=0xfffff7de1c4c m=5 sigcode=1 addr=0x60
signal arrived during cgo execution
goroutine 1 gp=0x40000021c0 m=5 mp=0x4000180008 [syscall]:
runtime.cgocall(0x11498, 0x40007a4ba8)
        /opt/hostedtoolcache/go/1.22.2/x64/src/runtime/cgocall.go:157 +0x44 fp=0x40007a4b70 sp=0x40007a4b30 pc=0x189d4
os/user._Cfunc_mygetpwuid_r(0x3e8, 0x40001a8c00, 0x400, 0x4000453d90, 0x4000453d94)
        _cgo_gotypes.go:163 +0x3c fp=0x40007a4ba0 sp=0x40007a4b70 pc=0x17d02c
os/user._C_getpwuid_r(0x3e8, 0x40001a8c00, 0x400)
...

Full logs

However, I found a working solution. Instead of applying the autopatchelf twice, can you try the following and move the installShellCompletion part to the fixupPhase like this:

  fixupPhase = ''
    runHook preFixup
    runHook postFixup
    # the shell completion only works with a patched $out/bin/limactl and therefore
    # needs to run after the autopatchelf hook is executed in postFixup.
    installShellCompletion --cmd limactl \
      --bash <($out/bin/limactl completion bash) \
      --fish <($out/bin/limactl completion fish) \
      --zsh <($out/bin/limactl completion zsh)
  '';

This should then work without any go runtime error.

[YorikSar]

@tricktron Thanks for the review.

Unfortunately, the installCheck phase fails with a go runtime error:

You're linking to aarch64-linux build. I didn't check it on this platform, and I assume that it's a very different issue.

Instead of applying the autopatchelf twice, can you try the following and move the installShellCompletion part to the fixupPhase like this:

I thought about putting it in the fixup phase, but I didn't like that all fixups will not be applied to generated completion scripts. It seems that scripts are not affected by fixups at this moment though, so I've moved it to the fixup phase as you suggested.

[tricktron]

Seems to now build again on aarch64-linux. Also builds on aarch64-darwin. Looking good, thank you😃.

[jtojnar]

This is broken. fixupPhase does quite a lot of stuff and when they override it like that, it will no longer happen. If it really needs to go to fixupPhase, it should be put it in postFixup hook.

[tricktron]

@jtojnar This does not work unfortunately because the postFixup runs before the binary patching. Anyway I think I found a way to manually patch this.

[SuperSandro2000]

postFixup should not run before binary patching. In case this really doesn't work, try using distPhase or postDistPhase

[tricktron]

@SuperSandro2000 No, postFixup does not work.

EDIT: Wow, both distPhase and postDist work. This made the build work and I assumed it worked. However, no completions file were generated in result/share. So I think that these phases are not executed at all. So this does not solve the problem.

I have never heard about this phase? What does it do and when is it run?

[YorikSar]

@SuperSandro2000 runHook first runs implicit hook, then explicit ones, so postFixup will be called before autoPatchelfHook's explicit autoPatchelfPostFixup hook. I've added another explicit postFixupHook from installPhase. I hope that keeps abstractions somewhat solid. I don't like the idea of relying on distPhase not doing anything useful and running the last in the list.

[tricktron]

@YorikSar I found another way to manually patch the binary following the manual steps here: https://nixos.wiki/wiki/Packaging/Binaries.

Basically, we go back to stdenv.mkDerivation so that we get a $CC, then patch the binary with patchelf --set-interpreter "$(cat $NIX_CC/nix-support/dynamic-linker)" $out/bin/limactl and remove the custom fixupPhase again. I also remove chmod +x $out/bin/limactl because it is not needed. Could you apply these changes?

Here is the diff:

diff --git a/pkgs/applications/virtualization/lima/bin.nix b/pkgs/applications/virtualization/lima/bin.nix
index fa94db5e1169..897cbac237de 100644
--- a/pkgs/applications/virtualization/lima/bin.nix
+++ b/pkgs/applications/virtualization/lima/bin.nix
@@ -1,4 +1,4 @@
-{ stdenvNoCC
+{ stdenv
 , lib
 , fetchurl
 , writeScript
@@ -37,38 +37,31 @@ let
     };
   };
 in
-stdenvNoCC.mkDerivation {
+stdenv.mkDerivation {
   inherit version;
   pname = "lima";
   src = fetchurl {
-    inherit (dist.${stdenvNoCC.hostPlatform.system} or
-      (throw "Unsupported system: ${stdenvNoCC.hostPlatform.system}")) url sha256;
+    inherit (dist.${stdenv.hostPlatform.system} or
+      (throw "Unsupported system: ${stdenv.hostPlatform.system}")) url sha256;
   };
 
   sourceRoot = ".";
 
   nativeBuildInputs = [ makeBinaryWrapper installShellFiles ]
-    ++ lib.optionals stdenvNoCC.isLinux [ autoPatchelfHook ];
+    ++ lib.optionals stdenv.isLinux [ autoPatchelfHook ];
 
   installPhase = ''
     runHook preInstall
     mkdir -p $out
     cp -r bin share $out
-    chmod +x $out/bin/limactl
+    patchelf --set-interpreter "$(cat $NIX_CC/nix-support/dynamic-linker)" $out/bin/limactl
     wrapProgram $out/bin/limactl \
       --prefix PATH : ${lib.makeBinPath [ qemu ]}
-    runHook postInstall
-  '';
-
-  fixupPhase = ''
-    runHook preFixup
-    runHook postFixup
-    # the shell completion only works with a patched $out/bin/limactl and so
-    # needs to run after the autoPatchelfHook is executed in postFixup.
     installShellCompletion --cmd limactl \
       --bash <($out/bin/limactl completion bash) \
       --fish <($out/bin/limactl completion fish) \
       --zsh <($out/bin/limactl completion zsh)
+    runHook postInstall
   '';
 
   doInstallCheck = true;
@@ -79,7 +72,7 @@ stdenvNoCC.mkDerivation {
 
   # Stripping removes entitlements of the binary on Darwin making it non-operational.
   # Therefore, disable stripping on Darwin.
-  dontStrip = stdenvNoCC.isDarwin;
+  dontStrip = stdenv.isDarwin;
 
   passthru.updateScript =
     let

[tricktron]

@YorikSar Seems like we can just move the shellCompletion installation to the postDist phase. Then we don´t have to patch manually.

EDIT: See #331782 (comment)

[YorikSar]

@tricktron I don't like the idea of pulling in the compiler just to patch the binary. It seems like you're reimplementing what autopatchelf already does, but using NIX_CC. I don't see the benefit of it compared to my original version.

I've modified it to add a postFixup hook to run after everything is patched. It seems cleaner than overriding fixupPhase entirely and less intrusive than pulling in the compiler or manually patching the binary.

[tricktron]

@tricktron I don't like the idea of pulling in the compiler just to patch the binary. It seems like you're reimplementing what autopatchelf already does, but using NIX_CC. I don't see the benefit of it compared to my original version.

I've modified it to add a postFixup hook to run after everything is patched. It seems cleaner than overriding fixupPhase entirely and less intrusive than pulling in the compiler or manually patching the binary.

@YorikSar I did not know about the postFixupHooks function. I completely agree, this looks much cleaner. Great job 👍.

[mjgallag]

It appears to still be failing in same way after this merged, see installShellCompletion: installed shell completion file /nix/store/8cj0mcb2d130xc2fgi76j9f2pz3fhxgl-lima-0.23.2/share/bash-completion/completions/limactl.bash' does not exist or has zero size`.

[YorikSar]

@mjgallag you’re pointing to upgrade logs, where a different version of lima is being built. Also, lines before this error are different:

�[31mFATA�[0m[0000] no such file or directory                    
�[31mFATA�[0m[0000] no such file or directory                    
�[31mFATA�[0m[0000] no such file or directory

This means that the binary was executed, but failed. Before this PR, the binary could not have been executed.

All in all, it seems like the new version does something different and fails with a different issue.

[YorikSar]

Found this in the diff: lima-vm/lima@v0.22.0...v0.23.2#diff-95475f7f5560ba438d76bce7e63eb2f2d63b59438023d2af973e941510cdcf9cR100 - it now checks home directory on each run. My guess is that it fails here. You could try setting LIMA_HOME to something in the workdir to avoid that.