Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Jul 25, 2019 •
edited

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
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
@worldofpeace
Copy link
Contributor

worldofpeace commented Jul 25, 2019 •
edited

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
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
Contributor

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
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

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
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 ttuegel is a code owner

@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 10.rebuild-darwin: 11-100 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@FRidh @worldofpeace @ttuegel @nixos-discourse @Mic92