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

Jump to bottom

[WIP] mpd: 0.20.23 -> 0.21.3 #51766

Closed
wants to merge 2 commits into from
Closed

[WIP] mpd: 0.20.23 -> 0.21.3 #51766

wants to merge 2 commits into from

Conversation

pbogdan
Copy link
Member

@pbogdan pbogdan commented Dec 9, 2018

https://raw.githubusercontent.com/MusicPlayerDaemon/MPD/v0.21.3/NEWS

Motivation for this change

Update to the latest release.

(Among other changes) upstream switched the build system to meson, and as neither the flag names nor semantics of enabling / disabling flags match the autoconf build this makes the diff rather large.

Also since I had to go through meson build options anyway to map the current features to their new flag names I added a bunch of new flags. Probably doesn't make sense to keep all of them but figured might show what's available in the new build system. Feature-wise this build should match the current derivation and shouldn't enable / disable any new features.

I also may have inadvertently broken support for platforms other that x86_64-linux as I'm unable to test outside that platform.

Finally, this isn't tested heavily. My own personal setup is extremely simple and I'm not even sure how to check many of the features (that are enabled by default).

Anyway, had these changes sitting around for a while so figured might open a PR in case it's helpful at all and helps with the update. Should at least take care of the tediousness of mapping things to the new build system.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@pbogdan
Copy link
Member Author

pbogdan commented Dec 9, 2018

@GrahamcOfBorg build mpd

Mic92
Mic92 reviewed Dec 9, 2018
View reviewed changes
nixos/modules/services/audio/mpd.nix
@@ -144,6 +146,7 @@ in {
###### implementation

config = mkIf cfg.enable {
environment.systemPackages = [ pkgs.mpd ];
Copy link
Member

Choose a reason for hiding this comment

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

Why is this required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not strictly required but it installs the man pages and the documentation which I thought might be useful.

@Mic92
Copy link
Member

Mic92 commented Dec 9, 2018

@GrahamcOfBorg build mpd

infinisil
infinisil reviewed Dec 9, 2018
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Tbh I'm not a big fan of having a hundred options for configuring packages. Why not just have a minimal and a full version? Nix doesn't need to reproduce every possible option in the build system. I know other (popular?) nixpkgs packages do this too and I wonder why.

@Mic92
Copy link
Member

Mic92 commented Dec 9, 2018 •
edited

Yeah it should be possible to use overrideAttrs to set mesonFlags and additional dependencies yourself. Also how did you test all these combination of flags do even work?

@pbogdan
Copy link
Member Author

pbogdan commented Dec 10, 2018

@infinisil I'm not too fond of that either but stuck with the existing style / convention of the existing expression. Personally I wouldn't be even opposed to removing them all and just providing mpd with the feature set matching current default configuration though I suppose some users may rely on some flags being present in the current expression?

@Mic92

Also how did you test all these combination of flags do even work?

I have not tested every possible permutation but for each flag I checked that at least the configure phase finds the required dependencies and proceeds to build. Enabling all of the features does result in a build failure (chromaprint support and one other feature that I can't remember right now would need fixing) but I didn't look into it too hard as I wasn't sure we will want to actually keep it.
The default configuration does work for me but as I said my personal use case is very simple.

@pbogdan pbogdan closed this Dec 21, 2018
@tobim tobim mentioned this pull request Jul 29, 2019
Merged
10 tasks
@pbogdan pbogdan deleted the mpd-update branch December 3, 2019 17:07
@jtojnar jtojnar mentioned this pull request May 16, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@infinisil infinisil infinisil left review comments

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pbogdan @Mic92 @infinisil @GrahamcOfBorg