Skip to content

qt5.wrapQtAppsHook: wrap more than just ELF's#65372

Closed
FRidh wants to merge 3 commits intoNixOS:masterfrom
FRidh:qt5
Closed

qt5.wrapQtAppsHook: wrap more than just ELF's#65372
FRidh wants to merge 3 commits intoNixOS:masterfrom
FRidh:qt5

Conversation

@FRidh
Copy link
Copy Markdown
Member

@FRidh FRidh commented Jul 25, 2019 •
edited
Loading

Motivation for this change

We should also be able to wrap Python scripts. Note that this also means
we can create nested wrappers. While unfortunate, until we have a way to
compose wrappers it's the only way.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

FRidh added 3 commits July 25, 2019 14:55
@FRidh
qt5.wrapQtAppsHook: wrap more than just ELF's
625f0be 
We should also be able to wrap Python scripts. Note that this also means
we can create nested wrappers. While unfortunate, until we have a way to
compose wrappers it's the only way.
@FRidh
git-cola: use qt5.wrapQtAppsHook
b1f567b
@FRidh
calibre: use qt5.wrapQtAppsHook, fixes NixOS#65313
2565197
@FRidh FRidh requested a review from ttuegel as a code owner July 25, 2019 12:58
@FRidh FRidh requested a review from worldofpeace July 25, 2019 12:58
@FRidh FRidh added the 6.topic: qt/kde Object-oriented framework for GUI creation label Jul 25, 2019
@ofborg ofborg bot requested review from AndersonTorres, domenkozar and pSub July 25, 2019 13:12
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. labels Jul 25, 2019
@worldofpeace
Copy link
Copy Markdown
Contributor

worldofpeace commented Jul 25, 2019 •
edited
Loading

7d6ab0a didn't actually break this, It probably didn't work even before this since it used to be conditioned on isQtApp.

Initially I removed this like you've done here, but I had to restore a similar behavior because of issues I noticed in the plasma5 session
#64720 (comment).

Possibly something there shouldn't be wrapped, but since it's added unconditionally when using qt5.mkDerivation it may be difficult to figure out what.
And even then, It may be a good idea for these scripts to at least wrap XDG_DATA_DIRS.

@ttuegel
Copy link
Copy Markdown
Member

ttuegel commented Jul 25, 2019

Possibly something there shouldn't be wrapped, but since it's added unconditionally when using qt5.mkDerivation it may be difficult to figure out what.

It's not added unconditionally; set dontWrapQtApps = true;.

@worldofpeace
Copy link
Copy Markdown
Contributor

worldofpeace commented Jul 25, 2019

So I guess the step forward would be to identify in plasma5 what packages provide scripts that shouldn't be wrapped and to set that.

@ttuegel
Copy link
Copy Markdown
Member

ttuegel commented Jul 25, 2019

I expect that most scripts don't need to be wrapped, and I would prefer to use isELF + semi-manually wrap (using wrapQtApp) the minority of scripts that need to be wrapped.

@ttuegel ttuegel mentioned this pull request Jul 25, 2019
Closed
@nixos-discourse
Copy link
Copy Markdown

nixos-discourse commented Jul 27, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/wrappers-and-hooks-do-not-invoke-wrapprogram-directly/3551/1

@FRidh FRidh mentioned this pull request Jul 29, 2019
Closed
@FRidh
Copy link
Copy Markdown
Member Author

FRidh commented Jul 30, 2019

Pushed 92cb811 and 1190f69 using wrapQtApp directly instead.

@FRidh FRidh closed this Jul 30, 2019
@FRidh FRidh mentioned this pull request Sep 11, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mic92 Mic92 Mic92 approved these changes

@ttuegel ttuegel Awaiting requested review from ttuegel

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

@pSub pSub Awaiting requested review from pSub

@domenkozar domenkozar Awaiting requested review from domenkozar

@AndersonTorres AndersonTorres Awaiting requested review from AndersonTorres

Assignees

No one assigned

Labels

6.topic: qt/kde Object-oriented framework for GUI creation 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@FRidh @worldofpeace @ttuegel @nixos-discourse @Mic92