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
libsidplayfp: 1.8.7 -> 2.0.4, sidplayfp: 1.4.4 -> 2.0.2, fix API options #82674
Conversation
e07ff64
to
34b59a4
Compare
PR updated to 2.0.2. |
34b59a4
to
b912800
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/175 |
b912800
to
c7b2019
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
version = "1.4.4"; | ||
let | ||
|
||
opt = lib.optional; |
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 would like to keep this as consistent as possible with other derivations
opt = lib.optional; | |
optional = lib.optional; |
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'll replace this and all the lib.
s in the library with inherit (lib)
s.
, alsaSupport ? stdenv.isLinux, alsaLib | ||
, pulseSupport ? stdenv.isLinux, libpulseaudio | ||
}: |
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.
not sure if this is needed in this instance, but it's common to do:
assert alsaSupport -> alsaLib;
assert alsaSupport -> libpulseaudio;
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.
Why exactly is this common? I don't quite get the point of it, maybe I'm missing something. If it's e.g. to protect against alsaLib being unavailable for some platform wouldn't alsaSupport = false;
be enough for that?
Edit: I'm trying to understand when and why I should incorporate this more into other derivations of mine as well.
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 usually see this in packages which reference the libraries in question. E.g:
buildInputs = [ (some-lib.override{ enablePython=true; }) ]'
but it's not always guarenteed that python will be available.
Either way, I don't see a case where it will fail on linux, but might be useful for a darwin user that an assumption is broken.
c7b2019
to
6b25d2e
Compare
6b25d2e
to
ef8ecea
Compare
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.
LGTM
shows usage
[5 built, 377 copied (1425.6 MiB), 322.3 MiB DL]
https://github.com/NixOS/nixpkgs/pull/82674
4 packages built:
audacious gjay libsidplayfp sidplayfp
Motivation for this change
New versions available.
sidplayfp
would only try to use OSS for playback, fixed that.cc @RamKromberg @dezgeg
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)I haven't tested
stilview
in the sidplayfp package, don't know how to use thatnix path-info -S
before and after)sidplayfp: 34679176 -> 74860048
libsidplayfp: 34491888 -> 39865888