Skip to content

nixos/binfmt: Add support for using statically-linked QEMU (continuation of #160802)#300070

Closed
bltavares wants to merge 10 commits intoNixOS:masterfrom
bltavares:pkgs.qemu-user-static
Closed

nixos/binfmt: Add support for using statically-linked QEMU (continuation of #160802)#300070
bltavares wants to merge 10 commits intoNixOS:masterfrom
bltavares:pkgs.qemu-user-static

Conversation

@bltavares
Copy link

(I'm not familiar with the etiquette on updating the PR of others, and I did not find the right git commands to keep #160802 authorship during the merge. I'd like to thank @zhaofengli for the original PR and I'll investigate how to keep the authorship intact meanwhile)

Description of changes

Built on top of #160802, this commit
addresses necessary updates to bring it up to parity with nixpkgs-unstable:

  • Introduce pipewireSupport: false as a new override option
  • Remove 8.1.1 patch as qemu is 8.2.2 in nixpkgs-unstable
  • Introduce new patch to expose libaio static due to upstream meson.build changes

In order to compile, the perl dependency must also be fixed
(#299623) on the pkgsStatic environment.
With this additional changeset, nix-shell -p qemu-user-static compiles.

Tested on:

  • x86_64-linux
  • aarch64-linux

Depends on:

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:
  • 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.05 Release Notes (or backporting 23.05 and 23.11 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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: lib The Nixpkgs function library labels Mar 29, 2024
The plugin .so's do not build statically.
Not needed for a minimal qemu-user-static build.
Not needed for a minimal qemu-user-static build.
This is synonymous with the "qemu-user-static" packages available in
other distros.
The fixBinary flag will be enabled if a static emulator is in use.
@bltavares bltavares force-pushed the pkgs.qemu-user-static branch from 353b714 to 65da6e6 Compare April 1, 2024 18:36
Built on top of NixOS#160802, this commit
addresses necessary updates to bring it up to parity with `nixpkgs-unstable`:

- Introduce `pipewireSupport: false` as a new override option
- Remove 8.1.1 patch as qemu is 8.2.2 in nixpkgs-unstable
- Introduce new patch to expose libaio static due to upstream meson.build changes

In order to compile, the `perl` dependency must also be fixed
(NixOS#299623) on the `pkgsStatic` environment.
With this additional changeset, `nix-shell -p qemu-user-static` compiles.

**Tested on**:
- `x86_64-linux`
- `aarch64-linux`

**Depends on:**
- [ ] NixOS#299623
@bltavares bltavares force-pushed the pkgs.qemu-user-static branch from 65da6e6 to d01bb6a Compare April 1, 2024 18:36
@bltavares bltavares marked this pull request as ready for review April 1, 2024 18:37
@Ericson2314
Copy link
Member

I don't like how lib needs to be adjusted for this. That doesn't feel right to me. Why can't we use the regular emulator function on pkgsStatic?

@bltavares
Copy link
Author

@Ericson2314 I'm not familiar with the original intention on #160802 but a similar question was asked in the PR https://github.com/NixOS/nixpkgs/pull/160802/files#r1208073679

@jcaesar
Copy link
Contributor

jcaesar commented Apr 18, 2024

In the interest of this not being left lying around for 2 years again, would it be possible to get the changes to pkgs/ merged first and then get the options working? (If asked, I'll submit that as a PR, I'm just not sure having a third PR on the same topic would be nice.)

PS: I'm testing this right now, and I'm not actually sure its working right.
{lib, pkgs, ...}: {
  environment.etc."binfmt.d/nix-hack-qemu-user-statc.conf".text = let
    pr = pkgs.fetchFromGitHub {
      owner = "NixOS";
      repo = "nixpkgs";
      rev = "pull/300070/head"; #"d01bb6a1f7b820437406b4b341f77537c04bdc50";
      hash = "sha256-7uBcm17HVjPW5JBmEnyg+yVb1qDkiXHKfeLjR7wfyek=";
    };
    patched = import pr {system = "x86_64-linux";};
    # Workaround for https://github.com/NixOS/nixpkgs/issues/295608
    qus = patched.qemu-user-static.override {
      pkgsStatic =
        patched.pkgsStatic
        // {
          qemu = patched.pkgsStatic.qemu.override {
            texinfo = patched.pkgsStatic.texinfo.override {
              perl = pkgs.perl;
            };
            hostCpuTargets = ["aarch64-linux-user"];
          };
        };
    };
  in
    lib.concatStringsSep ":" [
      ""
      "aarch64-linux"
      "M"
      ""
      "\\x7fELF\\x02\\x01\\x01\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x00\\x02\\x00\\xb7\\x00"
      "\\xff\\xff\\xff\\xff\\xff\\xff\\xff\\x00\\xff\\xff\\xff\\xff\\xff\\xff\\x00\\xff\\xfe\\xff\\xff\\xff"
      "${qus}/bin/qemu-aarch64"
      "FOCP"
    ];
}

and running

docker run --rm -ti --platform linux/arm64 ubuntu:jammy bash -c 'apt-get update && apt-get -y install dnsutils'

yields me

…
Setting up dnsutils (1:9.18.18-0ubuntu0.22.04.2) ...
Processing triggers for libc-bin (2.35-0ubuntu3.6) ...
Segmentation fault (core dumped)
Segmentation fault (core dumped)
dpkg: error processing package libc-bin (--configure):
 installed libc-bin package post-installation script subprocess returned error exit status 139
Errors were encountered while processing:
 libc-bin
E: Sub-process /usr/bin/dpkg returned an error code (1)

@Ericson2314
Copy link
Member

The pkgs part can land first, but also needs to be reworked. We should not refer to pkgsStatic in pkgs like this.

@bltavares
Copy link
Author

I'm not familiar enough with conventions to understand how staticPkgs should be used. Is there a reference where I can learn more about these conventions?

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/docker-ignoring-platform-when-run-in-nixos/21120/17

@@ -30,6 +30,7 @@ let

getEmulator = system: (lib.systems.elaborate { inherit system; }).emulator pkgs;
Copy link
Member

Choose a reason for hiding this comment

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

What @Ericson2314 probably means is to simply optionally do this:

Suggested change
getEmulator = system: (lib.systems.elaborate { inherit system; }).emulator pkgs;
getEmulator = system: (lib.systems.elaborate { inherit system; }).emulator pkgsStatic;

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
Comment on lines +337 to +338
in [ ]
++ lib.optional hasNonFixedRule "/run/binfmt"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
in [ ]
++ lib.optional hasNonFixedRule "/run/binfmt"
in lib.optional hasNonFixedRule "/run/binfmt"

@jcaesar
Copy link
Contributor

jcaesar commented May 28, 2024

would it be possible to get the changes to pkgs/ merged first

I've gone ahead and opened the PR for that. If that does get merged, you can already use the static binfmt interpreter with

boot.binfmt.registrations = lib.genAttrs ["aarch64-linux" "armv7l-linux" "riscv64-linux"] (sys: {
  interpreter = "${pkgs.pkgsStatic.qemu-user}/bin/qemu-${(lib.systems.elaborate sys).qemuArch}";
  wrapInterpreterInShell = false;
  preserveArgvZero = true;
  matchCredentials = true;
  fixBinary = true;
});

so figuring out the module setup is less pressing. Though at least a test that ensures that pkgsStatic.qemu-user stays compiling/working would necessary, right?

Comment on lines +19 to +40
guestAgentSupport = false;
numaSupport = false;
seccompSupport = false;
alsaSupport = false;
pulseSupport = false;
sdlSupport = false;
jackSupport = false;
pipewireSupport = false;
gtkSupport = false;
vncSupport = false;
smartcardSupport = false;
spiceSupport = false;
ncursesSupport = false;
libiscsiSupport = false;
smbdSupport = false;
tpmSupport = false;
uringSupport = false;
capstoneSupport = false;
enableDocs = false;
enableTools = false;
enableBlobs = false;
hostCpuTargets = qemuTargets;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is somewhat redundant with https://github.com/NixOS/nixpkgs/pull/300070/files#diff-2165823a8d82c5dd1353601bd290df8bd431f9ee2096750d9ef655cf5d251998L256-L274, right? I think it might be nice if only one of the two could be kept.

Copy link
Contributor

@JeffLabonte JeffLabonte left a comment

Choose a reason for hiding this comment

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

With comments resolved, I approve. I need this

@SuperSandro2000 SuperSandro2000 changed the title (pkgs.qemu-user-static) Update #160802 into nixpkgs-master nixos/binfmt: Add support for using statically-linked QEMU (continuation of #160802) Jun 20, 2024
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

This is already looking really good and I think we can almost merge this

@@ -254,7 +254,9 @@ stdenv.mkDerivation (finalAttrs: {

# Add a ‘qemu-kvm’ wrapper for compatibility/convenience.
postInstall = lib.optionalString (!toolsOnly) ''
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this depend on an option we have? I think that would be nicer, to not accidentally drop it in some update.

Copy link
Member

Choose a reason for hiding this comment

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

Can you rebase this commit and move the hunks into the initial commits to get a cleaner git history?

@jcaesar
Copy link
Contributor

jcaesar commented Jun 22, 2024

@SuperSandro2000 I have a significantly cleaned up version of the changes to qemu in #314998, maybe that's easier to merge? (Should have mentioned it here sooner.)

But if you do go for this PR, note:

  1. the find-aio patch isn't necessary (aio is required for disk emulation only(?))
  2. the indirect dependency on static perl can be avoided (texinfo is required for docs only)
  3. using callPackage as is in all-packages isn't a good idea (see comment)
  4. what's pkg-info (see comment)?

(Alternatively, there's also the option of having qemu-user as a completely separate package, like so.)

@jcaesar
Copy link
Contributor

jcaesar commented Aug 26, 2024

The changes to pkgs.qemu / pkgsStatic.qemu-user have been merged. I'm now trying to get the changes to the modules in with #334859.

I don't like how lib needs to be adjusted for this. That doesn't feel right to me. Why can't we use the regular emulator function on pkgsStatic?

Not sure it's meaningful, but I can now answer that question: Not all targets can have emulators from pkgsStatic. I don't think wine will ever work, e.g. So you need to decide somewhere, per target, whether to use the emulator from pkgsStatic or not. Adjusting lib for that is most natural, I think. (Though I did clean up that area a little.)

@jcaesar
Copy link
Contributor

jcaesar commented Oct 6, 2024

#334859 has been merged and this functionality is now usable. @bltavares Would you mind if I close this PR?

@Atemu
Copy link
Member

Atemu commented Oct 6, 2024

#334859 explicitly supersedes this PR.

@Atemu Atemu closed this Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: lib The Nixpkgs function library 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants