kmix: init at 16.12.1 #22324

rongcuid · 2017-01-31T18:24:41Z

Motivation for this change

ALSA works, but KMix for KDE5 is not present. So I add it. I don't know how long I will keep using KDE or nix, so currently I didn't add myself as maintainer.

Note that I didn't test PulseAudio

Solves #22307

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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/)
Fits CONTRIBUTING.md.

mention-bot · 2017-01-31T18:24:42Z

@CarlDong, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ttuegel, @FRidh and @vandenoever to be potential reviewers.

peterhoeg · 2017-02-01T00:02:25Z

Further to our exchange on the referenced issue:

a) change the title of the PR and commit to "kmix: init at 16.12.1"

b) get rid of the commented out propagatedBuildInputs if they are not in use

c) you can add libpulseaudio and alsaLib to line 5 as well as buildInputs to get support for pulse/alsa.

To answer your question, if you don't specify the dependency, kmix will not know about it.

rongcuid · 2017-02-01T00:31:14Z

Can I put something like "pulseaudio or alsa" in build depends?

I will fix my commits, too.

peterhoeg · 2017-02-01T00:39:02Z

It is possible to specify parameters to the derivation in order to build different variants, but you still need to choose what the dependencies are in the default case - as mentioned, all dependencies have to be specified and the concept from other distros of "just build against whatever is on the machine" simply doesn't exist here. This is one of the (many) benefits!

Since both alsa and pulse are built for the normal KDE environment in NixOS, I suggest you add in both.

rongcuid · 2017-02-01T00:46:20Z

I don't quite understand yet... If I add in pulseaudio as a dep, wont't that pull in lots of pulse-stuff which clutter my system?

peterhoeg · 2017-02-01T01:02:56Z

pkgs/desktops/kde-5/applications/kmix.nix

+  ecm, kdoctools,
+  kglobalaccel, kxmlgui, kcoreaddons, kdelibs4support,
+  plasma-framework, pkgs
+}:


Don't pass in pkgs. Instead specify libpulseaudio, alsaLib.

peterhoeg · 2017-02-01T01:03:23Z

pkgs/desktops/kde-5/applications/kmix.nix

+        #maintainers = [ lib.maintainers.ttuegel ];
+      };
+      nativeBuildInputs = [ ecm kdoctools ];
+      buildInputs = with pkgs; [ libpulseaudio alsaLib ];


After changing it above, you can get rid of ``with pkgs; ```

peterhoeg · 2017-02-01T01:06:18Z

I don't quite understand yet... If I add in pulseaudio as a dep, wont't that pull in lots of pulse-stuff which clutter my system?

If you are running KDE, pulseaudio will already be pulled in.

rongcuid · 2017-02-01T01:07:09Z

Really? I never saw PA in my Backends... Only GStreamer and VLC.

ttuegel · 2017-02-01T01:42:04Z

Really? I never saw PA in my Backends... Only GStreamer and VLC.

GStreamer and VLC are Phonon backends; they decode media for Phonon. Phonon still needs a way to communicate with your hardware, either ALSA or PulseAudio.

If you are running KDE, pulseaudio will already be pulled in.

Only libpulseaudio, which is a small library, not the entire PulseAudio server.

Please do not add a PulseAudio dependency to kmix. PulseAudio users should use plasma-pa which is the replacement for kmix. It is installed by default when PulseAudio is enabled on NixOS.

ttuegel · 2017-02-01T01:42:32Z

pkgs/desktops/kde-5/applications/kmix.nix

+      name = "kmix";
+      meta = {
+        license = with lib.licenses; [ gpl2 lgpl21 fdl12 ];
+        #maintainers = [ lib.maintainers.ttuegel ];


Please add a maintainer.

I am trying to add myself as maintainer... The instruction in default.nix of kde-5/applications doesn't make sense at all.

rongcuid · 2017-02-01T05:47:30Z

I added myself as a maintainer. And, the dependency is libpulseaudio and alsaLib, so should be correct.

peterhoeg · 2017-02-01T06:06:28Z

You still need to add libpulseaudio and alsaLib to line 5.

rongcuid · 2017-02-01T06:07:52Z

Added... It works without, though

peterhoeg · 2017-02-01T06:10:27Z

Please also squash all the commits.

peterhoeg · 2017-02-01T06:13:11Z

And are you sure about the license(s)?

rongcuid · 2017-02-01T06:16:59Z

I am not sure. KMix website says GPL2(if my memory is right), but I copied that line from Konsole. I just assumed that they are the same...

I'll need to learn some more git to actually squash commits, though.

peterhoeg · 2017-02-01T06:17:02Z

Added... It works without, though

I just tried, it doesn't without.

cmake is mentioning an unmet optional dependency - canberra.

As that is already part of KDE anyway, please add libcanberra_kde to buildInput.

rongcuid · 2017-02-01T06:21:55Z

How do you test the nix file? I tried nix-build but got some problems about missing arguments.

And I will push the commit once I squash them

peterhoeg · 2017-02-01T06:30:27Z

KMix website says GPL2(if my memory is right),

Source also mentions gpl2.

How do you test the nix file? I tried nix-build but got some problems about missing arguments.

nix-build -A kde5.kmix while inside the checked-out nixpkgs.

OK, I got a rejection after squashing my commits

Yes, you will need to git push --force if you have squashed. If you've only been working on this from one machine, that's fine. It is indeed potentially dangerous if you are collaborating with others as you will overwrite their changes if you haven't pulled first.

peterhoeg · 2017-02-01T06:31:15Z

libcanberra_kde is still missing.

Signed-off-by: Rongcui Dong <rongcuid@outlook.com>

rongcuid · 2017-02-01T06:42:29Z

This commit should work, as I tested myself.

LnL7 · 2017-02-01T21:44:45Z

lib/maintainers.nix

@@ -414,6 +414,7 @@
  roblabla = "Robin Lambertz <robinlambertz+dev@gmail.com>";
  roconnor = "Russell O'Connor <roconnor@theorem.ca>";
  romildo = "José Romildo Malaquias <malaquias@gmail.com>";
+  rongcuid = "Rongcui Dong <rongcuid@outlook.com>";


It's the convention to use your github username as the attribute name.

Fixed it. By changing my github username

Oh... that also works 😀

LnL7 · 2017-02-01T21:58:44Z

Thanks!

rongcuid mentioned this pull request Jan 31, 2017

Attempted to add KMix for KDE 5 #22307

Closed

rongcuid changed the title ~~KMix, should be working~~ kmix: init at 16.12.1 Feb 1, 2017

peterhoeg reviewed Feb 1, 2017

View reviewed changes

ttuegel reviewed Feb 1, 2017

View reviewed changes

rongcuid force-pushed the kmix branch from 4b3ab68 to 65e1bb9 Compare February 1, 2017 06:29

kmix: init at 16.12.1

c1250e9

Signed-off-by: Rongcui Dong <rongcuid@outlook.com>

rongcuid force-pushed the kmix branch from 65e1bb9 to c1250e9 Compare February 1, 2017 06:42

LnL7 reviewed Feb 1, 2017

View reviewed changes

LnL7 approved these changes Feb 1, 2017

View reviewed changes

LnL7 merged commit bc735d4 into NixOS:master Feb 1, 2017

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kmix: init at 16.12.1 #22324

kmix: init at 16.12.1 #22324

rongcuid commented Jan 31, 2017 •

edited

mention-bot commented Jan 31, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg Feb 1, 2017

peterhoeg Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

ttuegel commented Feb 1, 2017

ttuegel Feb 1, 2017

rongcuid Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

LnL7 Feb 1, 2017

rongcuid Feb 1, 2017

LnL7 Feb 1, 2017

LnL7 commented Feb 1, 2017

kmix: init at 16.12.1 #22324

kmix: init at 16.12.1 #22324

Conversation

rongcuid commented Jan 31, 2017 • edited

Motivation for this change

Things done

mention-bot commented Jan 31, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg Feb 1, 2017

Choose a reason for hiding this comment

peterhoeg Feb 1, 2017

Choose a reason for hiding this comment

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

ttuegel commented Feb 1, 2017

ttuegel Feb 1, 2017

Choose a reason for hiding this comment

rongcuid Feb 1, 2017

Choose a reason for hiding this comment

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

peterhoeg commented Feb 1, 2017

rongcuid commented Feb 1, 2017

LnL7 Feb 1, 2017

Choose a reason for hiding this comment

rongcuid Feb 1, 2017

Choose a reason for hiding this comment

LnL7 Feb 1, 2017

Choose a reason for hiding this comment

LnL7 commented Feb 1, 2017

rongcuid commented Jan 31, 2017 •

edited