libwacom: simplify tests enablement #1

Atemu · 2024-08-12T21:09:37Z

Description of changes

Things done

Add a 👍 reaction to pull requests you find important.

philiptaron

What do you think @Atemu?

philiptaron · 2024-08-12T21:14:09Z

pkgs/by-name/li/libwacom/package.nix

-    });
+  passthru = {
+    # finalAttrs.finalPackage does not have .override
+    tests = libwacom.override { withTests = true; };


Let's keep this finalAttrs.finalPackage.overrideAttrs so that libwacom-surface has the right .tests derivation.

Suggested change

tests = libwacom.override { withTests = true; };

tests = finalAttrs.finalPackage.overrideAttrs (prevAttrs: { doCheck = true; });

As mentioned, this is undesirable because it makes our code depend on doCheck which is an implementation detail of mkDerivation.

OTOH, we also need to pass through libwacom to itself which is a bit of an odd pattern and comes with some caveats.

Additionally, this isn't really something we need to expose externally as that flag itself is an implementation detail for producing a passthru attr.

What I'm saying is that libwacom-surface is defined as libwacom.overrideAttrs.

https://github.com/NixOS/nixpkgs/blob/b8e345cd4a8bd35b08858a87a12326a0099557a8/pkgs/by-name/li/libwacom-surface/package.nix

That means nix-build -A libwacom-surface.tests is going to run tests against the base, not against the patched version.

Yeah, that's simpler and breaks fewer surprises. Please pull.

philiptaron · 2024-08-12T21:14:23Z

pkgs/by-name/li/libwacom/package.nix

+  # Tests are in the `tests` pass-through derivation because one of them is flaky, frequently causing build failures.
+  # See https://github.com/NixOS/nixpkgs/issues/328140
+  withTests ? false,


I like this and will apply

philiptaron · 2024-08-12T21:14:41Z

pkgs/by-name/li/libwacom/package.nix

  mesonFlags = [
-    testsDisabled
-    "--sysconfdir=/etc"
+    (lib.mesonEnable "tests" withTests)


Suggested change

(lib.mesonEnable "tests" withTests)

(lib.mesonEnable "tests" finalAttrs.finalPackage.doCheck)

philiptaron · 2024-08-12T21:14:49Z

pkgs/by-name/li/libwacom/package.nix

-    testsDisabled
-    "--sysconfdir=/etc"
+    (lib.mesonEnable "tests" withTests)
+    (lib.mesonOption "sysconfdir" "/etc")


philiptaron · 2024-08-12T21:15:07Z

pkgs/by-name/li/libwacom/package.nix


-      mesonFlags = map replaceDisabledWithEnabled prevAttrs.mesonFlags;
+  nativeCheckInputs = lib.optionals withTests [


Suggested change

nativeCheckInputs = lib.optionals withTests [

nativeCheckInputs = lib.optionals finalAttrs.finalPackage.doCheck [

Now passes nixf-tidy

Atemu · 2024-08-12T21:38:37Z

Ooh got a (literally) last minute simplification in even ;)

You did create a merge commit instead of fast-forwarding though; you should rebase that away for Nixpkgs purposes.

philiptaron · 2024-08-12T21:49:05Z

There's a maybe bug about finalAttrs.finalPackage.doCheck vs. finalAttrs.doCheck lingering.

finalAttrs.finalPackage.doCheck is the value of doCheck after it has been processed by stdenv.mkDerivation.

finalAttrs.doCheck is the value of doCheck before it has been processed by stdenv.mkDerivation.

Normally, stdenv.mkDerivation doesn't change this value, so the two are equivalent. If it were to do so, I think it's better to rely on the changed value. What do you think @Atemu?

philiptaron · 2024-08-12T21:49:36Z

Ooh got a (literally) last minute simplification in even ;)

Oh I saw it 😁 I love those with removals.

Atemu · 2024-08-12T22:04:19Z

finalAttrs.finalPackage.doCheck is the value of doCheck after it has been processed by stdenv.mkDerivation.

Not-so-final attrs, huh?

Normally, stdenv.mkDerivation doesn't change this value, so the two are equivalent. If it were to do so, I think it's better to rely on the changed value. What do you think @Atemu?

I think it's fine to keep it as is. doCheck is an argument of/input to mkDerivation, not an output. It shouldn't/cannot change it.

Oh I saw it 😁 I love those with removals.

No, I removed the doCheck conditional on nativeCheckInputs which is redundant.

This adds some extremely helpful and popular encoders in by default: * openjpeg * celt * libwebp * libaom On the `master` branch, closure size for ffmpeg-headless went up 18.5 MiB. ``` $ nix store diff-closures nixpkgs#ffmpeg-headless^bin .#ffmpeg-headless^bin celt: ∅ → 0.11.3, +168.4 KiB ffmpeg-headless: +70.0 KiB giflib: ∅ → 5.2.2, +398.7 KiB lcms2: ∅ → 2.16, +466.2 KiB lerc: ∅ → 4.0.0, +840.2 KiB libaom: ∅ → 3.9.0, +8047.8 KiB libdeflate: ∅ → 1.20, +427.0 KiB libtiff: ∅ → 4.6.0, +655.9 KiB libvmaf: ∅ → 3.0.0, +2665.0 KiB libwebp: ∅ → 1.4.0, +2559.7 KiB openjpeg: ∅ → 2.5.2, +1525.1 KiB zstd: ∅ → 1.5.6, +1158.0 KiB $ nvd diff $(nix build nixpkgs#ffmpeg-headless^bin --print-out-paths --no-link) $(nix build .#ffmpeg-headless^bin --print-out-paths --no-link) <<< /nix/store/4n60lnj3zkjpasd4c56bzhpx2m8lc1sx-ffmpeg-headless-6.1.1-bin >>> /nix/store/884f487w5hac6rs94jq6hq5zqkxdv666-ffmpeg-headless-6.1.1-bin Added packages: [A.] #1 celt 0.11.3 [A.] NixOS#2 giflib 5.2.2 [A.] NixOS#3 lcms2 2.16 [A.] NixOS#4 lerc 4.0.0 [A.] NixOS#5 libaom 3.9.0 [A.] NixOS#6 libdeflate 1.20 [A.] NixOS#7 libtiff 4.6.0 [A.] NixOS#8 libvmaf 3.0.0 [A.] NixOS#9 libwebp 1.4.0 x2 [A.] NixOS#10 openjpeg 2.5.2 [A.] NixOS#11 zstd 1.5.6 Closure size: 66 -> 78 (15 paths added, 3 paths removed, delta +12, disk usage +18.5MiB). ```

Strongly inspired by the forgejo counterpart[1], for the following reasons: * The feature is broken with the current module and crashes on authentication with the following stacktrace (with a PAM service `gitea` added): server # Stack trace of thread 1008: server # #0 0x00007f3116917dfb __nptl_setxid (libc.so.6 + 0x8ddfb) server # #1 0x00007f3116980ae6 setuid (libc.so.6 + 0xf6ae6) server # NixOS#2 0x00007f30cc80f420 _unix_run_helper_binary (pam_unix.so + 0x5420) server # NixOS#3 0x00007f30cc8108c9 _unix_verify_password (pam_unix.so + 0x68c9) server # NixOS#4 0x00007f30cc80e1b5 pam_sm_authenticate (pam_unix.so + 0x41b5) server # NixOS#5 0x00007f3116a84e5b _pam_dispatch (libpam.so.0 + 0x3e5b) server # NixOS#6 0x00007f3116a846a3 pam_authenticate (libpam.so.0 + 0x36a3) server # NixOS#7 0x00000000029b1e7a n/a (.gitea-wrapped + 0x25b1e7a) server # NixOS#8 0x000000000047c7e4 n/a (.gitea-wrapped + 0x7c7e4) server # ELF object binary architecture: AMD x86-64 server # server # [ 42.420827] gitea[897]: pam_unix(gitea:auth): unix_chkpwd abnormal exit: 159 server # [ 42.423142] gitea[897]: pam_unix(gitea:auth): authentication failure; logname= uid=998 euid=998 tty= ruser= rhost= user=snenskek It only worked after turning off multiple sandbox settings and adding `shadow` as supplementary group to `gitea.service`. I'm not willing to maintain additional multiple sandbox settings for different features, especially given that it was probably not used for quite a long time: * There was no PR or bugreport about sandboxing issues related to PAM. * Ever since the module exists, it used the user `gitea`, i.e. it had never read-access to `/etc/shadow`. * Upstream has it disabled by default[2]. If somebody really needs it, it can still be brought back by an overlay updating `tags` accordingly and modifying the systemd service config. [1] 07641a9 [2] https://docs.gitea.com/usage/authentication#pam-pluggable-authentication-module

The first assumption[1] we had was that the `aarch64-unknown-none` target was missing from rustc and that this was the cause for the regression. However, it turns out that the relevant code from `rustc` wasn't used anyways because the Makefile does `--sysroot /dev/null`[2] which prevents rustc from using its own libcore. So luckily we don't have to patch the Rust bootstrap to get aarch64-linux back working[3]. In fact, the Rust part seems broken for both 6.10 and 6.11[4], but I decided to not bother since none of those are longterm versions. So all that's left here is to enable Rust for aarch64-linux because it clearly works[5]. [1] NixOS#315121 (comment) [2] https://lore.kernel.org/all/[email protected]/ [3] Of course I only realized this _after_ I spent some time hacking a rustc patch together 🙃 [4] This broke with error[E0463]: can't find crate for `core` | = note: the `aarch64-unknown-none` target may not be installed = help: consider downloading the target with `rustup target add aarch64-unknown-none` = help: consider building the standard library from source with `cargo build -Zbuild-std` [5] While the build is fine, the VM tests are still panicking, but that's also the case for `kernel-generic` because of a 9p regression: switch_root: can't execute '/nix/store/zv87gw0yxfsslq0mcc35a99k54da9a4z-nixos-system-machine-test/init': Exec format error [ 1.734997] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000100 [ 1.736002] CPU: 0 UID: 0 PID: 1 Comm: switch_root Not tainted 6.12.0-rc1 #1-NixOS [...] Reported as https://lore.kernel.org/all/[email protected]/T/#u

Atemu mentioned this pull request Aug 12, 2024

libwacom{,-surface}: move tests to passthru, move to by-name, nixfmt, update all to 2.12.2 NixOS/nixpkgs#334218

Merged

19 tasks

philiptaron reviewed Aug 12, 2024

View reviewed changes

Atemu force-pushed the issue-328140/tests-to-passthru-tests branch from f88642a to e43286d Compare August 12, 2024 21:34

Atemu added 3 commits August 12, 2024 23:36

libwacom: simplify tests enablement

33df696

libwacom: remove ambiguous with statement

5011fc2

Now passes nixf-tidy

libwacom: use lib.mesonOption

ba96bbb

Atemu force-pushed the issue-328140/tests-to-passthru-tests branch from e43286d to ba96bbb Compare August 12, 2024 21:36

philiptaron approved these changes Aug 12, 2024

View reviewed changes

philiptaron merged this pull request into philiptaron:issue-328140/tests-to-passthru-tests Aug 12, 2024
9 checks passed

Atemu deleted the issue-328140/tests-to-passthru-tests branch August 12, 2024 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libwacom: simplify tests enablement #1

libwacom: simplify tests enablement #1

Atemu commented Aug 12, 2024

philiptaron left a comment

philiptaron Aug 12, 2024

Atemu Aug 12, 2024

philiptaron Aug 12, 2024

Atemu Aug 12, 2024

philiptaron Aug 12, 2024

philiptaron Aug 12, 2024

philiptaron Aug 12, 2024

philiptaron Aug 12, 2024

Atemu commented Aug 12, 2024

philiptaron commented Aug 12, 2024

philiptaron commented Aug 12, 2024

Atemu commented Aug 12, 2024

	tests = libwacom.override { withTests = true; };
	tests = finalAttrs.finalPackage.overrideAttrs (prevAttrs: { doCheck = true; });

	(lib.mesonEnable "tests" withTests)
	(lib.mesonEnable "tests" finalAttrs.finalPackage.doCheck)


		mesonFlags = map replaceDisabledWithEnabled prevAttrs.mesonFlags;
		nativeCheckInputs = lib.optionals withTests [

	nativeCheckInputs = lib.optionals withTests [
	nativeCheckInputs = lib.optionals finalAttrs.finalPackage.doCheck [

libwacom: simplify tests enablement #1

libwacom: simplify tests enablement #1

Conversation

Atemu commented Aug 12, 2024

Description of changes

Things done

philiptaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atemu commented Aug 12, 2024

philiptaron commented Aug 12, 2024

philiptaron commented Aug 12, 2024

Atemu commented Aug 12, 2024