-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
gnomeExtensions.volume-mixer: bring it back and fix it #87481
Conversation
meta = with stdenv.lib; { | ||
description = "GNOME Shell Extension allowing separate configuration of PulseAudio devices"; | ||
license = licenses.gpl2; | ||
maintainers = with maintainers; [ aneeshusa ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't use this extension anymore so I wouldn't be a good reviewer/maintainer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither. Do you want to be removed fron the maintainers list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that can interpreted that as a yes but it would be great to find someone that uses it to find bugs etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added myself as maintainer. Not sure when I have the time to test this on master again though...
I marked this as stale due to inactivity. → More info |
src = fetchFromGitHub { | ||
owner = "aleho"; | ||
repo = "gnome-shell-volume-mixer"; | ||
rev = "${version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rev = "${version}"; | |
rev = version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Result of 1 package built:
|
Can this file https://github.com/bjornfor/nixpkgs/blob/master/pkgs/desktops/gnome-3/extensions/volume-mixer.nix then be deleted? |
I don't see |
Oh, you literally meant the volume-mixer.nix file in my very old nixpkgs fork? You looked at the master branch, but that's not the branch this PR is against. Anyway, I updated the master branch in my fork just now. |
fff4019
to
da8c501
Compare
This reverts commit ee5506c. Further: * Add it to the gnomeExtensions attrset. (The reason for removing it in the first place was because it wasn't registered there.) * Move from .../volume-mixer.nix to .../volume-mixer/default.nix, to align with the other extensions. * Update to latest version. * Fix the installPhase. * Comment that there is a Makefile for this package, but it requires npm, so keep the buildPhase unchanged. * Add missing deps: pulseaudio and python3 * Add a small patch to find pulseaudio. * Replace maintainer aneeshusa with bjornfor, as aneeshusa doesn't seem interested in this package anymore.
da8c501
to
7a090b8
Compare
]; | ||
|
||
buildInputs = [ | ||
glib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is added for glib-compile-schemas
, it should go to nativeBuildInputs
, ideally with a comment.
substituteInPlace ./shell-volume-mixer@derhofbauer.at/pautils/pa.py \ | ||
--subst-var-by pulseaudio ${pulseaudio} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use substituteAll
directly in patches
.
|
||
buildInputs = [ | ||
glib | ||
pulseaudio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an actual build input or just needed for hardcoding the path?
--subst-var-by pulseaudio ${pulseaudio} | ||
''; | ||
|
||
# Could use the Makefile, but it requires npm... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we would ask upstream to make the dependency installation optional and allow using sassc from PATH
.
''; | ||
|
||
installPhase = '' | ||
mkdir -p $out/share/gnome-shell/extensions/${uuid} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When overriding installPhase
, you should also provide runHook preInstall
and runHook postInstall
. Though, in this case, it is better to rely on the default installPhase
.
''; | ||
|
||
installPhase = '' | ||
mkdir -p $out/share/gnome-shell/extensions/${uuid} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be also nice to ask upstream to add a make install
target.
|
||
meta = with stdenv.lib; { | ||
description = "GNOME Shell Extension allowing separate configuration of PulseAudio devices"; | ||
license = licenses.gpl2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gpl2
is unclear, use gpl2Plus
or gpl2Only
. See https://discourse.nixos.org/t/lib-licenses-gpl3-co-are-now-deprecated/8206 for more details.
description = "GNOME Shell Extension allowing separate configuration of PulseAudio devices"; | ||
license = licenses.gpl2; | ||
maintainers = with maintainers; [ bjornfor ]; | ||
homepage = https://github.com/aleho/gnome-shell-volume-mixer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unquoted URLs are deprecated: NixOS/rfcs#45
|
Please have a look at #137131. Note that my proposed patch framework works on top of the already compiled extensions.gnome.org sources, thus some non-trivial adjustments may be required to the patch. I'd appreciate if you tried to rebase this on top of my PR to test out the patch mechanism. But I can also give it a shot myself if you tell me which patches I need to apply (when ignoring the compilation setup). |
@piegamesde : Nice! I don't have enough interest/time to work on this now unfortunately. But thanks a lot for your work on the gnome extensions. Much appreciated! |
I'm going to close this then, hopefully the next motivated person will find this and start off from here. GNOME extensions that don't work out of the box need somebody who uses them, otherwise they'll rot really quickly. |
Motivation for this change
Seems like a useful extension.
This reverts commit ee5506c.
Further:
the first place was because it wasn't registered there.)
align with the other extensions.
npm, so keep the buildPhase unchanged.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)