NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.41k stars 14.36k forks source link

do not invoke wrapProgram directly, make a wrapProgramsHook #78792

Open worldofpeace opened 4 years ago

worldofpeace commented 4 years ago

Describe the bug Having all these setup-hooks like wrap{Python,{G,Qt}Apps}Hook that also wrap automatically has been a total pain. Intersecting any of these frameworks results in double (or more) wrapped files and it's just not a nice solution.

We should instead have none of these hooks do wrapping at all, just populate a shell array and forward it to makeWrapperArgs. After all these hooks run we run wrapProgramsHook which actually invokes wrapProgram on all paths mentioned in the array.

This idea was originally mentioned at https://discourse.nixos.org/t/wrappers-and-hooks-do-not-invoke-wrapprogram-directly/3551

worldofpeace commented 4 years ago

Added to the milestone because I can't imagine continuing without it.

FRidh commented 4 years ago

The hardest part here is that we want to set values per target. Using shell arrays means nested arrays. That does not seem like a very good idea to me. Therefore, I suggest using JSON directly. It's a pain for hooks written in bash, but long-term I think that's the best way forward. Together with structured attributes this seems like a good goal for 20.09.

veprbl commented 4 years ago

An alternative approach could be to teach the wrapProgram to detect an existing wrapper and amend it if needed.

mkg20001 commented 4 years ago

Another idea would be to make the command just collect things into an array and then at the end execute all the changes in one hook?

rnhmjoj commented 4 years ago

Linking here #83321 for discussion.

deliciouslytyped commented 4 years ago

@veprbl wouldn't such a patching based approach likely be more complex than just generating things properly in the first place? - unless...you make the wrapper execute based on some sort of easy to modify description file.

Which is I guess the same as the JSON suggestion, just asking whether we should have that generated at Nix time or used at runtime.

We've run into some sort of classic compositionality problem here.

veprbl commented 4 years ago

@veprbl wouldn't such a patching based approach likely be more complex than just generating things properly in the first place?

From what I can tell, the proposed alternative is to set wrapper arguments via makeWrapperArgs for python, Qt and Gtk applications and use old style makeWrapper for the rest. And there will be some exceptions, because makeWrapperArgs is not very flexible. And, any mixing of makeWrapperArgs and makeWrapper or multiple application of makeWrapper will still produce a double wrapping. So, as you can see, in this case there is also quite a bit of complexity, and the worst part is that it is pushed onto the maintainers.

unless...you make the wrapper execute based on some sort of easy to modify description file.

Wrappers should not read or parse any descriptions to not hinder the runtime performance. But, yes, they will need to be generated from a machine-readable manifest.

deliciouslytyped commented 4 years ago

Why not change makeWrapper to also inspect the manifest?

(I guess I just like runtime/after-the-fact introspection; could we put the manifest in the nix-support directory anyway? (I think that's where this kind of stuff goes right?) Is it just me or have people been reluctant to expose more things in nix-support? It can't hurt to have the information in there anyway?)

nixos-discourse commented 4 years ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/declarative-wrappers-another-idea/6661/1

stale[bot] commented 4 years ago

Hello, I'm a bot and I thank you in the name of the community for opening this issue.

To help our human contributors focus on the most-relevant reports, I check up on old issues to see if they're still relevant. This issue has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

The community would appreciate your effort in checking if the issue is still valid. If it isn't, please close it.

If the issue persists, and you'd like to remove the stale label, you simply need to leave a comment. Your comment can be as simple as "still important to me". If you'd like it to get more attention, you can ask for help by searching for maintainers and people that previously touched related code and @ mention them in a comment. You can use Git blame or GitHub's web interface on the relevant files to find them.

Lastly, you can always ask for help at our Discourse Forum or at #nixos' IRC channel.

stale[bot] commented 3 years ago

I marked this as stale due to inactivity. → More info

stale[bot] commented 2 years ago

I marked this as stale due to inactivity. → More info

turion commented 2 years ago

What's the status on this? I didn't know about double-wrapping being problematic and recently double-wrapped a program (https://github.com/NixOS/nixpkgs/pull/163608). Would be nice to find out what the right resolution to this is.