Skip to content

transmission_4: init at 4.0.3#215316

Closed
ajs124 wants to merge 6 commits into
NixOS:masterfrom
helsinki-systems:upd/transmission
Closed

transmission_4: init at 4.0.3#215316
ajs124 wants to merge 6 commits into
NixOS:masterfrom
helsinki-systems:upd/transmission

Conversation

@ajs124

@ajs124 ajs124 commented Feb 8, 2023

Copy link
Copy Markdown
Member
Description of changes

https://github.com/transmission/transmission/releases/tag/4.0.0

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ajs124 ajs124 marked this pull request as ready for review February 8, 2023 14:22
@ofborg ofborg Bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 8, 2023
@ajs124

ajs124 commented Feb 8, 2023

Copy link
Copy Markdown
Member Author

seems to break fragments and torrential

@ajs124

ajs124 commented Feb 8, 2023

Copy link
Copy Markdown
Member Author

fragments (even after updating to 2.1) seems incompatible with this release of transmission:

 ERROR transmission_client::client > Unable to parse json: invalid type: boolean `true`, expected i32 at line 1 column 6015

cc @emilytrau

@mlyxshi

mlyxshi commented Feb 9, 2023

Copy link
Copy Markdown
Member

According to Building-Transmission, I think we can use fetchSubmodules to fetch third-party dependencies so that we don't need to keep track of them in Nixpkgs.

@ruuda

ruuda commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

Result of nixpkgs-review on x86_64-linux:

2 packages failed to build:
fragments torrential

5 packages built:
dht libtransmission transmission transmission-gtk transmission-qt

@winterqt

winterqt commented Feb 10, 2023

Copy link
Copy Markdown
Member

@mlyxshi

According to Building-Transmission, I think we can use fetchSubmodules to fetch third-party dependencies so that we don't need to keep track of them in Nixpkgs.

We prefer not using vendored libraries whenever possible, and since things work fine with our versions, I don't see why we should change that all of a sudden.

I only use transmission-daemon, transmission-remote, and web html, https://github.com/mlyxshi/flake/blob/main/pkgs/transmission/default.nix

Is there something you forgot to say here? (I ask because I don't see its relevance, and want to make sure this bump doesn't regress anything.)

@vcunat

vcunat commented Feb 12, 2023

Copy link
Copy Markdown
Member
  • I think it would be nice to fix *-darwin (see ofBorg), as they currently build in nixpkgs (86, aarch).

@winterqt

Copy link
Copy Markdown
Member

Sure, I'll look into it some time in the next few hours, looks simple enough.

@winterqt

winterqt commented Feb 12, 2023

Copy link
Copy Markdown
Member
diff --git a/pkgs/applications/networking/p2p/transmission/default.nix b/pkgs/applications/networking/p2p/transmission/default.nix
index 80e159d9545..07a82bbc24c 100644
--- a/pkgs/applications/networking/p2p/transmission/default.nix
+++ b/pkgs/applications/networking/p2p/transmission/default.nix
@@ -32,6 +32,7 @@
 , enableCli ? true
 , installLib ? false
 , apparmorRulesFromClosure
+, Foundation
 }:
 
 let
@@ -62,6 +63,12 @@ in stdenv.mkDerivation {
       "-DENABLE_DAEMON=${mkFlag enableDaemon}"
       "-DENABLE_CLI=${mkFlag enableCli}"
       "-DINSTALL_LIB=${mkFlag installLib}"
+    ] ++ lib.optionals stdenv.isDarwin [
+      # Transmission sets this to 10.13 if not explicitly specified, see https://github.com/transmission/transmission/blob/0be7091eb12f4eb55f6690f313ef70a66795ee72/CMakeLists.txt#L7-L16.
+      "-DCMAKE_OSX_DEPLOYMENT_TARGET=${stdenv.hostPlatform.darwinMinVersion}"
+
+      # This doesn't work on Darwin, see #214945.
+      "-DRUN_CLANG_TIDY=OFF"
     ];
 
   nativeBuildInputs = [
@@ -90,7 +97,7 @@ in stdenv.mkDerivation {
   ++ lib.optionals enableGTK3 [ gtk3 gtkmm3 xorg.libpthreadstubs ]
   ++ lib.optionals enableSystemd [ systemd ]
   ++ lib.optionals stdenv.isLinux [ inotify-tools ]
-  ++ lib.optionals stdenv.isDarwin [ libiconv ];
+  ++ lib.optionals stdenv.isDarwin [ libiconv Foundation ];
 
   postInstall = ''
     mkdir $apparmor
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 72a973650f7..1c7bef1076b 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -33334,7 +33334,9 @@ with pkgs;
 
   transcribe = callPackage ../applications/audio/transcribe { };
 
-  transmission = callPackage ../applications/networking/p2p/transmission { };
+  transmission = darwin.apple_sdk_11_0.callPackage ../applications/networking/p2p/transmission {
+    inherit (darwin.apple_sdk_11_0.frameworks) Foundation;
+  };
   libtransmission = transmission.override {
     installLib = true;
     enableDaemon = false;

(Did you want the inputs to be sorted somehow? e.g. I see you added gtkmm3 alphabetically in both the callPackage args and the buildInputs entry. Feel free to edit it to do that if you want.)

@ajs124

ajs124 commented Feb 13, 2023

Copy link
Copy Markdown
Member Author

What does the darwin.apple_sdk_11_0 in all-packages.nix do and is that really necessary? I'd assume/hope the inherit is enough.

The inputs are sorted… somehow? I just added gtkmm3 there, because that's where the other gtk stuff is.

Edit: apparently darwin.apple_sdk_11_0.callPackage uses a newer sdk on darwin and is a noop otherwise?

@ajs124 ajs124 mentioned this pull request Feb 16, 2023
12 tasks
@ajs124 ajs124 force-pushed the upd/transmission branch 3 times, most recently from 30afa8b to c3d931f Compare February 16, 2023 12:32
@winterqt

Copy link
Copy Markdown
Member

Edit: apparently darwin.apple_sdk_11_0.callPackage uses a newer sdk on darwin and is a noop otherwise?

Correct, it's needed for x86_64-darwin support specifically, as aarch64-darwin already uses a newer SDK.

@winterqt

Copy link
Copy Markdown
Member

@ajs124 Can you change the CLANG_TIDY line to be unconditional, saying it doesn't work with clangStdenv (which Darwin uses by default)? I think that's better because it'll maintain support for both Darwin and clangStdenv on Linux users. (Also: the clang-tidy mechanism is a noop for non-Clang stdenv users right now anyways.)

@ajs124

ajs124 commented Feb 16, 2023

Copy link
Copy Markdown
Member Author

what about using stdenv.cc.isClang?

edit: ah, if it's a noop, I guess we can just disable it unconditionally, yes. why is it a noop though?

@winterqt

Copy link
Copy Markdown
Member

The CMake file doesn't even try to run clang-tidy with the gccStdenv (as it doesn't exist, so it silently disables it), but with the Clang stdenv, it'll see that clang-tidy exists, but the binary doesn't actually work because it's not wrapped.

@winterqt winterqt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Guess we can just leave the alphabetical Foundation thing as is? shrug

Comment on lines 71 to 72

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# This doesn't work on Darwin, see #214945.

Comment on lines 66 to 67

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Keep styling?

Suggested change
# breaks on darwin or with clangStdenv and is a noop otherwise
"-DRUN_CLANG_TIDY=OFF"
"-DRUN_CLANG_TIDY=OFF" # breaks on darwin or with clangStdenv and is a noop otherwise

@ajs124

ajs124 commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

@vcunat @winterqt do you think this is good to go?

@chvp

chvp commented Jun 10, 2023

Copy link
Copy Markdown
Member

There doesn't seem to be a lot of movement on the bug linked above. Since the plan is to keep both transmission 3 and 4 around anyway, could we merge this but perhaps keep 3.0 as the default? I am personally looking to upgrade because of the performance improvements realized in 4.0. (I also don't know how common it is to skip downloading files, and the condition of zero-sized files having to be in the torrent as well makes it a rare-enough occurrence I would guess.)

Also, 4.0.3 got released a few weeks ago.

@ajs124 ajs124 force-pushed the upd/transmission branch from 16d92f3 to 8bae0fe Compare June 13, 2023 18:41
@ajs124 ajs124 marked this pull request as ready for review June 13, 2023 18:42
@ajs124 ajs124 force-pushed the upd/transmission branch from 8bae0fe to 68d19ed Compare June 13, 2023 18:42
@ajs124 ajs124 changed the title transmission: 3.0.0 -> 4.0.0 transmission: 3.0.0 -> 4.0.3 Jun 13, 2023
Comment thread pkgs/top-level/all-packages.nix Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should probably go to aliases.nix. The only consumer, torrential already uses transmission_3 directly.

Comment thread pkgs/top-level/all-packages.nix Outdated
Comment on lines 34894 to 34895

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These should be changed to use transmission_3 too.

Comment thread pkgs/top-level/all-packages.nix Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be transmission_4, right?

Suggested change
libtransmission_4 = transmission.override {
libtransmission_4 = transmission_4.override {

@chvp

chvp commented Jun 21, 2023

Copy link
Copy Markdown
Member

I pushed a commit to my nixpkgs fixing eval: chvp@9bc6db8 Feel free to cherry-pick/rebase into this branch.

jtojnar and others added 6 commits June 21, 2023 13:29
- Use one dependency per-line for cleaner diffs.
- Order derivation attributes according to convention.
- Extract apparmor rules expression into let binding.
- Use finalAttrs instead of let binding for consistent overriding.
transmission/libutp@fda9f4b...9cb9f9c

Will be required for Transmission 4.0.0, keep the old version for other programs.
Introduces pared down Transmission 3 package for apps that depend on it, until they are ported to Transmission 4.
Only keeping the library and daemon, as they are needed for Torrential and Fragments, respectively.
Not separating into different derivations them since the daemon only adds 1M to the closure.
@ajs124 ajs124 force-pushed the upd/transmission branch from 68d19ed to ac70c89 Compare June 21, 2023 11:34
@vcunat vcunat changed the title transmission: 3.0.0 -> 4.0.3 transmission_4: init at 4.0.3 Jun 21, 2023
@sveitser

Copy link
Copy Markdown
Contributor

Have been using transmission_4 from 68d19ed6aa236e1f045dfe80e494538f35df99a0 with the transmission nixos module for a few days and it has been working great.

@chvp chvp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using the full patch as it's in this PR now, transmission 4 is running fine on my server as well (on x86_64).

meta = {
description = "Old version of libtransmission library for apps that depend on it";
homepage = "http://www.transmissionbt.com/";
license = lib.licenses.gpl2Plus; # parts are under MIT

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then the license should be both

description = "Old version of libtransmission library for apps that depend on it";
homepage = "http://www.transmissionbt.com/";
license = lib.licenses.gpl2Plus; # parts are under MIT
maintainers = with lib.maintainers; [ ];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Who wants to?

@ajs124 ajs124 closed this Jul 2, 2023
@ajs124

ajs124 commented Jul 2, 2023

Copy link
Copy Markdown
Member Author

feel free to take over anyone, I don't have time for this anymore

@ajs124 ajs124 deleted the upd/transmission branch July 2, 2023 15:47
@chvp chvp mentioned this pull request Jul 2, 2023
12 tasks
@chvp

chvp commented Jul 3, 2023

Copy link
Copy Markdown
Member

I've taken over in #241146.

@doronbehar

Copy link
Copy Markdown
Contributor

For all those interested, I propose a rename making transmission = transmission_4 in #258058 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: wait-for-upstream Waiting for upstream fix (or their other action). 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.