udiskie: 1.4.8 -> 1.5.1 #18396
udiskie: 1.4.8 -> 1.5.1 #18396
Conversation
Also switch to `fetchFromGitHub` and do minor formatting.
| @@ -27121,13 +27121,15 @@ in modules // { | |||
| }; | |||
| }; | |||
|
|
|||
| udiskie = buildPythonPackage rec { | |||
| version = "1.4.8"; | |||
| udiskie = buildPythonApplication rec { | |||
There was a problem hiding this comment.
this is indeed an application. As such, this expression should be moved outside of python-packages.nix and put somewhere else in nixpkgs, since python-packages.nix is only intended for libraries or packages that need to be available for Python versions.
There was a problem hiding this comment.
Added a commit that moves the package to applications/misc/udiskie. I kept a reference in pythonPackages for backwards compatibility. I'm not sure whether that is necessary, though…
FRidh
added
6.topic: python
8.has: package (update)
| }; | ||
|
|
||
| preConfigure = '' | ||
| export XDG_RUNTIME_DIR=/tmp |
There was a problem hiding this comment.
you can set environment variables outside of the phases. Just passing XDG_RUNTIME_DIR=/tmp; is sufficient.
There was a problem hiding this comment.
you can set environment variables outside of the phases. Just passing XDG_RUNTIME_DIR=/tmp; is sufficient.
I know this feature is used all over the place, but I always felt that was weird/wrong. Like a leaky abstraction.
There was a problem hiding this comment.
As far as I can tell the XDG_RUNTIME_DIR variable doesn't matter at all in the udiskie build. The package builds and runs just fine without it. I removed it and force-pushed.
It might be used by the test suite but that is disabled anyway…
There was a problem hiding this comment.
@bjornfor could you explain why you think it is weird or wrong? I can imagine it is strange to set something outside of a phase, and so it might not be clear when exactly these env vars are set.
There was a problem hiding this comment.
On 12 September 2016 at 11:07, Frederik Rietdijk
notifications@github.com wrote:
In pkgs/applications/misc/udiskie/default.nix:
+, gobjectIntrospection, gtk3, hicolor_icon_theme, libnotify
+, pythonPackages, udisks2, wrapGAppsHook }:
+
+pythonPackages.buildPythonApplication rec {
- name = "udiskie-${version}";
- version = "1.5.1";
- src = fetchFromGitHub {
- owner = "coldfix";
- repo = "udiskie";
- rev = version;
- sha256 = "01x5fvllb262x6r3547l23z7p6hr7ddz034bkhmj2cqmf83sxwxd";
- };
- preConfigure = ''
- export XDG_RUNTIME_DIR=/tmp
@bjornfor could you explain why you think it is weird or wrong? I can imagine it is strange to set something outside of a phase, and so it might not be clear when exactly these env vars are set.
Why I think it's weird:
- No clear distinction between Nix value and Shell value. It's just
"all the same". - A very "global" scope. No way to set an envvar for just configure phase.
I think something like this would be cleaner (inspired by Buildroot):
builderEnv = { MYVAR = "VALUE"; }; # this will be set for all
processes spawned in a build
configureEnv = { MYVAR = "VALUE"; }; # just when invoking ./configure
I guess it all comes down to ones perception of the
stdenv.mkDerivation function. Mine is probably wrong :-)
There was a problem hiding this comment.
Yes I agree that would be much nicer. We should build wrappers in such a way as well I think.
But for the case here, since at this point the state is inherited from one phase to another, there is no difference between using export X=y; or X=y; except if you want to export the var after a certain phase.
This is an application, not a python library and should therefore be in its own package.
|
Thanks for the review! |
Motivation for this change
Version bump and use of GApps wrapper and icon hooks. Unfortunately, despite the use of gapps wrapper I still get some missing icons in the tray menu.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)