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.

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

wrapGAppsHook: don't add empty variables (see also #75443) #75450

Merged

Conversation

KoviRobi
Copy link
Contributor

Adding empty variables can lead to this problem:

 wrapProgram \
     ./pye_menu_shell \
     --prefix PATH : /nix/store/4c3z5r6yxsf2cxwwyazhdn92xixn4j5b-python3-3.7.5/bin:/nix/store/b3l3niilvqcxcsbxmd6sgqk1dy1rk81c-pye-menu-1.0/bin:/nix/store/y8j1cfj8d9r5rbbxc22w7hnfjw5f4fd3-cairo-1.16.0-dev/bin:/nix/store/6mg7lfbdh9pgx7pbxr3544qqbrigdl1q-freetype-2.10.1-dev/bin:/nix/store/gpszqcy0xi0lavbbjdq82zkkjp3jbp2a-bzip2-1.0.6.0.1-bin/bin:/nix/store/031c5pk5lzabgmpqpyd46hzi625as6bp-libpng-apng-1.6.37-dev/bin:/nix/store/f8kl7kmpv130aw9zm542p74a3hg0yc13-fontconfig-2.12.6-bin/bin:/nix/store/bqp30vkncmm222mjvwggz0s7p318sflj-expat-2.2.7-dev/bin:/nix/store/w57xa8g4s4aviwmqwgra7m5hwj2b005m-glib-2.60.7-dev/bin:/nix/store/v5d4966ahvfir2hwpv003022f3pb7vik-gettext-0.19.8.1/bin:/nix/store/qpvxhl1jr0fxnrx9idnpdagqs00m5m2z-glib-2.60.7/bin \
     --set PYTHONNOUSERSITE true \
     --set GDK_PIXBUF_MODULE_FILE /nix/store/7ddlakx6xjczqbfs80xjd14f30fzadws-gdk-pixbuf-2.38.1/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache \
     --prefix XDG_DATA_DIRS : /nix/store/0snjc1qg89zqn3v35l9d55xrykh9nj5c-gtk+3-3.24.10/share/gsettings-schemas/gtk+3-3.24.10:/nix/store/b41z51vdv11n6df8ki5vj8dynxw98f9l-gsettings-desktop-schemas-3.32.0/share/gsettings-schemas/gsettings-desktop-schemas-3.32.0:/nix/store/0snjc1qg89zqn3v35l9d55xrykh9nj5c-gtk+3-3.24.10/share/gsettings-schemas/gtk+3-3.24.10 \
-     --prefix GST_PLUGIN_SYSTEM_PATH_1_0 : \
+     --prefix GST_PLUGIN_SYSTEM_PATH_1_0 : "" \
     --prefix GI_TYPELIB_PATH : /nix/store/0snjc1qg89zqn3v35l9d55xrykh9nj5c-gtk+3-3.24.10/lib/girepository-1.0:/nix/store/z29l5xaaxh1s0697mcldj71ab0zshry1-librsvg-2.44.15/lib/girepository-1.0:/nix/store/pija1xzm7izxfb5m2hvhvlwp1l38ffxa-gobject-introspection-1.60.2/lib/girepository-1.0 \
-     --prefix GRL_PLUGIN_PATH :
+     --prefix GRL_PLUGIN_PATH : ""

Where the diff is to highlight the problem: we don't have a valid value
for GST_PLUGIN_SYSTEM_PATH_1_0 or GRL_PLUGIN_PATH, and instead of
passing the empy string, the empty string gets unquoted somewhere, so we
end up passing no arguments, thus the parser in wrapProgram takes
--prefix as the argument of GST_PLUGIN_SYSTEM_PATH_1_0, and then
GI_TYPELIB_PATH is missing it's --prefix so wrapProgram complains/dies.

The easiest change is to not add empty arguments to the wrapper
See also #75443

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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.
Notify maintainers

cc @

Adding empty variables can lead to this problem:

```diff
 wrapProgram \
     ./pye_menu_shell \
     --prefix PATH : /nix/store/4c3z5r6yxsf2cxwwyazhdn92xixn4j5b-python3-3.7.5/bin:/nix/store/b3l3niilvqcxcsbxmd6sgqk1dy1rk81c-pye-menu-1.0/bin:/nix/store/y8j1cfj8d9r5rbbxc22w7hnfjw5f4fd3-cairo-1.16.0-dev/bin:/nix/store/6mg7lfbdh9pgx7pbxr3544qqbrigdl1q-freetype-2.10.1-dev/bin:/nix/store/gpszqcy0xi0lavbbjdq82zkkjp3jbp2a-bzip2-1.0.6.0.1-bin/bin:/nix/store/031c5pk5lzabgmpqpyd46hzi625as6bp-libpng-apng-1.6.37-dev/bin:/nix/store/f8kl7kmpv130aw9zm542p74a3hg0yc13-fontconfig-2.12.6-bin/bin:/nix/store/bqp30vkncmm222mjvwggz0s7p318sflj-expat-2.2.7-dev/bin:/nix/store/w57xa8g4s4aviwmqwgra7m5hwj2b005m-glib-2.60.7-dev/bin:/nix/store/v5d4966ahvfir2hwpv003022f3pb7vik-gettext-0.19.8.1/bin:/nix/store/qpvxhl1jr0fxnrx9idnpdagqs00m5m2z-glib-2.60.7/bin \
     --set PYTHONNOUSERSITE true \
     --set GDK_PIXBUF_MODULE_FILE /nix/store/7ddlakx6xjczqbfs80xjd14f30fzadws-gdk-pixbuf-2.38.1/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache \
     --prefix XDG_DATA_DIRS : /nix/store/0snjc1qg89zqn3v35l9d55xrykh9nj5c-gtk+3-3.24.10/share/gsettings-schemas/gtk+3-3.24.10:/nix/store/b41z51vdv11n6df8ki5vj8dynxw98f9l-gsettings-desktop-schemas-3.32.0/share/gsettings-schemas/gsettings-desktop-schemas-3.32.0:/nix/store/0snjc1qg89zqn3v35l9d55xrykh9nj5c-gtk+3-3.24.10/share/gsettings-schemas/gtk+3-3.24.10 \
-     --prefix GST_PLUGIN_SYSTEM_PATH_1_0 : \
+     --prefix GST_PLUGIN_SYSTEM_PATH_1_0 : "" \
     --prefix GI_TYPELIB_PATH : /nix/store/0snjc1qg89zqn3v35l9d55xrykh9nj5c-gtk+3-3.24.10/lib/girepository-1.0:/nix/store/z29l5xaaxh1s0697mcldj71ab0zshry1-librsvg-2.44.15/lib/girepository-1.0:/nix/store/pija1xzm7izxfb5m2hvhvlwp1l38ffxa-gobject-introspection-1.60.2/lib/girepository-1.0 \
-     --prefix GRL_PLUGIN_PATH :
+     --prefix GRL_PLUGIN_PATH : ""
```
Where the diff is to highlight the problem: we don't have a valid value
for GST_PLUGIN_SYSTEM_PATH_1_0 or GRL_PLUGIN_PATH, and instead of
passing the empy string, the empty string gets unquoted somewhere, so we
end up passing no arguments, thus the parser in wrapProgram takes
--prefix as the argument of GST_PLUGIN_SYSTEM_PATH_1_0, and then
GI_TYPELIB_PATH is missing it's --prefix so wrapProgram complains/dies.

The easiest change is to not add empty arguments to the wrapper
@KoviRobi
Copy link
Contributor Author

Draft pull-request while I figure out how to test this properly, I feel like this might impact a lot of packages

Copy link
Contributor

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

This change is a good idea.

While makeWrapper will ignore empty values to prefix, passing them does not make much sense. Also, we guard the other env vars, so we should do it here for consistency.

Though the issue still occurred from improper use due to bad documentation. We should fix that as well.

Staging automation moved this from WIP to Ready Dec 10, 2019
@jtojnar
Copy link
Contributor

jtojnar commented Dec 10, 2019

Verified that it fixes your project build, even with the broken makeWrapperArgs:

diff --git a/default.nix b/default.nix
index e0b2c15..a168ea8 100644
--- a/default.nix
+++ b/default.nix
@@ -1,11 +1,17 @@
 let nixpkgs = import <nixpkgs> {};
 in
-{ python3 ? nixpkgs.python3,
-  gtk3 ? nixpkgs.gtk3,
+{ python3 ? nixpkgs.python3, librsvg ? nixpkgs.librsvg, makeSetupHook ? nixpkgs.makeSetupHook, makeWrapper ? nixpkgs.makeWrapper
+  ,gtk3 ? nixpkgs.gtk3,
   gobject-introspection ? nixpkgs.gobject-introspection,
   wrapGAppsHook ? nixpkgs.wrapGAppsHook,
   lib ? nixpkgs.lib 
 }:
+
+let
+  wrapGAppsHook = makeSetupHook {
+    deps = [ gtk3 librsvg makeWrapper ];
+  } /home/jtojnar/Projects/nixpkgs/pkgs/build-support/setup-hooks/wrap-gapps-hook2.sh;
+in
 python3.pkgs.buildPythonPackage rec {
   pname = "pye-menu";
   version = "1.0";

@jtojnar jtojnar changed the base branch from master to staging December 10, 2019 18:47
@worldofpeace
Copy link
Contributor

Though the issue still occurred from improper use due to bad documentation. We should fix that as well.

Done in #75465

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

LGTM, I actually had the same diff in a stash when I noticed it weeks ago.

@worldofpeace
Copy link
Contributor

worldofpeace commented Dec 11, 2019

The build failure isn't valid, I can build this hook locally no problem. (rebased)

@worldofpeace worldofpeace marked this pull request as ready for review December 11, 2019 04:57
@worldofpeace worldofpeace merged commit 25879b3 into NixOS:staging Dec 11, 2019
Staging automation moved this from Ready to Done Dec 11, 2019
@KoviRobi KoviRobi deleted the fix-wrap-gapps-hook-shell-quoting branch October 10, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants