Skip to content

glycin-loaders: replace glycinPathsPatch with setup hook#364572

Closed
pluiedev wants to merge 5 commits intoNixOS:masterfrom
pluiedev:push-krttwlnxvwww
Closed

glycin-loaders: replace glycinPathsPatch with setup hook#364572
pluiedev wants to merge 5 commits intoNixOS:masterfrom
pluiedev:push-krttwlnxvwww

Conversation

@pluiedev
Copy link
Member

glycinPathsPatch doesn't work with apps that use rustPlatform.cargoSetupHook since it assumes a setup where snapshots of GNOME sources already contain vendored Rust dependencies under vendor/.

patchHook instead provides a much more flexible approach that uses the $cargoDepsCopy variable provided by cargoSetupHook, which points to the local, modifiable copy of Cargo dependencies, to patch src/sandbox.rs just like glycinPathsPatch. If $cargoDepsCopy is not found, then the old behavior is used where Cargo dependencies are assumed to be under vendor/.

The patchHook also consolidates the various checksum patches and additions to gappsWrapperArgs, making packages that use glycin much more DRY. Hopefully, new packages using glycin would be able to simply add glycin-loaders.patchHook to their nativeBuildInputs, and have it work OOTB.

glycin-loaders and its dependents are also reformatted, cleaned up and brought more inline with modern standards and conventions.

Closes #364390

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@pluiedev pluiedev force-pushed the push-krttwlnxvwww branch 2 times, most recently from 4d52b25 to 461f800 Compare December 12, 2024 12:16
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 12, 2024
@pluiedev pluiedev added the 6.topic: GNOME GNOME desktop environment and its underlying platform label Dec 12, 2024
@pluiedev pluiedev mentioned this pull request Dec 12, 2024
13 tasks
@github-actions github-actions bot removed the 6.topic: GNOME GNOME desktop environment and its underlying platform label Dec 12, 2024
@pluiedev pluiedev requested a review from getchoo December 12, 2024 17:29
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Dec 13, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 31, 2024
@nixpkgs-ci nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 25, 2025
@magneticflux- magneticflux- mentioned this pull request Aug 16, 2025
13 tasks
@getchoo getchoo force-pushed the push-krttwlnxvwww branch from 20bdac7 to cc54455 Compare August 21, 2025 00:16
@getchoo getchoo force-pushed the push-krttwlnxvwww branch from cc54455 to 94887da Compare August 21, 2025 00:17
@getchoo getchoo changed the title glycin-loaders: replace glycinPathsPatch with patchHook, reformat & cleanup glycin-loaders: replace glycinPathsPatch with setup hook Aug 21, 2025
@getchoo getchoo removed 2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Aug 21, 2025
@getchoo
Copy link
Member

getchoo commented Aug 21, 2025

After talking with @pluiedev on Discord, I've fixed the merge conflicts, tidied up the hook itself, and made it act more like similar hooks (gdk-pixbuf for example). There's some nice docs now too

Built against Loupe and all seems well 👍

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 6.topic: GNOME GNOME desktop environment and its underlying platform labels Aug 21, 2025
@nixpkgs-ci nixpkgs-ci bot added the 8.has: documentation This PR adds or changes documentation label Aug 21, 2025
@getchoo getchoo force-pushed the push-krttwlnxvwww branch from 94887da to f21defe Compare August 21, 2025 00:36
pluiedev and others added 4 commits August 21, 2025 02:36
`glycinPathsPatch` doesn't work with apps that use `rustPlatform.cargoSetupHook` since it assumes a
setup where snapshots of GNOME sources already contain vendored Rust dependencies under `vendor/`.

The setup hook instead provides a much more flexible approach that uses
the `$cargoDepsCopy` variable provided by `cargoSetupHook`, which points to the local, modifiable
copy of Cargo dependencies, to patch `src/sandbox.rs` just like `glycinPathsPatch`. If
`$cargoDepsCopy` is not found, then the old behavior is used where Cargo dependencies are assumed to
be under `vendor/`.

The setup hook also consolidates the various checksum patches and additions to `gappsWrapperArgs`,
making packages that use glycin much more DRY. Hopefully, new packages using glycin would be able to
simply add `glycin-loaders` to their `buildInputs`, and have it work OOTB.

Co-authored-by: Seth Flynn <getchoo@tuta.io>
@getchoo getchoo force-pushed the push-krttwlnxvwww branch from f21defe to 16ab70a Compare August 21, 2025 07:10
@getchoo
Copy link
Member

getchoo commented Aug 21, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 364572
Commit: 16ab70a3858a62446ad29ef24af58b9416290df1


x86_64-linux

✅ 7 packages built:
  • authenticator
  • fractal
  • glycin-loaders
  • glycin-loaders.dev
  • loupe
  • nixpkgs-manual
  • snapshot

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 23, 2025
Copy link
Member

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible.

Can we get another rebase to resolve conflicts please. Hopefully it can be merged before another conflict arises!

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Sep 22, 2025
@jtojnar
Copy link
Member

jtojnar commented Jan 18, 2026

Thank you for the patches and sorry, I somehow missed this.

In the meanwhile we have introduced libglycin package for non-Rust programs and I think it makes more sense to have the setup hook there. But the vendor patching is not relevant for non-Rust programs so I have split it into a separate hook defined in passthru.

I took the liberty to rebase this change set and split it into more commits as part of introduction of libglycin-gtk4 in #481377

@pluiedev
Copy link
Member Author

Superseded by #481377

@pluiedev pluiedev closed this Jan 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: GNOME GNOME desktop environment and its underlying platform 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

glycin-loaders.glycinPathsPatch does not work with fetchCargoVendor

6 participants