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

lollypop-portal: init at 0.9.7 #43447

Merged
merged 2 commits into from Jul 16, 2018
Merged

lollypop-portal: init at 0.9.7 #43447

merged 2 commits into from Jul 16, 2018

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

This #43292

Notes

I didn't exactly know where this belonged in nixpkgs so feel free to suggest a better place.
There will also need to be a stupid simple nixos module for this.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor Author

@cocreature Can you test this?

jtojnar
jtojnar reviewed Jul 13, 2018
View reviewed changes
pkgs/misc/lollypop-portal/default.nix Outdated
postFixup = ''
buildPythonPath "$out/libexec/lollypop-portal $pythonPath"

wrapProgram $out/libexec/lollypop-portal \
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternately, you can append to gappsWrapperArgs:

nixpkgs/pkgs/desktops/gnome-3/core/nautilus/default.nix

Lines 26 to 33 in 9bbb84f

preFixup = ''
gappsWrapperArgs+=(
# Thumbnailers
--prefix XDG_DATA_DIRS : "${gdk_pixbuf}/share"
--prefix XDG_DATA_DIRS : "${librsvg}/share"
--prefix XDG_DATA_DIRS : "${shared-mime-info}/share"
)
'';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol That's what I was trying to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that this won't make a wrapper for the program at libexec which is why I think I did this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me:

--- a/pkgs/misc/lollypop-portal/default.nix
+++ b/pkgs/misc/lollypop-portal/default.nix
@@ -14,7 +14,7 @@
      sha256 = "0rn5xmh6391i9l69y613pjad3pzdilskr2xjfcir4vpk8wprvph3";
   };
 
-  nativeBuildInputs = [ 
+  nativeBuildInputs = [
     gobjectIntrospection
     meson
     ninja
@@ -41,15 +41,12 @@
     pycairo
   ];
 
-  dontWrapGApps = true;
-
-  postFixup = ''
+  preFixup = ''
     buildPythonPath "$out/libexec/lollypop-portal $pythonPath"
-
-    wrapProgram $out/libexec/lollypop-portal \
-      "''${gappsWrapperArgs[@]}" \
-        --prefix PYTHONPATH : "$program_PYTHONPATH" \
-        --prefix PATH : "${stdenv.lib.makeBinPath [ easytag kid3 ]}"
+    gappsWrapperArgs+=(
+      --prefix PYTHONPATH : "$program_PYTHONPATH"
+      --prefix PATH : "${stdenv.lib.makeBinPath [ easytag kid3 ]}"
+    )
   '';
 
   meta = with stdenv.lib; {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird I ended up with the same change and it fails?

Can you run ./result/libexec/lollypop-portal without an import error?

Copy link
Contributor

@jtojnar jtojnar Jul 13, 2018 •
edited

Choose a reason for hiding this comment

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

Yep, produces almost the same wrapper file (the order of variables is slightly different).

@cocreature
Copy link
Contributor

@worldofpeace Thanks a lot! I can confirm that this works for me!

I have two small comments:

  1. It might make sense to make lollypop depend on this. Technically, it is not a required dependency but I am not sure if this is intended by upstream. At the very least a comment in the lollypop derivation might be useful since just not having menu items show up is somewhat confusing.
  2. You seem to have added kid3 and easytag to the PATH of lollypop-portal. I wonder if it might not be best to just have users install those packages if they want them especially given that lollypop supports more tag editors (see https://gitlab.gnome.org/World/lollypop/blob/master/lollypop/define.py#L24 but I think you could even configure an arbitrary editor).

@worldofpeace
Copy link
Contributor Author

@cocreature Thanks for your comments.

It might make sense to make lollypop depend on this. Technically, it is not a required dependency but I am not sure if this is intended by upstream. At the very least a comment in the lollypop derivation might be useful since just not having menu items show up is somewhat confusing.

It is true that this isn't a required dependency because the author is responsible for the ubuntu packaging and it's not required there. Maybe I could improve the package descriptions also, since a nix search lollypop would yield both of these packages. IMHO the author doesn't make this very clear in the first place.

You seem to have added kid3 and easytag to the PATH of lollypop-portal. I wonder if it might not be best to just have users install those packages if they want them especially given that lollypop supports more tag editors (see https://gitlab.gnome.org/World/lollypop/blob/master/lollypop/define.py#L24 but I think you could even configure an arbitrary editor).

kid3-cli needs to be in the path for you to set the cover and popularity.
So that would obviously belong in the lollypop-portal derivation default.
As for all the various tag editors, Perhaps we could utilize a nixos module for this configuration or options in the derivation(which are hard to discover).

@cocreature
Copy link
Contributor

Alright, sounds good! No further comments from my side. For now just depending on easytag is totally fine for me.

@worldofpeace
Copy link
Contributor Author

Your exclamation confused me. 😆

@cocreature
Copy link
Contributor

Sorry about that 😀

@fpletz fpletz merged commit e3291c3 into NixOS:master Jul 16, 2018
@worldofpeace worldofpeace deleted the lollypop-portal branch July 16, 2018 05:17
@peti
Copy link
Member

peti commented Jul 20, 2018

This commit introduces evaluation errors into Nixpkgs:

$ nix-env -qaP --meta --json  >/dev/null
derivation 'lollypop-portal' has invalid meta attribute 'override'
derivation 'lollypop-portal' has invalid meta attribute 'overrideDerivation'

@worldofpeace
Copy link
Contributor Author

@peti This commit fixed that. I frequently misspell that.

@peti
Copy link
Member

peti commented Jul 20, 2018

Cool, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

Assignees
No one assigned
Labels
8.has: package (new) 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
6 participants
@worldofpeace @cocreature @peti @jtojnar @fpletz @GrahamcOfBorg