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

testers.hasPkgConfigModules: add optional version check #307770

Merged
merged 6 commits into from
May 3, 2024

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Apr 29, 2024

Description of changes

  • testers: add pkgConfigModulesVersions, which tests that pkgconf modules' version metadata match the package's
  • miniz: use the new tester
  • doc: mention the new tester in pkg-config section

Not done: making the new tester run as part of testMetaPkgConfig, as the new test fails on some drvs.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nixpkgs-review.
  • 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@nbraud
Copy link
Contributor Author

nbraud commented Apr 29, 2024

Not sure how I feel about the tester's name, there probably are better choices

@mkg20001
Copy link
Member

would it make sense to put both tests into one and then have a flag to enable/disable checking version?

@roberth
Copy link
Member

roberth commented Apr 29, 2024

I think it'd be fine to put the check in the existing function - sort of; does need a rename then, but that's all.
They're both about pkg-config, both check a package, and both checks run in the same environment (e.g. buildInputs).

Usually, small functions are good, but derivations do have some overhead, such as extra substituter queries, so I try to factor that in to my decisions, although I'm probably not very consistent about that anyway. Just my two cents.

@nbraud
Copy link
Contributor Author

nbraud commented Apr 29, 2024

I think it'd be fine to put the check in the existing function - sort of; does need a rename then, but that's all.

Which existing function? There's both hasPkgConfigModules, and testMetaPkgConfig which wraps the former in another layer of runCommand for some reason.

Usually, small functions are good, but derivations do have some overhead, such as extra substituter queries.

Absolutely. My preference would also be to have that in testMetaPkgConfig... but there exist drvs like mir which already use the tester and would fail the new check.
I guess it would be possible to put it in hasPkgConfigModules, since it has an attrset parameter.

@nbraud nbraud changed the title testers.pkgConfigModulesVersions: init testers.hasPkgConfigModules: add optional version check Apr 29, 2024
@nbraud
Copy link
Contributor Author

nbraud commented Apr 29, 2024

@roberth I made the check part of testers.hasPkgConfigModules instead, and updated the example in the nixpkgs documentation to use that rather than testMetaPkgConfig. Any thoughts on this?

In principle, I guess the test could be made part of testMetaPkgConfig, using a passthru attribute on the drv to control whether to enforce consistency of version numbers. That seems uglier though, both in terms of API and efficiency.

@nbraud
Copy link
Contributor Author

nbraud commented Apr 29, 2024

Fixed corner case where package has no version attribute

@nbraud
Copy link
Contributor Author

nbraud commented Apr 29, 2024

Result of nixpkgs-review pr 307770 run on x86_64-linux 1

@nbraud nbraud marked this pull request as ready for review April 29, 2024 19:47
@roberth
Copy link
Member

roberth commented Apr 30, 2024

Interface and implementation look good.
Could you add test cases to pkgs/build-support/testers/hasPkgConfigModules/tests.nix?

@nbraud
Copy link
Contributor Author

nbraud commented Apr 30, 2024

Could you add test cases to pkgs/build-support/testers/hasPkgConfigModules/tests.nix?

Done, and thanks: the zlib-does-not-have-ylib test made me notice a very odd bug:

a=0
((a++))  # Increments a and returns 1, which causes the script to exit under `set -e`
((a+=1)) # Increments a but does not return the result, does not cause an error

PS: Sorry, forgot to git pfusch first

@nbraud
Copy link
Contributor Author

nbraud commented Apr 30, 2024

Sorry, I had forgotten to rebase the fixup! commit away. Fixed, introducing no change

@nbraud
Copy link
Contributor Author

nbraud commented May 1, 2024

@roberth Could you confirm this is OK to merge for you, now there are tests?

@nbraud nbraud merged commit a817fda into NixOS:master May 3, 2024
25 checks passed
@nbraud nbraud deleted the testers/pkgConfigVersion branch May 3, 2024 20:19
@roberth
Copy link
Member

roberth commented May 4, 2024

Awesome, thanks!

@OPNA2608 OPNA2608 mentioned this pull request May 8, 2024
13 tasks
@mweinelt
Copy link
Member

mweinelt commented May 9, 2024

Bisect indicates this commit broke python3.tests.pkg-config (for all python versions).

1e9d263dd71d0b42baa8b0d6699864af9fbb669f is the first bad commit
commit 1e9d263dd71d0b42baa8b0d6699864af9fbb669f
Author: nicoo <nicoo@mur.at>
Date:   Mon Apr 29 16:33:54 2024 +0000

    testers.hasPkgConfigModules: Optionally check each module's version

 .../testers/hasPkgConfigModules/tester.nix         | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
 > checking pkg-config module python3 in /nix/store/m51f0kclg01xdc0mjxqs5cpyaxqbqhg5-python3-3.12.3
 > ❌ pkg-config module python3 exists and has version 3.12 when 3.12.3 was expected
 > 1 version mismatches

@roberth
Copy link
Member

roberth commented May 10, 2024

@mweinelt fix in #310535. Sorry for the oversight.

nbraud added a commit to nbraud/nixpkgs that referenced this pull request May 12, 2024
Shell script is currently buggy and effectively ignores the value, always enforcing version match:
  NixOS#307770 (comment)
nbraud added a commit to nbraud/nixpkgs that referenced this pull request May 12, 2024
Broke tests, as the version check was effectively always enabled:
  NixOS#307770 (comment)
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.

None yet

5 participants