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

deluge 2.0.3 and libtorrentRasterbar 1.2.5 #77762

Merged
5 commits merged into from Apr 18, 2020
Merged

deluge 2.0.3 and libtorrentRasterbar 1.2.5 #77762

5 commits merged into from Apr 18, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 15, 2020

Motivation for this change

Alternative to #64496 and #64542

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.

@dtzWill
Copy link
Member

dtzWill commented Jan 15, 2020

Thank you! Sorry for not following-up on my PR 🐈 .

On that thread there was discussion from some folks experiencing missing icons (quite possibly fixed with a rebase or changes you've made), so hopefully that's not a problem still.... but the other detail was ensuring the NixOS service module is fixed/updated accordingly. I don't know anything about that.

As a start, let's see what running the test does:

@GrahamcOfBorg test deluge

@ghost
Copy link
Author

ghost commented Jan 15, 2020

Tests are not working currently.
Icon issue is still there when deluge is run from nix-shell, I will have a look.
I hope we can make it so that users can just decide what deluge version to use by setting a package option. Not sure if it is necessary to run them in parallel.

@ghost
Copy link
Author

ghost commented Jan 15, 2020

I think the best option for the service might be to use the stateVersion to determine the default package. This is because upgrading changes the application state irreversably, so we want users who are currently using 1.x to make a conscious choice on that.
On the other hand we want new users to get the latest version by default.

@peterhoeg
Copy link
Member

The nixos module requires some serious work for deluge 2.

I have a WIP branch here https://github.com/peterhoeg/nixpkgs/commits/u/deluge that might help you.

@jtojnar jtojnar mentioned this pull request Jan 26, 2020
10 tasks
@svalaskevicius
Copy link
Contributor

svalaskevicius commented Jan 26, 2020

Do the toolbar icons appear in this pr? I had to add adwaita icon theme for them in my pr (I didn't see this one before)
edit : I'm not running gnome, just xmonad, so they're not available by default

@svalaskevicius
Copy link
Contributor

Oh and hicolor icon theme as well for the logs to stop spamming about missing icons

@ghost
Copy link
Author

ghost commented Feb 17, 2020

Does anyone still need to run deluge 1.x? I just rechecked all my trackers and it seems like they all support deluge 2.x now, so I have no need for running the old version.

@peterhoeg
Copy link
Member

The nixos module also needs to support this before we can merge it.

@ghost
Copy link
Author

ghost commented Feb 20, 2020

That's why I'm asking, there is still significant work to be done to support both versions in parallel, which would be unnecessary if noone is using it.
But yes, the declarative configuration for deluge 2.0 something I have not taken a closer look yet, but I will do.

@peterhoeg
Copy link
Member

That's why I'm asking, there is still significant work to be done to support both versions in parallel, which would be unnecessary if noone is using it.

If I were you, I definitely wouldn't try supporting both in tandem considering how much has changed - it would be a ton of work for very little gain.

@mweinelt
Copy link
Member

Icons are working as soon as I add gnome3.adwaita-icon-theme to my environment. That fixes icons in a lot of other apps and according to https://nixos.wiki/wiki/GNOME we are on our own to fix them if we're not actually using GNOME.

@ghost
Copy link
Author

ghost commented Mar 19, 2020

Fixed module and test for deluge 2.x while preserving support for deluge 1.x

@ghost
Copy link
Author

ghost commented Mar 19, 2020

Do we want to fix the icon thing? I think I saw a comment somewhere that users are expected to have one icon theme in their environment according to the manual, so this would not strictly be a bug.

Also, could users still change the icon theme used by deluge by adding one to their environment if we include one?

@mweinelt
Copy link
Member

mweinelt commented Mar 20, 2020 via email

@ghost
Copy link
Author

ghost commented Mar 27, 2020

I moved this to my overlay and am using it on all my machines. From my side this is ready to be merged.

@peterhoeg
Copy link
Member

@petabyteboy, do you mind fixing up the merge conflicts so we can get this merged?

@ghost
Copy link
Author

ghost commented Apr 8, 2020

Feel free to do so. I'm running this since two weeks. Other than concerns about data migration it should be good to go. The topic of data migration is solved sufficiently for me, but others might have different standards.

@peterhoeg
Copy link
Member

@bricewge raised some issues though.

Would also be great to hear from maintainers @domenkozar @ebzzry

@ghost
Copy link
Author

ghost commented Apr 8, 2020

Oh, I will address those comments now.

@ghost
Copy link
Author

ghost commented Apr 8, 2020

So while working on the comments I also rebased it to master and noticed that something was not quite right. It turns libtorrentRasterbar was already updated in #83414, and deluge 1.3.15 continues to build fine with the new version. This is a surprise to me, but well, let's leave it that way then.

@ghost
Copy link
Author

ghost commented Apr 8, 2020

Rebased, squashed together the libtorrentRasterbar updates and updated it to 1.2.5.

@peterhoeg
Copy link
Member

With the state check I think it's safe to merge despite the user having to migrate from 1.x to 2.x, but we definitely need a few lines in the release notes.

@ghost
Copy link
Author

ghost commented Apr 9, 2020

I added a note to the release notes, I hope I found the correct section (it is called backwards incompatible but most changes described there are more like forward incompatible).

@ghost ghost changed the title deluge 2.0.3 and libtorrentRasterbar 1.2.3 deluge 2.0.3 and libtorrentRasterbar 1.2.5 Apr 9, 2020
@ghost ghost requested review from peterhoeg and bricewge April 10, 2020 17:58
@ghost
Copy link
Author

ghost commented Apr 17, 2020

@worldofpeace can you tell me if the release note is ok like this? I would like to merge this.

@worldofpeace
Copy link
Contributor

@petabyteboy Note looks good and is in the correct section. Seems fine to me, let's go 👍

Milan Pässler and others added 5 commits April 17, 2020 20:02
…2.1 -> 4.2.2"

This partially reverts commit cc03fb4.
The libtorrentRasterbar update broke deluge 1.x, the hash was not
updated and obsolete dependencies and flags were not removed.
https://raw.githubusercontent.com/arvidn/libtorrent/libtorrent-1_2_5/ChangeLog

The old release is kept available as libtorrentRasterbar-1_1_x for deluge 1.x.
* let's try 2.0 version now, no time better than the present! Maybe!
* bz2 -> xz
* maybe python3
* disable pyGtkGlade for deps, maybe not needed?
* fix gtk/etc deps, deluge-gtk works! \o/
* restore installation of images and such

The old version is kept available as some torrent trackers have not
updated their whitelists yet.
@ghost
Copy link
Author

ghost commented Apr 17, 2020

Thanks everyone ✨

@ghost ghost merged commit 16a4332 into NixOS:master Apr 18, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants