testers.hasPkgConfigModules: fix bug in versionCheck handling

[nbraud]

Description of changes

• add testcase for the bug reported in testers.hasPkgConfigModules: add optional version check #307770 (comment)
• fix bug

Alternative to #310535

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
• 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
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[nbraud]

@roberth Sorry, it was easier to put together an alternative PR, than figure out how to make it through Github's “suggestions” features.

In short, the bug was entirely caused by the broken conditional @OPNA2608 pointed out, and I don't think we need to do anything more complicated around it.

Apologies for the breakage, I forgot to include in the o.g. PR a test for the case where versionCheck is unspecified

[nbraud]

Result of nixpkgs-review pr 311069 run on x86_64-linux 1
PS: right, nixpkgs-review does not run tests 😓

[nbraud]

I manually checked that python3.tests.pkg-config builds fine now:

❯ nix-build -A python3.tests.pkg-config
[...]
checking pkg-config module python3 in /nix/store/lpi16513bai8kg2bd841745vzk72475x-python3-3.11.9
❌ pkg-config module python3 exists and has version 3.11 when 3.11.9 was expected
[...]
/nix/store/qwvjxvsxf2ylbwbd4izzfnzyjj5gbcaw-check-meta-pkg-config-modules-for-python3-3.11.9

Ideally we'd replace the ❌ with something like ⚠ when versionCheck is false (or unset) ... and I just realised I know how to do it 😅

[roberth]

Good idea to just open a PR. I like your idea, but it needs a bit of work.

I didn't bother with a warning, but if we're going to warn, we should be careful not to cause warning fatigue.
Let's consider the possible combinations:

• versionCheck == true:
  • ok: report ok
  • mismatch: report error
• versionCheck == false:
  • Caller has decided that they don't want it.
  • mismatch: Don't cause warning fatigue. Don't report anything.
  • ok: It may seem better to report, but probably the caller has a good reason to disable the check; maybe the version is unreliable instead of always incorrect
• unset:
  • ok: Caller should set it to true. Tell them to enable the check?
  • mismatch: Caller should provide the expected version or disable the check, and we can tell in a message.

What do you think?

[nbraud]

I didn't bother with a warning, but if we're going to warn, we should be careful not to cause warning fatigue.

Not sure what you mean, I fixed the extent conditional but didn't add a new warning?
Or did you mean in the tester's log output, rather than a nix-level warning

Right now, callers won't normally see the version-mismatch messages because they are only part of the tester's build log, and not displayed unless the test fails. I added 43efaaa to make it clear that those version mismatches are not test failures (unless versionCheck is true)

I had some thoughs how to gently push callers towards using versionCheck, but that seems out-of-scope for this bugfix PR... and for the 24.05 release cycle TBH

[roberth]

Or did you mean in the tester's log output

That's what I meant, yeah.

callers won't normally see the version-mismatch messages because they are only part of the tester's build log,

Users of the package won't, but maintainers and contributors to the package will, and they won't easily understand what's going on.

I had some thoughs how to gently push callers towards using versionCheck, but that seems out-of-scope for this bugfix PR... and for the 24.05 release cycle TBH

Interesting. I agree that we should first fix the error behavior, but I don't think we should trade it for a release that causes warning fatigue. Maybe remove the message altogether when the version check is disabled? Then we can merge that and add a helpful message without hurry.

[roberth]

To make my suggestion more concrete, from #311069 (comment)

This is a little silly and untested, but I guess it gets the intended behavior across, fwiw.

Suggested change

	echo "${if versionCheck then "❌" else "⚠"} pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"
	((versionMismatch+=1))
	${if versionCheck
	then ''
	echo "❌ pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"
	((versionMismatch+=1))
	''
	# TODO: figure out the UX for enabling the version check; see https://github.com/NixOS/nixpkgs/pull/311069
	else ''
	echo "✅ pkg-config module $moduleName exists and has version $moduleVersion"
	''}

[roberth]

It's tempting to add more info to the message, but that will just serve to distract whoever reads it, if we don't explain it, which is out of scope for this PR.

[nbraud]

I'd prefer more-uniform code, rather than more Bash codegen through text templating; would something like this be fine with you?

Suggested change

	echo "${if versionCheck then "❌" else "⚠"} pkg-config module $moduleName exists and has version $moduleVersion when $version was expected"
	((versionMismatch+=1))
	echo "${if versionCheck then "❌" else "ℹ️"} pkg-config module $moduleName exists at version $moduleVersion != $version (drv version)"
	((versionMismatch+=1))

The wording is more neutral, so it seems like a normal info message unless versionCheck
(the versionMismatch counter is unused unless versionCheck, but it's also easier to leave it in)

[nbraud]

I think it's useful to be notified regardless that some modules don't match the drv version number (for whatever reason)
If not, there's a much simpler change: moving the [[ -z "$versionCheck" ]] condition

[nbraud]

callers won't normally see the version-mismatch messages because they are only part of the tester's build log,

Users of the package won't, but maintainers and contributors to the package will, and they won't easily understand what's going on.
[...]
Interesting. I agree that we should first fix the error behavior, but I don't think we should trade it for a release that causes warning fatigue. Maybe remove the message altogether when the version check is disabled? Then we can merge that and add a helpful message without hurry.

I find it odd to talk about “warning fatigue” in that context, since it's usually meant for warnings emitted in nix evaluation.

Regardless, I feel the issue is somewhat overstated, since the message is only ever visible when explicitely looking at the build log of foo.tests.pkg-config. It's not visible when evaluating the drv, or looking at the drv's build log.

In any case, we could easily just not emit the warning at all unless versionCheck if that's a blocker

[roberth]

LGTM if you could rebase away the fixup.

I find it odd to talk about “warning fatigue” in that context, since it's usually meant for warnings emitted in nix evaluation.

It's a concept that's applicable to all warnings. Eval warnings can be more visible, so it's worse there. Still bad in build logs though.

In any case, we could easily just not emit the warning at all unless versionCheck if that's a blocker

Let's just fix it first. We can get into friendly nudging later.

[roberth]

Thanks!

[nbraud]

Thanks for the merge, @roberth, and sorry for the lack of reply: I was out of the country for the RIPE88 meeting