setup-hooks: add auto-patch-pc-hook by marcin-serwin · Pull Request #394610 · NixOS/nixpkgs

marcin-serwin · 2025-03-30T13:30:57Z

patch-pc replaces dependencies in pc files with their absolute paths, auto-patch-pc-hook automatically runs it during fixup phase for all .pc files produced by the package. See #334195 for motivation. Feel free to also comment on the patch-pc code in this PR, I'll tag it as 0.1.0 once I'm sure no more changes are necessary.

I am currently in the process of rebuilding my personal NixOS system on top of the #394607 branch. So far it's going well, the only problematic package is xorgproto which produces .pc files that reference packages depending on xorgproto, creating circular dependency. I've patched them out for now, not sure what's the proper way to address it.

cc @emilazy

Things done

Add a 👍 reaction to pull requests you find important.

emilazy · 2025-03-30T14:25:40Z

cc @alyssais @K900

Thanks so much for working on this!

I think we might want this hook to be in stdenv as an awful lot of packages produce .pc files? But OTOH that does mean another tool in the bootstrap. I guess it will usually fail loudly, though, so perhaps this is fine.

I’d like to request you pick another licence, if possible; CC0 is not really designed for code, and has an explicit disclaimer against granting patent rights that has led to e.g. Fedora rejecting its use for software.

K900 · 2025-03-30T14:30:55Z

I think it makes sense to just put the code for the tool in nixpkgs.

marcin-serwin · 2025-03-30T14:52:43Z

I’d like to request you pick another licence, if possible; CC0 is not really designed for code, and has an explicit disclaimer against granting patent rights that has led to e.g. Fedora rejecting its use for software.

Changed to Unlicense

I think it makes sense to just put the code for the tool in nixpkgs.

Should the files go into pkgs/by-name/pa/patch-pc/ and the src changed to ./ or is there some other procedure for this?

K900 · 2025-03-30T15:01:30Z

I'd put it in a subdirectory, personally, just for convenience.

emilazy · 2025-03-30T15:28:36Z

I don’t think the code needs to be in Nixpkgs; we depend on plenty of tools like patchelf that aren’t. But if it is, we should probably make it MIT. Otherwise if someone copied code from elsewhere in Nixpkgs into the tool it could cause a headache.

K900 · 2025-03-30T16:40:03Z

I thought it was smaller, to be honest, but also, looking at it now... Do we really have to reinvent string handling in C again? Can we do Python or something?

marcin-serwin · 2025-03-30T16:46:13Z

Can we do Python or something?

Python depends on curl, this would make the bootstrap more complicated.

K900 · 2025-03-30T16:47:27Z

We don't need curl if we vendor the sources though?

marcin-serwin · 2025-03-30T16:49:21Z

curl will depend on this package to patch its .pc file. Even with vendored sources this creates patch-pc -> Python -> curl -> patch-pc cycle.

K900 · 2025-03-30T16:50:48Z

Can we just leave this out of some of the bootstrap? Or go with C++ or Rust or something. Hell, at least glib?

marcin-serwin · 2025-03-30T16:55:14Z

Can we just leave this out of some of the bootstrap?

Probably, that's what I meant when I said that it'll make the bootstrap more complicated.

Or go with C++ or Rust or something. Hell, at least glib?

rustc depends on curl, glib has .pc files to patch. C++ is viable I guess.

K900 · 2025-03-30T16:57:46Z

I'm really really sorry if I'm coming on too strong here, but I also really, really don't want to see more C string manipulation code in nixpkgs, or anywhere for that matter. Skipping this for a few bootstrap packages is absolutely worth the maintenance cost savings of not doing it in C.

emilazy · 2025-03-30T17:34:27Z

Probably much of the manual string parsing could be avoided by using the libpkgconf API.

marcin-serwin · 2025-04-01T20:56:16Z

I've rewritten it in python. Turns out that with python3Minimal the bootstrap changes were minimal. I'm still rebuilding my system with this hook, but more than half of my packages have built already so I think that it's safe to say that the hook itself works.

K900 · 2025-04-02T06:51:49Z

Sorry for making you rewrite this! But I absolutely love the new version.

marcin-serwin · 2025-04-15T22:04:35Z

I've managed to build my system atop of #394607, but I had to make some chanages:

I've removed dependency on findutils to simplify bootstrap. It's replaced by more specific globbing which I think covers all the places where the .pc files may be placed.
The hook relies on PKG_CONFIG_PATH being populated which is done by pkg-config-wrapper. I added pkg-config as a dummy propagated build input until switch to pkgconf is complete. After the switch, pkgconf can be propagated and used directly by the hook to avoid passing the binary as a parameter.
When building libreoffice, I've hit "output '[...]' is not allowed to refer to the following paths" error due to the disallowedRequisites present in this derivation. I suspect that after making the .pc requires absolute they are picked as direct dependencies for some derivations. Since .pc files are sometimes present in the out output instead of the dev output the libreoffice picks them up as transitive deps and that's why the error occurs. I've worked around this by switching from disallowedReferences to disallowedReferences which only checks the package itself and does not recurse into deps.
The bootstrap for linux only involved special casing for fetchurl which is localized to few overrides in top-level/all-packages. However, the darwin process is a bit more involved due to code signing – my attempt at fixing it is spread across several files. Is there a better way to do it?

LordGrimmauld

Some more documentation and explanation would be nice.

The changes themselves look fine honestly - so far nothing uses this, but this is quite compact and certainly more maintainable than the C proposal you had.

Feel free to ignore my python nits, my coding style in py is a bit peculiar. But figured i'd bring those up, even if they end up ignored. Your code is very clean. Noone stops learning, discussing the best way to do things can be nice. But i also don't want to turn this to bike shedding hell, so if that ends up too annoying just close those comments please!

LordGrimmauld · 2025-04-17T19:55:56Z

pkgs/top-level/all-packages.nix

nit: autoPatchPcHook might not be very descriptive, autoPatchPkgConfigHook might be easier to understand when looking at any package that would use this

pkgs/build-support/setup-hooks/patch-pc.py

pkgs/top-level/all-packages.nix

LordGrimmauld · 2025-04-17T20:01:31Z

pkgs/top-level/all-packages.nix

Is this actually intended to be a different pkgconf than above propagated pkg-config? Might it make more sense to make a let...in for the whole block and use the same here as in propagation?

Is this actually intended to be a different pkgconf than above propagated pkg-config?

Yes, the pkg-config in propagated build inputs is the freedesktop's implementation which is currently the default in nixpkgs. We can't use it in the patch-pc.py because it does not support the --path flag. I want to avoid packages having both pkg-config and pkgconf in their nativeBuildInputs so I propagate the pkg-config to ensure that its setup hook is registered while the script uses pkgconf. Once nixpkgs switches to pkgconf this can be simplified to propagating pkgconf and the script using the the program from PATH.

LordGrimmauld · 2025-04-17T20:04:57Z

pkgs/build-support/setup-hooks/patch-pc.py

stylistic nit, feel free to ignore:

Suggested change

print(f"Requires{".private" if private else ""}:", ", ".join(requires))

print(f"Requires{".private" if private else ""}: ", end="")

print(*requires, sep=", ")

You could technically also replace ".private" if private else "" with ".private" * private because you are checking for bool parameter, but honestly don't, that would be too deep into python weirdness. But the print is a serious suggestion.

Applied the print suggestion.

LordGrimmauld · 2025-04-17T20:12:04Z

pkgs/build-support/setup-hooks/patch-pc.py

Stylistic nit (feel free to ignore):

Suggested change

requires = (transform_dep_spec(dep) for dep in list_requires(file, private))

requires = map(transform_dep_spec, list_requires(file, private))

We do functional programming in nix, a map might be more appropriate than list comprehension, but i am happy with either.

I don't write a lot of Python but my understanding is that list comprehensions are considered more pythonic than the functional equivalents, e.g., see here.

LordGrimmauld

Thanks for the documentation! Now the need for it is clearer, and when/how to use too. The python code looks clean, and the nix/bash part looks at least manageable. I am not too familiar with stdenv things, my approval shouldn't be sufficient to blindly merge this, but no complaints left here.

K900 · 2025-04-21T14:05:33Z

I think we should enable this by default and have an opt-out in case it breaks things. Still need to review the Python bits closer.

marcin-serwin · 2025-04-21T14:14:50Z

I think we should enable this by default and have an opt-out in case it breaks things. Still need to review the Python bits closer.

How can we do an opt-out that also removes runtime dependencies? Other hooks I've looked at do something like if [ -z $dontRunSomeHook ]; then ... in the bash script but this will not work for bootstrap.

K900 · 2025-04-21T14:31:42Z

Can do something in mkDerivation I guess, but yeah, that might be a problem...

LordGrimmauld · 2025-04-21T14:36:30Z

Maybe stdenv people have better ideas how to do this "properly"? @emilazy boop

LordGrimmauld · 2025-04-21T14:38:40Z

~~One option might be to add the hook to pkgconf.propagatedNativeBuildInputs? It doesn't need to be in all of stdenv i think.~~ Actually no that would get recursion...

nix-owners bot requested a review from Ericson2314 March 30, 2025 13:32

marcin-serwin requested a review from LordGrimmauld March 30, 2025 13:32

marcin-serwin mentioned this pull request Mar 30, 2025

pkg-config: remove requires-private patch #394607

Draft

13 tasks

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from cbc5b33 to 350e8f4 Compare March 30, 2025 13:51

github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 30, 2025

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from 350e8f4 to 2db8959 Compare March 30, 2025 14:18

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from 2db8959 to 0055976 Compare March 30, 2025 14:51

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from aadce28 to a15c363 Compare March 30, 2025 15:18

marcin-serwin marked this pull request as draft March 30, 2025 18:29

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from a15c363 to d92ea30 Compare April 1, 2025 20:54

marcin-serwin marked this pull request as ready for review April 1, 2025 20:54

marcin-serwin changed the title ~~patch-pc: init at 0.1.0; create auto-patch-pc-hook~~ setup-hooks: add auto-patch-pc-hook Apr 1, 2025

github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 1, 2025

marcin-serwin marked this pull request as draft April 8, 2025 21:10

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from d92ea30 to b8929dc Compare April 15, 2025 21:53

marcin-serwin marked this pull request as ready for review April 15, 2025 22:04

LordGrimmauld reviewed Apr 17, 2025

View reviewed changes

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from b8929dc to c7d2020 Compare April 19, 2025 13:01

github-actions bot added 8.has: documentation This PR adds or changes documentation and removed 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 19, 2025

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from c7d2020 to cb180cd Compare April 19, 2025 13:16

nix-owners bot requested review from GetPsyched, fricklerhandwerk and hsjobeki April 19, 2025 13:18

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from cb180cd to fac5a50 Compare April 19, 2025 13:22

setup-hooks: add auto-patch-pc-hook

dbcace1

marcin-serwin force-pushed the push-qyxlqonrzxnu branch from fac5a50 to dbcace1 Compare April 19, 2025 13:25

LordGrimmauld approved these changes Apr 19, 2025

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 19, 2025

Aleksanaa added the 2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off label Apr 21, 2025

LordGrimmauld removed the 2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off label May 28, 2025

LordGrimmauld mentioned this pull request Aug 20, 2025

systemd: remove bash #429972

Merged

13 tasks

nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 18, 2025

marcin-serwin mentioned this pull request Feb 2, 2026

patch-shebangs: optimize bash and add ANSI C implementation #482713

Open

13 tasks

	print(f"Requires{".private" if private else ""}:", ", ".join(requires))
	print(f"Requires{".private" if private else ""}: ", end="")
	print(*requires, sep=", ")

	requires = (transform_dep_spec(dep) for dep in list_requires(file, private))
	requires = map(transform_dep_spec, list_requires(file, private))

Uh oh!

Conversation

marcin-serwin commented Mar 30, 2025

Things done

Uh oh!

emilazy commented Mar 30, 2025

Uh oh!

K900 commented Mar 30, 2025

Uh oh!

marcin-serwin commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

K900 commented Mar 30, 2025

Uh oh!

emilazy commented Mar 30, 2025

Uh oh!

K900 commented Mar 30, 2025

Uh oh!

marcin-serwin commented Mar 30, 2025

Uh oh!

K900 commented Mar 30, 2025

Uh oh!

marcin-serwin commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

K900 commented Mar 30, 2025

Uh oh!

marcin-serwin commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

K900 commented Mar 30, 2025

Uh oh!

emilazy commented Mar 30, 2025

Uh oh!

marcin-serwin commented Apr 1, 2025

Uh oh!

K900 commented Apr 2, 2025

Uh oh!

marcin-serwin commented Apr 15, 2025

Uh oh!

LordGrimmauld left a comment

Choose a reason for hiding this comment

Uh oh!

LordGrimmauld Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

marcin-serwin Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LordGrimmauld Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

marcin-serwin Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

LordGrimmauld Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

marcin-serwin Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

LordGrimmauld Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcin-serwin Apr 19, 2025

Choose a reason for hiding this comment

Uh oh!

LordGrimmauld left a comment

Choose a reason for hiding this comment

Uh oh!

K900 commented Apr 21, 2025

Uh oh!

marcin-serwin commented Apr 21, 2025

Uh oh!

K900 commented Apr 21, 2025

Uh oh!

LordGrimmauld commented Apr 21, 2025

marcin-serwin commented Mar 30, 2025 •

edited

Loading

marcin-serwin commented Mar 30, 2025 •

edited

Loading

marcin-serwin commented Mar 30, 2025 •

edited

Loading

LordGrimmauld Apr 17, 2025 •

edited

Loading

LordGrimmauld commented Apr 21, 2025 •

edited

Loading