Skip to content

treewide: replace wrapGAppsHook with wrapGAppsHook4 for gtk4 apps#301865

Merged
Aleksanaa merged 1 commit intoNixOS:masterfrom
Aleksanaa:wrapGAppsHook4-fix
Apr 7, 2024
Merged

treewide: replace wrapGAppsHook with wrapGAppsHook4 for gtk4 apps#301865
Aleksanaa merged 1 commit intoNixOS:masterfrom
Aleksanaa:wrapGAppsHook4-fix

Conversation

@Aleksanaa
Copy link
Member

@Aleksanaa Aleksanaa commented Apr 5, 2024 •
edited
Loading

Description of changes

This is checked with

grep -rl 'gtk4' ./pkgs | xargs grep -l 'wrapGAppsHook' | xargs grep -L 'wrapGAppsHook4'

With some exceptions:

  • ./pkgs/applications/misc/almanah/default.nix
    Still uses gtk3
  • ./pkgs/applications/networking/browsers/brave/default.nix
  • ./pkgs/applications/networking/browsers/opera/default.nix
  • ./pkgs/desktops/gnome/core/evolution-data-server/default.nix
  • ./pkgs/desktops/gnome/core/gnome-terminal/default.nix
  • ./pkgs/desktops/gnome/extensions/gsconnect/default.nix
  • ./pkgs/desktops/gnome/extensions/extensionOverrides.nix
  • ./pkgs/desktops/gnome/misc/gpaste/default.nix
    Still uses gtk3
  • ./pkgs/development/libraries/gtk/4.x.nix
  • ./pkgs/development/lisp-modules/packages.nix
  • ./pkgs/development/tools/cambalache/default.nix
  • ./pkgs/development/tools/electron/wrapper.nix
  • ./pkgs/tools/inputmethods/ibus/default.nix
  • ./pkgs/by-name/pa/paper-plane/package.nix`

Also checked top-level reference, didn't see any of these using wrapGAppsHook = wrapGAppsHook4.

Since using wrapGAppsHook should be considered a mistake in such packages, I didn't consider issues like keeping overriding interface.

Please comment here as soon as possible if you think there is a good reason to use wrapGAppsHook in any of these packages.

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/)
  • 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
    • (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.

@Aleksanaa Aleksanaa requested a review from jtojnar as a code owner April 5, 2024 14:43
@github-actions github-actions bot added the 6.topic: GNOME GNOME desktop environment and its underlying platform label Apr 5, 2024
jtojnar
jtojnar reviewed Apr 5, 2024
View reviewed changes
@bobby285271
Copy link
Member

bobby285271 commented Apr 5, 2024

@ofborg build gnome.gnome-sound-recorder

I imagine this needs either gnome.post_install() or gtk-update-icon-cache → gtk4-update-icon-cache 🙃

jtojnar
jtojnar reviewed Apr 5, 2024
View reviewed changes
@Aleksanaa
Copy link
Member Author

Aleksanaa commented Apr 5, 2024

I imagine this needs either gnome.post_install() or gtk-update-icon-cache → gtk4-update-icon-cache 🙃

Then our troubles are even bigger. I have seen quite a few projects like this. 🙃

@jtojnar
Copy link
Member

jtojnar commented Apr 5, 2024

  • I believe zrythm is GTK 4.
  • Also checked gpaste since the library supports both versions but the GUI app is still GTK 3.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Apr 5, 2024
@Aleksanaa Aleksanaa force-pushed the wrapGAppsHook4-fix branch from 4b855ed to 152d09f Compare April 5, 2024 15:24
@Aleksanaa Aleksanaa marked this pull request as draft April 5, 2024 15:30
@Aleksanaa
Copy link
Member Author

Aleksanaa commented Apr 5, 2024

Just ran nixpkgs-review locally

Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/301865

4 packages failed to build:
audio-sharing confy gnome.gnome-sound-recorder parlatype

18 packages built:
conjure curtail dino errands gnome-network-displays gnome-solanum gnome.gnome-control-center gnome.gnome-control-center.debug goldwarden gpu-viewer greetd.regreet lact lxi-tools-gui phosh phosh-mobile-settings szyszka tangram zrythm

Not so bad after all ☺️

@Aleksanaa Aleksanaa force-pushed the wrapGAppsHook4-fix branch from 152d09f to 02b4a21 Compare April 5, 2024 16:05
@Aleksanaa
Copy link
Member Author

Aleksanaa commented Apr 5, 2024

All successfully built!

Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/301865

22 packages built:
audio-sharing confy conjure curtail dino errands gnome-network-displays gnome-solanum gnome.gnome-control-center gnome.gnome-control-center.debug gnome.gnome-sound-recorder goldwarden gpu-viewer greetd.regreet lact lxi-tools-gui parlatype phosh phosh-mobile-settings szyszka tangram zrythm

@Aleksanaa Aleksanaa marked this pull request as ready for review April 5, 2024 16:12
jtojnar
jtojnar reviewed Apr 5, 2024
View reviewed changes
@Aleksanaa Aleksanaa force-pushed the wrapGAppsHook4-fix branch from 02b4a21 to 85a34fd Compare April 5, 2024 16:29
@Aleksanaa Aleksanaa requested a review from jtojnar April 5, 2024 17:04
@Aleksanaa Aleksanaa force-pushed the wrapGAppsHook4-fix branch from 85a34fd to 9dddc11 Compare April 5, 2024 17:34
@Aleksanaa Aleksanaa mentioned this pull request Apr 5, 2024
Merged
13 tasks
jtojnar
jtojnar approved these changes Apr 5, 2024
View reviewed changes
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

For most packages I only skimmed the sources but looks great, thanks.

@wegank wegank 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 Apr 5, 2024
@Aleksanaa Aleksanaa requested a review from bobby285271 April 6, 2024 02:29
@Aleksanaa
Copy link
Member Author

Aleksanaa commented Apr 7, 2024

Merging this. Hope I'm not making the world explode

@Aleksanaa Aleksanaa merged commit 7b01c27 into NixOS:master Apr 7, 2024
@Aleksanaa Aleksanaa mentioned this pull request Apr 8, 2024
Merged
13 tasks
This was referenced Apr 8, 2024
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtojnar jtojnar jtojnar approved these changes

@bobby285271 bobby285271 Awaiting requested review from bobby285271

Assignees

No one assigned

Labels

6.topic: GNOME GNOME desktop environment and its underlying platform 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Aleksanaa @bobby285271 @jtojnar @wegank