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

nicotine-plus: 1.4.1 -> unstable-2020-05-24 #86683

Closed
wants to merge 3 commits into from
Closed

nicotine-plus: 1.4.1 -> unstable-2020-05-24 #86683

wants to merge 3 commits into from

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented May 3, 2020 •
edited

Motivation for this change

Includes port to Python 3 and GTK+ 3/PyGObject; see nicotine-plus/nicotine-plus#106 for details.

This unstable version identifies itself as 1.4.3, and is planned to be released as 2.0.0 after it has had more fixes and testing through the official upstream PPA. The codebase is old and flaky to begin with, and there have been several unrelated bug fixes in the years since the last stable release, so I think it's worth updating to be able to use non-obsolescent versions of the core dependencies, and have nixos-unstable users help shake out any remaining issues with the port. I verified that searching and downloading seem to work fine.

Drop the rename of the executable to nicotine-plus, as the test depends on the executable name and there doesn't seem to be any strong reason to diverge from upstream here (the original Nicotine is long dead). This is technically a breaking change, so it might be best to include a symlink at $out/bin/nicotine-plus temporarily.

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.

@emilazy emilazy marked this pull request as draft May 4, 2020 12:32
@emilazy
Copy link
Member Author

emilazy commented May 4, 2020

Upstream doesn't want this packaged yet.

@emilazy emilazy changed the title nicotine-plus: 1.4.1 -> 1.4.3 nicotine-plus: 1.4.1 -> unstable-2020-05-18 May 18, 2020
@emilazy
Copy link
Member Author

emilazy commented May 18, 2020

Updated; the integration tests now run successfully. Marking as ready for review per nicotine-plus/nicotine-plus#99 (comment) :)

@emilazy emilazy marked this pull request as ready for review May 18, 2020 18:42
@ofborg ofborg bot requested a review from klntsky May 18, 2020 18:55
@emilazy emilazy mentioned this pull request May 18, 2020
Closed
@emilazy
Copy link
Member Author

emilazy commented May 18, 2020 •
edited

@GrahamcOfBorg build nicotine-plus

@emilazy
Copy link
Member Author

emilazy commented May 18, 2020

Simplified with strictDeps = false at the suggestion of @jtojnar.

@Ericson2314 I guess with gobject-introspection, Python, and checkPhase this is already a mess for cross-compilation, but disabling strictDeps makes me a little anxious; is there a better way to get setup hooks running for (install)CheckPhase? Previously none of the setup hooks for gdk-pixbuf and similar were running.

@ofborg ofborg bot requested a review from klntsky May 18, 2020 20:48
@emilazy emilazy force-pushed the nicotine-plus-1.4.3 branch 2 times, most recently from 608034a to 25681b4 Compare May 18, 2020 22:18
@emilazy
Copy link
Member Author

emilazy commented May 18, 2020

Bumped upstream commit; it now uses the gobject-introspection libnotify bindings.

@emilazy
Copy link
Member Author

emilazy commented May 18, 2020

Pushed with some cleanups and optimistically enabling xvfb_run on Darwin in the hopes that that will make the build and tests work (xorg.xorgserver is supported there, after all). Let's see what ofborg thinks of it.

@emilazy
xvfb_run: fix on darwin
34b9541 
utillinux is used only for mcookie, which can be replaced with `openssl
rand -hex 16`, per https://bugs.freedesktop.org/show_bug.cgi?id=7052.
@emilazy
pythonPackages.miniupnpc: inherit from miniupnpc_2
637df7c 
This should hopefully fix the build on Darwin.
@emilazy emilazy force-pushed the nicotine-plus-1.4.3 branch 2 times, most recently from f933d80 to b8834c6 Compare May 19, 2020 01:03
@emilazy
Copy link
Member Author

emilazy commented May 19, 2020

This now builds on Darwin, though I can't test it. The xvfb_run portability change is no longer required for that, as we can't run the integration tests anyway due to gtk3 not supporting X11 by default on Darwin, so I can drop it if it's undesired.

@emilazy emilazy changed the title nicotine-plus: 1.4.1 -> unstable-2020-05-18 nicotine-plus: 1.4.1 -> unstable-2020-05-19 May 19, 2020
@emilazy
Copy link
Member Author

emilazy commented May 19, 2020 •
edited

Updated with new upstream commits, some licensing pedantry, and a fix to the installation code that makes the desktop entry and manual page work.

There's now an exception being thrown in the integration tests (that somehow doesn't make them fail?) that I don't fully understand and should investigate further at some point:

nicotine-plus-unstable> Run nicotine as shell command                                         Traceback (most recent call last):
nicotine-plus-unstable> File "/nix/store/gpl8jbk7r4p9m7b9jmk5s7jym39p5cy2-nicotine-plus-unstable-2020-05-19/bin/.nicotine-wrapped", line 277, in <module>
nicotine-plus-unstable> run()
nicotine-plus-unstable> File "/nix/store/gpl8jbk7r4p9m7b9jmk5s7jym39p5cy2-nicotine-plus-unstable-2020-05-19/bin/.nicotine-wrapped", line 253, in run
nicotine-plus-unstable> app = frame.MainApp(data_dir, config, plugins, trayicon, hidden, bindip, port)
nicotine-plus-unstable> File "/nix/store/gpl8jbk7r4p9m7b9jmk5s7jym39p5cy2-nicotine-plus-unstable-2020-05-19/lib/python3.7/site-packages/pynicotine/gtkgui/frame.py", line 3730, in __init__
nicotine-plus-unstable> self.frame = NicotineFrame(data_dir, config, plugins, trayicon, start_hidden, bindip, port)
nicotine-plus-unstable> File "/nix/store/gpl8jbk7r4p9m7b9jmk5s7jym39p5cy2-nicotine-plus-unstable-2020-05-19/lib/python3.7/site-packages/pynicotine/gtkgui/frame.py", line 517, in __init__
nicotine-plus-unstable> self.downloads = Downloads(self)
nicotine-plus-unstable> File "/nix/store/gpl8jbk7r4p9m7b9jmk5s7jym39p5cy2-nicotine-plus-unstable-2020-05-19/lib/python3.7/site-packages/pynicotine/gtkgui/downloads.py", line 100, in __init__
nicotine-plus-unstable> cols[i].set_visible(self.frame.np.config.sections["columns"]["downloads_columns"][i])
nicotine-plus-unstable> IndexError: list index out of range

But it seems to work fine outside the build.

@mathiascode
Copy link

@emilazy Some column counts were outdated in the config file. nicotine-plus/nicotine-plus#134 should take care of the issue.

@emilazy emilazy force-pushed the nicotine-plus-1.4.3 branch 3 times, most recently from 1eeb74d to 7596ba7 Compare May 22, 2020 17:06
@emilazy emilazy mentioned this pull request May 22, 2020
Open
15 tasks
@emilazy
nicotine-plus: 1.4.1 -> unstable-2020-05-24
d4940ee 
Includes port to Python 3 and GTK+ 3/PyGObject; see
nicotine-plus/nicotine-plus#106 for details.

This unstable version identifies itself as 1.4.3, and is planned to be
released as 2.0.0 after it has had more fixes and testing through the
official upstream PPA. The codebase is old and flaky to begin with,
and there have been several unrelated bug fixes in the years since
the last stable release, so I think it's worth updating to be able
to use non-obsolescent versions of the core dependencies, and have
nixos-unstable users help shake out any remaining issues with the port.
I verified that searching and downloading seem to work fine.

Drop the rename of the executable to `nicotine-plus`, as the test
depends on the executable name and there doesn't seem to be any strong
reason to diverge from upstream here (the original Nicotine is long
dead). This is technically a breaking change, so it might be best to
include a symlink at `$out/bin/nicotine-plus` temporarily.
@emilazy emilazy changed the title nicotine-plus: 1.4.1 -> unstable-2020-05-19 nicotine-plus: 1.4.1 -> unstable-2020-05-24 May 24, 2020
@mathiascode
Copy link

Nicotine+ 2.0.0 has been released.

@mathiascode
Copy link

Nicotine+ 2.0.1 has been released.

SuperSandro2000
SuperSandro2000 requested changes Nov 29, 2020
View reviewed changes
pkgs/applications/networking/soulseek/nicotine-plus/default.nix

python27Packages.buildPythonApplication {
python3.pkgs.buildPythonApplication {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python3.pkgs.buildPythonApplication {
python3Packages.buildPythonApplication {

pkgs/applications/networking/soulseek/nicotine-plus/default.nix
rev = "4e057d64184885c63488d4213ade3233bd33e67b";
sha256 = "11j2qm67sszfqq730czsr2zmpgkghsb50556ax1vlpm7rw3gm33c";
rev = "0f9e4e1c2391196d4070df5851ff48601aceaf7f";
hash = "sha256-kSDqoxmX9beqL98tMQylbhCZVMmQy6/oVOyW1xnxFLY=";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hash = "sha256-kSDqoxmX9beqL98tMQylbhCZVMmQy6/oVOyW1xnxFLY=";
sha256 = "sha256-kSDqoxmX9beqL98tMQylbhCZVMmQy6/oVOyW1xnxFLY=";

pkgs/applications/networking/soulseek/nicotine-plus/default.nix
(GeoIP.override { inherit geoip; })
];

# Insert real docs directory.
# os.getcwd() is not needed
checkInputs = [ python3.pkgs.pytest ] ++ lib.optionals (!stdenv.isDarwin) [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
checkInputs = [ python3.pkgs.pytest ] ++ lib.optionals (!stdenv.isDarwin) [
checkInputs = [ python3Packages.pytest ] ++ lib.optionals (!stdenv.isDarwin) [

pkgs/applications/networking/soulseek/nicotine-plus/default.nix
# The gtk3 package doesn't enable X11 on Darwin, so we can't
# use xvfb-run.
xvfb_run
python3.pkgs.robotframework
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python3.pkgs.robotframework
python3Packages.robotframework

pkgs/applications/networking/soulseek/nicotine-plus/default.nix
substituteInPlace ./pynicotine/gtkgui/frame.py \
--replace "paths.append(os.getcwd())" "paths.append('"$out"/doc')"
# Remove non-free files.
sh debian/nicotine-rm-nonfree
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sh debian/nicotine-rm-nonfree
sh debian/nicotine-rm-nonfree

Please reference bash from nixpkgs.

pkgs/applications/networking/soulseek/nicotine-plus/default.nix
Comment on lines +98 to +99
robot test/integration/nicotine.robot

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
robot test/integration/nicotine.robot
robot test/integration/nicotine.robot

pkgs/applications/networking/soulseek/nicotine-plus/default.nix
robot test/integration/nicotine.robot

'' + ''
runHook postInstallCheck
'';

meta = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = {
meta = lib; {

pkgs/applications/networking/soulseek/nicotine-plus/default.nix
Comment on lines +109 to +114
lib.licenses.gpl3Plus
# Per sounds/default/license.txt
lib.licenses.cc0
];
maintainers = with lib.maintainers; [ klntsky emily ];
platforms = lib.platforms.unix;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lib.licenses.gpl3Plus
# Per sounds/default/license.txt
lib.licenses.cc0
];
maintainers = with lib.maintainers; [ klntsky emily ];
platforms = lib.platforms.unix;
licenses.gpl3Plus
# Per sounds/default/license.txt
licenses.cc0
];
maintainers = with maintainers; [ klntsky emily ];
platforms = platforms.unix;

pkgs/development/python-modules/miniupnpc/default.nix

meta = with stdenv.lib; {
meta = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = {
meta = with lib; {

pkgs/development/python-modules/miniupnpc/default.nix
Comment on lines +11 to +12
license = lib.licenses.mit;
maintainers = with lib.maintainers; [ peterhoeg ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
license = lib.licenses.mit;
maintainers = with lib.maintainers; [ peterhoeg ];
license = licenses.mit;
maintainers = ib.maintainers; [ peterhoeg ];

@stale
Copy link

stale bot commented Jun 2, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2021
@peterhoeg
Copy link
Member

We already have 3.0.6 in master.

@peterhoeg peterhoeg closed this Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

@klntsky klntsky Awaiting requested review from klntsky

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: python 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@emilazy @mathiascode @peterhoeg @SuperSandro2000 @klntsky