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

plasma-applet-volumewin7mixer: init at v23 #46045

Merged
merged 1 commit into from Sep 30, 2018
Merged

plasma-applet-volumewin7mixer: init at v23 #46045

merged 1 commit into from Sep 30, 2018

Conversation

mdevlamynck
Copy link
Contributor

Motivation for this change

Adds a volume mixer applet for plasma https://github.com/Zren/plasma-applet-volumewin7mixer.

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.

lschuermann
lschuermann reviewed Sep 4, 2018
View reviewed changes
pkgs/applications/misc/plasma-applet-volumewin7mixer/default.nix Outdated
@@ -0,0 +1,27 @@
{ stdenv, cmake, extra-cmake-modules, plasma-framework, kwindowsystem, fetchFromGitHub, }:

let version = "23"; in
Copy link
Member

Choose a reason for hiding this comment

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

Most packages integrate the version attribute into the attribute set passed to mkDerivation. This can be done by writing rec before the attribute set.

stdenv.mkDerivation rec {
  name = "plasma-applet-volumewin7mixer-${version}";
  version = "23";

Also you should interpolate the version into the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed ;)

lschuermann
lschuermann reviewed Sep 4, 2018
View reviewed changes
pkgs/applications/misc/plasma-applet-volumewin7mixer/default.nix Outdated
meta = with stdenv.lib; {
description = "A fork of the default volume plasmoid with a Windows 7 theme (vertical sliders).";
homepage = "https://github.com/Zren/plasma-applet-volumewin7mixer";
license = licenses.gpl3;
Copy link
Member

Choose a reason for hiding this comment

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

I can't find a LICENSE file in the repository, or some other indication that its licensed under the terms and conditions of the GPL-3.

It does say GPL-2 or later on the KDE store page, and some files have a GPL-2 / GPL-2.1 or later header. I'm not quite sure whether this is enough to say that the whole package is licensed under the GPL-3.

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 assumed the license from the packages for other distros mentioned in the README (archlinux and chakralinux).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a search on what GPL means on archlinux, it should probably be gpl2Plus.

Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice if the license is clearly indicated in the source repository. I don't know what this situation means in the context of nixpkgs, maybe some maintainer like @Mic92 can help out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this package is a fork of the original volume mixer applet of the plasma desktop (which is GPL2), I guess the license should stay the same right? I'll open an issue in the original project to confirm what the license is, to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Zren/plasma-applet-volumewin7mixer#3 (comment) Yep, this is GPL2.

@mdevlamynck mdevlamynck mentioned this pull request Sep 5, 2018
Closed
matthewbauer
matthewbauer reviewed Sep 29, 2018
View reviewed changes
pkgs/applications/misc/plasma-applet-volumewin7mixer/default.nix Outdated
@@ -0,0 +1,26 @@
{ stdenv, cmake, extra-cmake-modules, plasma-framework, kwindowsystem, plasma-pa, fetchFromGitHub, }:
Copy link
Member

Choose a reason for hiding this comment

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

Should remove the trailing comma here

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.

@matthewbauer
Copy link
Member

@GrahamcOfBorg build plasma-applet-volumewin7mixer

@GrahamcOfBorg
Copy link

No attempt on x86_64-darwin (full log)

The following builds were skipped because they don't evaluate on x86_64-darwin: plasma-applet-volumewin7mixer

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: plasma-applet-volumewin7mixer

Partial log (click to expand)

-- Installing: /nix/store/qribdr5syka9msz3vd8qabygs9vdmk53-plasma-applet-volumewin7mixer-23/share/appdata/org.kde.plasma.volumewin7mixer.appdata.xml
-- Installing: /nix/store/qribdr5syka9msz3vd8qabygs9vdmk53-plasma-applet-volumewin7mixer-23/share/kservices5/plasma-applet-org.kde.plasma.volumewin7mixer.desktop
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/qribdr5syka9msz3vd8qabygs9vdmk53-plasma-applet-volumewin7mixer-23
strip is /nix/store/y4ymnvgxygpq05h03kyzbj572zmh6zla-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/qribdr5syka9msz3vd8qabygs9vdmk53-plasma-applet-volumewin7mixer-23
checking for references to /build in /nix/store/qribdr5syka9msz3vd8qabygs9vdmk53-plasma-applet-volumewin7mixer-23...
postPatchMkspecs
/nix/store/qribdr5syka9msz3vd8qabygs9vdmk53-plasma-applet-volumewin7mixer-23

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: plasma-applet-volumewin7mixer

Partial log (click to expand)

-- Installing: /nix/store/zlxigv9nd8w9hz33ch9v7kxq636brqpl-plasma-applet-volumewin7mixer-23/share/appdata/org.kde.plasma.volumewin7mixer.appdata.xml
-- Installing: /nix/store/zlxigv9nd8w9hz33ch9v7kxq636brqpl-plasma-applet-volumewin7mixer-23/share/kservices5/plasma-applet-org.kde.plasma.volumewin7mixer.desktop
glibPreFixupPhase
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/zlxigv9nd8w9hz33ch9v7kxq636brqpl-plasma-applet-volumewin7mixer-23
strip is /nix/store/h0lbngpv6ln56hjj59i6l77vxq25flbz-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/zlxigv9nd8w9hz33ch9v7kxq636brqpl-plasma-applet-volumewin7mixer-23
checking for references to /build in /nix/store/zlxigv9nd8w9hz33ch9v7kxq636brqpl-plasma-applet-volumewin7mixer-23...
postPatchMkspecs
/nix/store/zlxigv9nd8w9hz33ch9v7kxq636brqpl-plasma-applet-volumewin7mixer-23

@matthewbauer matthewbauer merged commit aaef04f into NixOS:master Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@lschuermann lschuermann lschuermann left review comments

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mdevlamynck @matthewbauer @GrahamcOfBorg @lschuermann @Janik-Haag