defaultPkgConfigPackages: init #212282

roberth · 2023-01-23T17:17:16Z

Description of changes

This is a mapping from pkg-config identifiers to packages, for anyone to use, but most useful for lang2nix tooling.

The initial list is based on cabal2nix from April 2022. It comes with tests to verify that the package actually exposes the expected pkg-config package. I've only had to update the vte package since then, as shown by the running the tests today.

cc @aakropotkin who also maintains a pkg-config mapping
cc @DavHau because probably dream2nix needs this too

Things done

roberth · 2023-01-23T17:17:40Z

@ofborg build tests.defaultPkgConfigPackages

sternenseemann · 2023-01-25T13:28:57Z

doc/languages-frameworks/pkg-config.section.md

+
+ - A [setup hook](#setup-hook-pkg-config) bundled with in the `pkg-config` package, to bring a derivation's declared build inputs into the environment.
+ - The [`validatePkgConfig` setup hook](https://nixos.org/manual/nixpkgs/stable/#validatepkgconfig), for packages that provide pkg-config modules.
+ - The `pkg-configPackages` package set: a set of aliases, named after the modules they provide. This is meant to be used by language-to-nix integrations. Hand-written packages should use the normal Nixpkgs attribute name instead.


The policy for hand written packages is of course hard to enforce, so it the question is if we should have a non-enforceable policy?

Hmm? What policy? I just see documentation of Nixpkgs' interfaces.

Hand-written packages should use the normal Nixpkgs attribute name instead

Ah, well I wouldn't sweat it. I think this supposed to be more "we're not changing the norms at this time" than "you must do it this way...".

A should, not a must. I prefer breaking rules over making rules anyway. Gee that sounds cheesy.

sternenseemann · 2023-01-25T14:29:17Z

pkgs/test/default.nix

+      # in order to filter out the unsupported packages without throwing any errors
+      # tryEval would be too fragile, masking different problems as if they're
+      # unsupported platform problems.
+      allPkgs = import ../top-level {


Suggested change

allPkgs = import ../top-level {

allPkgs = import pkgs.path {

That risks copying a path to the store. pkgs.path is too complicated for me; most imports happen by path expression, which is known to work well.

sternenseemann · 2023-01-25T14:35:13Z

pkgs/test/pkg-config-packages.nix

+    # Make sure licensing info etc is preserved, as this is a concern for e.g. cache.nixos.org,
+    # as hydra can't check this meta info in dependencies.
+    # The test itself is just Nixpkgs, with MIT license.
+    // builtins.intersectAttrs


I'd just update the meta of the source package instead which seems more comprehensible – hydra will behave the same for both (i.e. pkg.meta // { description = … ; }). The new meta set is run through mkDerivation anyways.

It isn't equivalent to the original package though, so I'd rather be conservative in terms of what we inherit. For example, the description would be wrong. For instance, your suggestion would copy the longDescription, which is wrong.

sternenseemann · 2023-01-25T14:36:27Z

cc @Ericson2314 who has also been working towards a (different?) solution of this problem.

Edit: See https://github.com/nixpkgs-architecture/issues/issues/14.

Ericson2314 · 2023-01-25T14:46:18Z

I didn't work any implementation yet, so this is great!

Can we add tests / meta to the individual packages saying what pkg-config they provide? Then, as a prelude to generating the global mapping, we can at least assert that it matches what the individual packages say.

Ericson2314 · 2023-01-25T14:53:57Z

pkgs/development/libraries/zlib/default.nix

+    tests = {
+      # NB: this may test a different variant of zlib than the package that
+      #     carries this `tests` attribute, due to overriding.
+      pkg-configPackages = tests.pkg-configPackages.zlib;


I would rather this call makePkgConfigTest, and then tests.pkg-configPackages gathers these to run all of them. That is I think better layering, but concretely it allows the tests for individual packages to be built regardless of the package set in question.

The point is exactly to violate layering, so that ofborg will test the relevant bit of pkg-configPackages when it's time to do so. I think this is the best we can achieve with the tooling we have, if we put in the effort.

Maybe we should just ignore this idea and rely on asynchronous testing of the whole pkg-configPackages on hydra.

Yeah I don't know the details of ofborg, but I thought the overriding was just for the top-level tests? I would think if the individual package is an unsupported systems, then ofborg would just skip the test too, which is fine?

I've factored out the tester into a properly tested and documented tester.

Ericson2314 · 2023-01-25T15:00:05Z

pkgs/top-level/pkg-config-packages.nix

@@ -0,0 +1,202 @@
+/* A set of aliases to be used in generated expressions.
+
+   In case of ambiguity, this will pick a sensible default.


I think we should provide all packages that match, something like one of:

pkgConfigName = [ pkg0 pkg1 ]; pkgConfigName = { inherit pkg0 pkg1; };

A key idea of pkg-config is that it tracks interfaces not implementations.

My intent is for this to be a slightly curated set of packages, providing defaults for lang2nix integrations. Adding multiple versions / variants / ... here will add a lot more churn and will confuse lang2nix integrations that don't have a means to pick one. I feel like this would be more suitable for a version 2.

How about I call this package set defaultPkgConfigPackages instead? That will leave room for a fancier implementation with a nice name.

@roberth sounds good! The complete one can be autogenerated.

Fill pkg-configPackages using a script #212290

The list data is in a .json now that can be evolved if needed. It's already quite extensible because it uses records wherever it can; top-level and in each module. It also has a top-level version field just in case.

roberth · 2023-01-25T16:22:27Z

@Ericson2314

Can we add tests / meta to the individual packages saying what pkg-config they provide?

A meta attribute should not be part of the derivation.

Then, as a prelude to generating the global mapping, we can at least assert that it matches what the individual packages say.

So then it should be part of the derivation, or we should generate a tests attribute for it.

Doing this kind of automatic stuff isn't really feasible in the current architecture, as hooks can, by nature, not affect the attribute set. If packages were modules, this would have a fantastic ui:

imports = [ pkgModules.pkg-config ];
pkg-config.export = [ "nix-expr" "nix-store" ];

pkgModules.pkg-config can then touch all the attributes it needs to, whether that's the package meta, tests, or adding an installCheck hook in the derivation.

Alternatively, we'd have to add this logic to mkDerivation itself, which I feel would make things worse.

Doing it in this PR would blow up the scope. It's already tested, just differently, so I don't think it should be a blocker.

Ericson2314 · 2023-01-25T16:33:10Z

@roberth Maybe I am confused, I thought yes the derivation doesn't get access to meta, but self in tests = ...self... still can via #119942?

Ericson2314 · 2023-01-25T16:35:21Z

We'd have

meta.pkg-config = [ "nix-store" "nix-expr" ];
tests.pkg-config = makePkgConfigTest self;

and makePkgConfigTest can pull out meta.pkg-config.

roberth · 2023-01-25T17:39:36Z

#119942

We'd have
meta.pkg-config = [ "nix-store" "nix-expr" ];
tests.pkg-config = makePkgConfigTest self;
and makePkgConfigTest can pull out meta.pkg-config.

That looks like it'd work, yes. It's a bit more manual than I'd like, but it's feasible, at least for the places where self is "available". That's probably most C/C++ packages anyway, because it's only the language-specific mkDerivation wrappers of which some don't support it yet.

Nonetheless, I'd like to leave that as future work. My goal here is really to get the ball rolling. I can't spend a lot of time on this.

roberth · 2023-01-25T17:45:30Z

Both ways of testing are independent, have different goals despite their similarity, and that we should eventually do both.

test the package set pkgs.defaultPkgConfig.nix-expr refers to a package that exposes nix-expr.pc
test a package let foolib = zlib.overrideAttrs xyz; in foolib exposes zlib.pc.

There's overlap in part of the implementation, but they are different subjects.

Ericson2314

I am happy to do that as follow-up then.

sternenseemann · 2023-01-26T13:27:00Z

Maybe we should take a step back and deliberate if a package set is the best option? In what situations do we actually need it?

If we have a lookup table lang2nix tools can use, that would work, too, and that generated expressions would be more overrideable. What bugs me right now that cabal2nix would with this solution generate an expression taking defaultPkgConfigPackages as an input for zlib. This is very annoying to override, not easy to introspect and assymetrical, since as soon as pkg-config would not be used (instead, e.g. extra-libraries), zlib would appear in the argument list again.

roberth · 2023-01-26T13:49:23Z

If you want json, the file can be converted and loaded with some functions. The package set and the generated tests are useful components of the json solution as well.
The tester can be extracted into a pkgs.testers tester if you're concerned about passthru tests. I've already removed the zlib example.

DavHau · 2023-01-28T07:14:00Z

If you want json, the file can be converted and loaded with some functions

I think that would be a good change. Having it as a .nix function restricts usage unnecessarily. Examples:

lang2nix pipelines aren't necessarily implemented in pure nix. They might need to access the data from another language, which is hard, if the data can only be accessed by evaluating .nix code.
Overridability: Hiding information behind functions seems to be a cause of anit-patterns like override, overrideAttrs. If the information is a simple mapping, let's expose it as such. This can only make it more flexible and doesn't cost us much.

roberth · 2023-01-29T08:43:39Z

There's overlap in part of the implementation, but they are different subjects.

The overlapped part is now testers.hasPkgConfigModule.

json

Done.

@ofborg build tests.testers.hasPkgConfigModule
@ofborg build tests.pkg-config.defaultPkgConfigPackages.ncurses
@ofborg build defaultPkgConfigPackages.libpng

By allowing null, we allow code to avoid filterAttrs, improving laziness in real world use cases. Specifically, this strategy prevents infinite recursion errors, performance issues and possibly other errors that are unrelated to the user's code.

K900 · 2023-01-30T17:40:30Z

This broke Hydra: https://hydra.nixos.org/build/207340888/nixlog/114

vcunat · 2023-01-30T22:10:30Z

Yes, f192e96 in particular. Channel blocked again because of that.

vcunat · 2023-01-30T22:13:59Z

Ah, it was reverted already in PR #213615

Ericson2314 · 2023-02-03T04:57:41Z

#214304

nixos-discourse · 2023-02-07T04:21:46Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/adding-pkg-config-metadata-to-nixpkgs-packages/25281/1

SuperSandro2000 · 2023-02-07T12:19:33Z

pkgs/build-support/testers/hasPkgConfigModule/tester.nix

+    r=$?
+    set -e
+    if [[ $r = 0 ]]; then
+      echo "✅ pkg-config module $moduleName exists and has version $version"


First time I see some unicode in setup tooks and I am surprised that no one complained yet.

roberth force-pushed the pkg-config-packages branch from 061efe8 to 6588f93 Compare January 23, 2023 17:19

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 23, 2023

roberth force-pushed the pkg-config-packages branch from 83d6170 to 139a681 Compare January 23, 2023 18:20

roberth mentioned this pull request Jan 23, 2023

Fill pkg-configPackages using a script #212290

Open

roberth force-pushed the pkg-config-packages branch from f4cde1b to 77d403e Compare January 25, 2023 11:09

github-actions bot added the 8.has: documentation label Jan 25, 2023

roberth mentioned this pull request Jan 25, 2023

Get pkg-config dependencies from pkg-configPackages NixOS/cabal2nix#594

Open

2 tasks

ofborg bot added the ofborg-internal-error label Jan 25, 2023

sternenseemann reviewed Jan 25, 2023

View reviewed changes

sternenseemann requested a review from Ericson2314 January 25, 2023 14:36

Ericson2314 reviewed Jan 25, 2023

View reviewed changes

roberth changed the title ~~pkg-configPackages: init~~ defaultPkgConfigPackages: init Jan 25, 2023

This was referenced Jan 25, 2023

Idea: declare pkg-config dependencies but only override Nixpkgs-style package names DavHau/drv-parts#15

Open

Idea: a pkg-config module DavHau/drv-parts#16

Open

Ericson2314 approved these changes Jan 25, 2023

View reviewed changes

pkg-configPackages: init

7790eaf

ofborg bot added 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 29, 2023

Ericson2314 approved these changes Jan 29, 2023

View reviewed changes

ofborg bot added 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jan 29, 2023

roberth added 4 commits January 30, 2023 00:35

lib.hydraJob: Tolerate null

473ac96

By allowing null, we allow code to avoid filterAttrs, improving laziness in real world use cases. Specifically, this strategy prevents infinite recursion errors, performance issues and possibly other errors that are unrelated to the user's code.

tests.defaultPkgConfigPackages: Add recurseIntoAttrs

f192e96

testers.hasPkgConfigModule: Extract and add tests, docs

b6bec17

defaultPkgConfigPackages: Add dontRecurseIntoAttrs

d8a2178

roberth force-pushed the pkg-config-packages branch from 3196db4 to d8a2178 Compare January 29, 2023 23:38

roberth marked this pull request as ready for review January 30, 2023 00:10

roberth requested review from edolstra, nbp and infinisil as code owners January 30, 2023 00:10

roberth merged commit 663c41a into NixOS:master Jan 30, 2023

cole-h removed the ofborg-internal-error label Jan 30, 2023

This was referenced Jan 31, 2023

Run the defaultPkgConfigPackages tests on hydra #213784

Merged

Convince upstream packages to use pkg-config #107647

Open

SuperSandro2000 reviewed Feb 7, 2023

View reviewed changes

infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023

		@@ -0,0 +1,202 @@
		/* A set of aliases to be used in generated expressions.

		In case of ambiguity, this will pick a sensible default.

defaultPkgConfigPackages: init #212282

defaultPkgConfigPackages: init #212282

Conversation

roberth commented Jan 23, 2023 • edited Loading

Description of changes

Things done

roberth commented Jan 23, 2023 • edited Loading

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

sternenseemann commented Jan 25, 2023 • edited Loading

Ericson2314 commented Jan 25, 2023 • edited Loading

Ericson2314 Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth Jan 25, 2023 • edited Loading

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

Ericson2314 Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberth commented Jan 25, 2023

Ericson2314 commented Jan 25, 2023

Ericson2314 commented Jan 25, 2023

roberth commented Jan 25, 2023

roberth commented Jan 25, 2023 • edited Loading

Ericson2314 left a comment

Choose a reason for hiding this comment

sternenseemann commented Jan 26, 2023 • edited Loading

roberth commented Jan 26, 2023

DavHau commented Jan 28, 2023

roberth commented Jan 29, 2023 • edited Loading

K900 commented Jan 30, 2023

vcunat commented Jan 30, 2023

vcunat commented Jan 30, 2023

Ericson2314 commented Feb 3, 2023

nixos-discourse commented Feb 7, 2023

Choose a reason for hiding this comment

roberth commented Jan 23, 2023 •

edited

Loading

roberth commented Jan 23, 2023 •

edited

Loading

sternenseemann commented Jan 25, 2023 •

edited

Loading

Ericson2314 commented Jan 25, 2023 •

edited

Loading

Ericson2314 Jan 25, 2023 •

edited

Loading

roberth Jan 25, 2023 •

edited

Loading

Ericson2314 Jan 25, 2023 •

edited

Loading

roberth commented Jan 25, 2023 •

edited

Loading

sternenseemann commented Jan 26, 2023 •

edited

Loading

roberth commented Jan 29, 2023 •

edited

Loading