treewide: use non-binary wrapper for wrapGAppsHook where appropriate#172320
treewide: use non-binary wrapper for wrapGAppsHook where appropriate#172320thiagokokada merged 8 commits intoNixOS:masterfrom
Conversation
The use of --add-flags in this derivation does not work with binary wrappers, which wrapGAppsHook uses since NixOS#164163.
|
Ok, the test passed and I am successfully running signal from this branch on my system. |
|
I think we can fix the other programs in this PR as well |
will do that! |
The use of --add-flags in this derivation does not work with binary
wrappers, which wrapGAppsHook uses since NixOS#164163.
The use of --add-flags in this derivation does not work with binary
wrappers, which wrapGAppsHook uses since NixOS#164163.
The use of --add-flags in this derivation does not work with binary
wrappers, which wrapGAppsHook uses since NixOS#164163.
ofborg
bot
added
10.rebuild-darwin: 0
|
I did build From a quick grep through nixpkgs, these are indeed all derivations that use |
|
Yes, but none of those I could find ( |
|
Please change the PR's title |
xaverdh
changed the title
ofborg
bot
removed
the
10.rebuild-linux: 1
I think e.g. cryptomator is also broken, unless I am mistaken? |
|
It doesn't look like it expands any variables at runtime. |
|
Will the |
|
At least it won't get expanded by Bash, because of the single quotes. |
|
But this will. Let's add |
|
Right, still there appears to be an issue with the |
|
Ah, I guess the binary implementation of |
|
There are also cases like this where the implementation is "leaked" to users, but Its not very likely somebody relies on injecting shell code there? |
|
It looks like we also need to fix |
|
There's a missing |
The use of --add-flags in this derivation does not work with binary
wrappers, which wrapGAppsHook uses since NixOS#164163.
|
I just tested vscode using this patch and now it works. |
The use of --add-flags in this derivation does not work with binary
wrappers, which wrapGAppsHook uses since NixOS#164163.
The use of --add-flags in this derivation assumed quotes to be expanded, which the binary
wrapper (which wrapGAppsHook uses since NixOS#164163) will not do.
The use of --add-flags in this derivation assumed quotes to be expanded, which the binary
wrapper (which wrapGAppsHook uses since NixOS#164163) will not do.
xaverdh
force-pushed
the
binary-wrapper-fixes
branch
from
e59f2e9 to
757f518
Compare
ofborg
bot
added
10.rebuild-darwin: 1-10
|
Do you think we can commit this as-is so we unblock people using vscode and other software impacted by this issue ? |
|
Done. |
|
Thanks ! Any clue on how to force a new evaluation on hydra? |
No, but not sure if this is a good idea. I mean, almost every PR here is important for those that need it, if we had to restart hydra evals on each PR merged we would never get anywhere. |
|
Started a new evaluation. I think vscode + signal + discord is past the threshold of "important for almost everyone". |
|
Thank you :) |
| autoPatchelfHook | ||
| maven | ||
| makeWrapper | ||
| (wrapGAppsHook.override { makeBinaryWrapper = makeWrapper; }) |
There was a problem hiding this comment.
Why is the override needed for cryptomator?
There was a problem hiding this comment.
We could also remove the single quotes, but then we would be implicitly relying on the binary wrapper being in scope, which isn't great.
There was a problem hiding this comment.
So makeBinaryWrapper ends up in cryptomator's nativeBuildInputs without? That should not happen.
There was a problem hiding this comment.
The derivation doesn't use gappsWrapperArgs.
There was a problem hiding this comment.
So makeBinaryWrapper ends up in cryptomator's nativeBuildInputs without? That should not happen.
Yes, because it's a propagated build input of the wrapGAppsHook (that is how setup hooks work). I agree that's not great, but I don't know how we should fix it. This is a limitation of Bash more than anything.
The derivation doesn't use gappsWrapperArgs.
It doesn't need to. It first creates the cryptomator executable as a wrapper over java, and then wrapGAppsHook wraps that wrapper automatically in the fixup phase.
There was a problem hiding this comment.
makeBinaryWrapper is also a setup hook, i.e. a Bash script that defines functions. Functions aren't scoped in Bash, so we would need to do something like
wrapGApp() (
local program="$1"
shift 1
source @makeWrapper@
wrapProgram "$program" "${gappsWrapperArgs[@]}" "$@"
)in order not to expose the functions to the global scope. Maybe it could work.
There was a problem hiding this comment.
But makeWrapper also depends on dieHook, so we'd also need to source that explicitly... it's kind of awkward.
There was a problem hiding this comment.
We could call one of the bash functions makeBinaryWrapper and use a boolean argument for wrapGAppsHook
There was a problem hiding this comment.
We could call one of the bash functions makeBinaryWrapper
I'm doing that in #172366, but the goal of the makeBinaryWrapper hook is to be a drop-in replacement for makeWrapper, so we can't just drop the makeWrapper function.
There was a problem hiding this comment.
Then let's call the other makeShellWrapper so people can decide which one they want to use if they feel like it. That would make this override superfluous.
|
@thiagokokada - wordexp looks interesting. As for patterns like I think --set X '${Y:-$Z}'would be equivalent to this (assuming wordexp is able to expand '$Y' and '$Z' at runtime) --set X '$Y'
--set-default X '$Z'Need to think a bit more about |
Huh... Actually re-reading wordexp documentation it really looks like what we want (it expand env vars too, I thought it only expanded things like I also wouldn't worry too much about the performance. I think this would be better to be behind a flag anyway, since I think having it enabled by default (at least for now) can cause some issues. @bergkvist Can you open an issue for this? I think it would be useful to concentrate the discussion there, instead of a closed PR. |
6 participants
The use of --add-flags in this derivation does not work with binary
wrappers, which wrapGAppsHook uses since #164163.
Description of changes
See discussion here
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes