Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formats.libconfig: fix unstable store path dependency in test #264581

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

ckiee
Copy link
Member

@ckiee ckiee commented Oct 31, 2023

Description of changes

Previously, this test would verify a writeText file with a constant content is at a precise store path, but this is not actually the case and the store path has changed maybe twice since the original PR was started (#246115), the latest time being after it was merged.

We now placehold the store path in expected.txt and substitute it just before we run the diff, alleviating the problem.

Fixes #246115 (comment), a nixpkgs-unstable regression.

cc @vcunat @h7x4

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/)
  • 23.11 Release Notes (or backporting 23.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.

@ckiee ckiee force-pushed the libconfig-fix-unstable-store-paths branch from 689e2e4 to 849b643 Compare October 31, 2023 14:38
@h7x4
Copy link
Member

h7x4 commented Oct 31, 2023

So sorry about this 😥

Previously, this test would verify a writeText file with a constant
content is at a precise store path, but this is not actually the case
and the store path has changed maybe twice since the original
"pkgs.formats: Add libconfig format generator" PR was started, the
latest time being after it was merged.

We now placehold the store path in expected.txt and substitute it just
before we run the diff, alleviating the problem.
@ckiee ckiee force-pushed the libconfig-fix-unstable-store-paths branch from 849b643 to faf4e7b Compare October 31, 2023 14:51
@ckiee
Copy link
Member Author

ckiee commented Oct 31, 2023

So, it turns out the original PR was just missing an outputHash as it was supposed to be a FOD and you need all three attrs (which could be clearer in the docs).

Still, a FOD in a test is a bit scary since it gets cached in the Nix store and keyed by the outputHash, so I think this is the way to go even if it is a bit more roundabout.

@infinisil
Copy link
Member

@h7x4 @ckiee Could you add yourself as code owners of pkgs/pkgs-lib/formats/libconfig? :)

@ckiee
Copy link
Member Author

ckiee commented Oct 31, 2023

@infinisil done, split into #264638 :)

@vcunat vcunat merged commit de4eef2 into NixOS:master Nov 1, 2023
18 checks passed
@vcunat
Copy link
Member

vcunat commented Nov 1, 2023

I only verified that the test builds for me, so that channels get unblocked. It sounds like some of you did actually check that the tests really do what they're meant to do.

@ckiee
Copy link
Member Author

ckiee commented Nov 1, 2023

Mhm. We trade off a bit of coverage for not getting another problem with this bit of the test.

@h7x4 h7x4 mentioned this pull request Jan 22, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants