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

beets: 1.4.9 -> unstable-2020-12-22 #108275

Merged
merged 4 commits into from Jan 3, 2021
Merged

beets: 1.4.9 -> unstable-2020-12-22 #108275

merged 4 commits into from Jan 3, 2021

Conversation

lovesegfault
Copy link
Member

@lovesegfault lovesegfault commented Jan 3, 2021 •
edited

Motivation for this change

During #90504 it became clear that bs1770gain is a menace to our users, given that it comes bundled with white supremacist messages. The only user of it is beets, which requires it for replaygain functionality in version 1.4.9.

The current HEAD of beets has added support for using ffmpeg for replaygain, and so this updates beets to the HEAD which removes the dependency on bs1770gain altogether. We also remove bs1770gain from the tree, as it now has no maintainers and no reverse dependencies.

I attempted to copy the stable drv as closely as possible, any cleanups and the like will follow in a later PR.

cc. @flokli, @Lassulus, @doronbehar @mweinelt

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.

@ofborg ofborg bot requested review from aszlig, domenkozar and pjones January 3, 2021 00:15
@flokli
Copy link
Contributor

flokli commented Jan 3, 2021

Mhhmm… Any chance we could just cherry-pick https://github.com/beetbox/beets/pull/3056/files?

@lovesegfault
Copy link
Member Author

Mhhmm… Any chance we could just cherry-pick https://github.com/beetbox/beets/pull/3056/files?

We could if all we wanted was to get rid of bs1770gain. There is, however, another big issue here which is beets basically doesn't do releases anymore, so regardless we should have a beetsUnstable package that tracks master.

It's pretty much the only way of having up-to-date beets at this point.

@lovesegfault
Copy link
Member Author

Talked to @flokli on IRC. We agreed that it's best to just package beets as unstable until they do another release, instead of packaging stable and unstable, as that's a lot more work.

@lovesegfault lovesegfault changed the title beetsUnstable: init at 2020-12-22 beets: 1.4.9 -> unstable-2020-12-22 Jan 3, 2021
@SuperSandro2000
Copy link
Member

Mhhmm… Any chance we could just cherry-pick beetbox/beets#3056 (files)?

FYI #90504 (comment)

SuperSandro2000
SuperSandro2000 reviewed Jan 3, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

After you moved the plugins we now have two times plugins in the path. I think it should be dropped from the nix file.

@thiagokokada
Copy link
Contributor

thiagokokada commented Jan 3, 2021 •
edited

Result of nixpkgs-review pr 108275 1

1 package built:
  • beets

Code-wise LGTM, builds and runs (didn't test any feature though). Nice to see the community taking care of this issues seriously too.

@lovesegfault
Copy link
Member Author

After you moved the plugins we now have two times plugins in the path. I think it should be dropped from the nix file.

Done :)

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

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

1 package built:
  • beets

doronbehar
doronbehar requested changes Jan 3, 2021
View reviewed changes
pkgs/tools/audio/beets/default.nix Outdated Show resolved Hide resolved
pkgs/tools/audio/beets/default.nix Show resolved Hide resolved
pkgs/tools/audio/beets/default.nix Outdated Show resolved Hide resolved
pkgs/tools/audio/beets/default.nix Outdated Show resolved Hide resolved
pkgs/tools/audio/beets/default.nix Outdated Show resolved Hide resolved
@lovesegfault
Copy link
Member Author

@doronbehar My goal with this PR is to get beets master working and in an acceptable state, not to undertake an overhaul of the drv. While I agree that it's needed, it's for a separate PR.

(Just to clarify why I rejected/marked as resolved the last round of comments)

@doronbehar
Copy link
Contributor

@doronbehar My goal with this PR is to get beets master working and in an acceptable state, not to undertake an overhaul of the drv. While I agree that it's needed, it's for a separate PR.

Of course, but some of the sed commands are not updated and in fact they do nothing. I pushed a fixup commit, which I'd like you to review :).

@lovesegfault
Copy link
Member Author

@doronbehar My goal with this PR is to get beets master working and in an acceptable state, not to undertake an overhaul of the drv. While I agree that it's needed, it's for a separate PR.

Of course, but some of the sed commands are not updated and in fact they do nothing. I pushed a fixup commit, which I'd like you to review :).

Tests pass, all looks good to me :)

Added you as co-author while rebasing.

lovesegfault and others added 4 commits January 3, 2021 14:36
@lovesegfault @doronbehar
beets: 1.4.9 -> unstable-2020-12-22
0faf72a 
The maintainer has stopped cutting releases[1]. Since the last release,
1.4.9, includes a dependency that is filled with hate speech[2] it's all
the more reason to package the unstable version and eliminate that
requirement.

Moreover a number of fixes, improvements, and features have landed
since.

[1]: beetbox/beets#3625
[2]: NixOS#90504

Co-authored-by: Doron Behar <doron.behar@gmail.com>
@lovesegfault @doronbehar
beets: add lovesegfault to maintainers
264b77f
@lovesegfault @doronbehar
beets: add doronbehar to maintainers
cea2b9e
@lovesegfault @doronbehar
bs1770gain: nuke
264f798 
The package is filled with white supremacist hate speech, and the only
reverse dependency on it, beets, can now use ffmpeg instead.
@doronbehar
Copy link
Contributor

Fixed a small comment badly written in the expression.

@doronbehar
Copy link
Contributor

It'd be nice to get a 👍 from a Darwin user for this, to make sure the darwin build succeeds as well. Also, I don't have currently at hand a functional beets library to test that basic commands work, but I guess we should count upon the tests. Also, for the record, the files in ./result show up that substituteAll patches have worked.

@ofborg ofborg bot requested a review from doronbehar January 3, 2021 12:46
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

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

1 package built:
  • beets

@lovesegfault lovesegfault merged commit 77d190f into NixOS:master Jan 3, 2021
@lovesegfault lovesegfault deleted the beetsUnstable branch January 3, 2021 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagokokada thiagokokada thiagokokada approved these changes

@aszlig aszlig Awaiting requested review from aszlig

@domenkozar domenkozar Awaiting requested review from domenkozar

@pjones pjones Awaiting requested review from pjones

@mweinelt mweinelt Awaiting requested review from mweinelt

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@doronbehar doronbehar Awaiting requested review from doronbehar

Assignees
No one assigned
Labels
8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lovesegfault @flokli @SuperSandro2000 @thiagokokada @doronbehar