Skip to content

nixos/systemd: Add package version tests to integration tests#443216

Merged
LordGrimmauld merged 1 commit intoNixOS:masterfrom
jpds:nixos-tests-systemd-version-check
Sep 18, 2025
Merged

nixos/systemd: Add package version tests to integration tests#443216
LordGrimmauld merged 1 commit intoNixOS:masterfrom
jpds:nixos-tests-systemd-version-check

Conversation

@jpds
Copy link
Contributor

@jpds jpds commented Sep 15, 2025

Adds a package version test to the systemd integration tests.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Sep 15, 2025
@jpds jpds requested a review from Ma27 September 15, 2025 17:47
@nixpkgs-ci nixpkgs-ci bot removed the 9.needs: reviewer This PR currently has no reviewers requested and needs attention. label Sep 15, 2025
@Ma27 Ma27 requested a review from a team September 16, 2025 08:02
Copy link
Contributor

@LordGrimmauld LordGrimmauld left a comment

Choose a reason for hiding this comment

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

i am a little confused, why is one an assert in and the other a succeed(" | grep") ? This feels more inconsistent than it could be.

@jpds
Copy link
Contributor Author

jpds commented Sep 16, 2025

@LordGrimmauld It might just be a preference thing but personally I prefer using assert for checking command outputs (which is also what the rest of the tests in this file do).

@LordGrimmauld
Copy link
Contributor

@LordGrimmauld It might just be a preference thing but personally I prefer using assert for checking command outputs (which is also what the rest of the tests in this file do).

sure, but why not make both of them assert then, why test one with grep ?

@jpds
Copy link
Contributor Author

jpds commented Sep 17, 2025

Because grep is what's normally used for waiting and then checking log files.

@LordGrimmauld
Copy link
Contributor

but you are grepping through stdout, not through a log file. I don't see the point still.

@jpds
Copy link
Contributor Author

jpds commented Sep 17, 2025

@LordGrimmauld This is pretty much the established pattern for using journalctl in nixos/tests/.

Copy link
Contributor

@LordGrimmauld LordGrimmauld left a comment

Choose a reason for hiding this comment

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

Maybe, but in that case journalctl supports a --grep flag (and you'll probably want to add regex escapes, the version string will contain . which is a special thing in regex).

@jpds jpds force-pushed the nixos-tests-systemd-version-check branch from 7364040 to 738cb93 Compare September 17, 2025 09:40
Copy link
Contributor

@LordGrimmauld LordGrimmauld left a comment

Choose a reason for hiding this comment

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

Tested, the "happy path" works, but if the version is broken the test will stall.

@jpds jpds force-pushed the nixos-tests-systemd-version-check branch from 738cb93 to 2b80bf0 Compare September 17, 2025 10:56
@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 17, 2025
@LordGrimmauld LordGrimmauld added this pull request to the merge queue Sep 18, 2025
Merged via the queue into NixOS:master with commit c12e462 Sep 18, 2025
27 checks passed
@ElvishJerricco
Copy link
Contributor

Wait I don't understand; why do we want this? It's not like there's any risk of a different version leaking in somehow, and this just makes it more annoying to bisect systemd bugs.

@LordGrimmauld
Copy link
Contributor

LordGrimmauld commented Sep 19, 2025

There is scenarios in which this breaks. It just broke for me yesterday: The git merge between #442587 and #427968 will not report conflicts around the version field, yet it would fail to actually bump the version of systemd. However, because the hash would be bumped, in case of a cached source systemd would still build with the new source. And this test would catch that error, i believe/hope.
That said, we do have (lib.mesonOption "version-tag" finalAttrs.version), so we might be compiling in what we have anyways. Would need to do some more testing on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants