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

Jump to bottom

flatpak: Preserve pixbuf path in validate-icon #63122

Merged
merged 2 commits into from Jun 15, 2019
Merged

flatpak: Preserve pixbuf path in validate-icon #63122

merged 2 commits into from Jun 15, 2019

Conversation

mat8913
Copy link
Contributor

@mat8913 mat8913 commented Jun 14, 2019

Motivation for this change

Without this, flatpak-validate-icon fails on SVG files and I can't build any flatpaks which have SVG icons.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

I tested with:

$ git checkout 46e1c7f17aafaab18091f8fe04c1586f4ab932af^; nix-build . -A flatpak; ./result/libexec/flatpak-validate-icon --sandbox 16 16 ~/.local/share/flatpak/exports/share/icons/hicolor/symbolic/apps/org.gnome.Boxes-symbolic.svg; echo $?
HEAD is now at 29dc0ba6577 vimPlugins: update (#63119)
/nix/store/5nw27q7hb34a9bd49zdpg44fc8p1kh0k-flatpak-1.2.4
Format not recognized
1
$ git checkout 46e1c7f17aafaab18091f8fe04c1586f4ab932af; nix-build . -A flatpak; ./result/libexec/flatpak-validate-icon --sandbox 16 16 ~/.local/share/flatpak/exports/share/icons/hicolor/symbolic/apps/org.gnome.Boxes-symbolic.svg; echo $?
Previous HEAD position was 29dc0ba6577 vimPlugins: update (#63119)
HEAD is now at 46e1c7f17aa flatpak: Preserve pixbuf path in validate-icon
/nix/store/4a8j3i5k7d0f9skxwwfknsk7czk1ndfv-flatpak-1.2.4
0

@mat8913
flatpak: Preserve pixbuf path in validate-icon
46e1c7f 
SVG support isn't built-in to GdkPixBuf, it's provided by an external
module. Therefore, GDK_PIXBUF_MODULE_FILE should be preserved in the
sandbox, otherwise `flatpak-validate-icon --sandbox` won't work on SVG
files.
@ofborg ofborg bot requested a review from jtojnar June 14, 2019 12:37
@jtojnar
Copy link
Contributor

jtojnar commented Jun 14, 2019 •
edited

Not sure if we should pick it up from environment or wrap it with librsvg, see #59665

cc @hedning

@mat8913
Copy link
Contributor Author

mat8913 commented Jun 15, 2019

The environment will have the correct value since it's already set by the wrapper.

$ grep GDK_PIXBUF_MODULE_FILE ./result/libexec/flatpak-validate-icon
export GDK_PIXBUF_MODULE_FILE='/nix/store/n26qkqmargjvb1pd0fdjp9qxh38g1zsr-librsvg-2.44.14/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache'

@jtojnar
Copy link
Contributor

jtojnar commented Jun 15, 2019

Cool.

Could you add librsvg # for flatpak-validate-icon to buildInputs anyway to make the dependency explicit?

@ofborg ofborg bot requested a review from jtojnar June 15, 2019 05:28
hedning
hedning approved these changes Jun 15, 2019
View reviewed changes
Copy link
Contributor

@hedning hedning left a comment

Choose a reason for hiding this comment

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

LGTM

@jtojnar jtojnar merged commit 3a61b88 into NixOS:master Jun 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hedning hedning hedning approved these changes

@jtojnar jtojnar jtojnar approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mat8913 @jtojnar @hedning