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

gnomeExtensions.volume-mixer: bring it back and fix it #87481

Closed
wants to merge 2 commits into from

Conversation

bjornfor
Copy link
Contributor

Motivation for this change

Seems like a useful extension.

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.
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 nixpkgs-review --run "nixpkgs-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.

meta = with stdenv.lib; {
description = "GNOME Shell Extension allowing separate configuration of PulseAudio devices";
license = licenses.gpl2;
maintainers = with maintainers; [ aneeshusa ];
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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

@stale
Copy link

stale bot commented Nov 17, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md and removed 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Nov 17, 2020
src = fetchFromGitHub {
owner = "aleho";
repo = "gnome-shell-volume-mixer";
rev = "${version}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rev = "${version}";
rev = version;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 87481 run on x86_64-linux 1

1 package built:
  • gnomeExtensions.volume-mixer

@SuperSandro2000
Copy link
Member

@bjornfor
Copy link
Contributor Author

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 pkgs/desktops/gnome-3/extensions/volume-mixer.nix anywhere now.

@bjornfor
Copy link
Contributor Author

I don't see pkgs/desktops/gnome-3/extensions/volume-mixer.nix anywhere now.

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.

@bjornfor bjornfor force-pushed the gnome-extension-volume-mixer branch 3 times, most recently from fff4019 to da8c501 Compare November 23, 2020 22:08
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.
];

buildInputs = [
glib
Copy link
Contributor

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.

Comment on lines +25 to +26
substituteInPlace ./shell-volume-mixer@derhofbauer.at/pautils/pa.py \
--subst-var-by pulseaudio ${pulseaudio}
Copy link
Contributor

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
Copy link
Contributor

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...
Copy link
Contributor

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}
Copy link
Contributor

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}
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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

@piegamesde
Copy link
Member

volume-mixer has been packaged in nixpkgs as of #118232. Unsurprisingly however, it does not work. I will have a look at a good way forward how to allow manual fixes for automatically packaged extensions.

@piegamesde
Copy link
Member

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

@bjornfor
Copy link
Contributor Author

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

@piegamesde
Copy link
Member

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.

@piegamesde piegamesde closed this Sep 11, 2021
@bjornfor bjornfor deleted the gnome-extension-volume-mixer branch November 5, 2022 15:20
@bjornfor bjornfor restored the gnome-extension-volume-mixer branch April 4, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants