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

jesec-rtorrent: 0.9.8-r15 -> 0.9.8-r16 #169236

Merged
merged 4 commits into from
Apr 22, 2022

Conversation

winterqt
Copy link
Member

Description of changes
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/)
  • 22.05 Release Notes (or backporting 21.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.

@winterqt
Copy link
Member Author

@AndersonTorres I do wish there was some way we could clean up the usage of jesec-rtorrent and rtorrent-jesec in top-level. Since the name of the packages have the jesec- prefix, I think it would make sense that pkgs.jesec-rtorrent would work, but that's the key of the set. Do you have any suggestions?

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 18, 2022
@@ -6,8 +6,10 @@
, gtest
, libtorrent
, ncurses
, jsonRpcSupport ? true, nlohmann_json
Copy link
Member

Choose a reason for hiding this comment

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

There is no reason to split this line.

The reason for this is obvious: the first is a Boolean argument that controls the insertion of the second one as a dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, except for the fact that this was the result of running the file through nixpkgs-fmt -- no formatter will be able to know this (and if Nixpkgs is auto-formatted in the future, this will happen anyways).

Copy link
Member

Choose a reason for hiding this comment

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

This was the only thing the formatter found? Six lines in a diff that worsens the legibility of a code?
And people ask why I don't like them...

Also formatters are not to be the last word. Machines are meant to help us, not us to help them.

@@ -6,8 +6,10 @@
, gtest
, libtorrent
, ncurses
, jsonRpcSupport ? true, nlohmann_json
Copy link
Member

Choose a reason for hiding this comment

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

This was the only thing the formatter found? Six lines in a diff that worsens the legibility of a code?
And people ask why I don't like them...

Also formatters are not to be the last word. Machines are meant to help us, not us to help them.

@AndersonTorres
Copy link
Member

Do you have any suggestions?

The relevant snippet is

  jesec-rtorrent = recurseIntoAttrs
    (callPackage ../tools/networking/p2p/jesec-rtorrent {
      callPackage = newScope pkgs.jesec-rtorrent;
    });

  rtorrent-jesec = jesec-rtorrent.rtorrent;
  libtorrent-jesec = jesec-rtorrent.libtorrent;

If we just flip rtorrent-jesec with jesec-rtorrent, it should work fine.

  rtorrent-jesec = recurseIntoAttrs
    (callPackage ../tools/networking/p2p/jesec-rtorrent {
      callPackage = newScope pkgs.rtorrent-jesec;
    });

  jesec-rtorrent = rtorrent-jesec.rtorrent;
  jesec-libtorrent = rtorrent-jesec.libtorrent;

@winterqt
Copy link
Member Author

I still think this is confusing considering the set name and the exposed package names at top level are so similar. I don't think there's any better solution, though...

@winterqt
Copy link
Member Author

@AndersonTorres Maybe we should consider reverting the addition of the package set/scope for the rtorrent and libtorrent variations?

@AndersonTorres
Copy link
Member

@winterqt

No problems to me.

However, do not to put libtorrent inside development/libraries/ because, well, they are not meant to be general-purpose torrent libraries...

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

LGTM, waiting ofborg.

@winterqt
Copy link
Member Author

@ofborg eval

@winterqt
Copy link
Member Author

@AndersonTorres Should be good now -- OfBorg passes. I did add an extra commit though, please see that before merging.

@AndersonTorres AndersonTorres merged commit 8411006 into NixOS:master Apr 22, 2022
@vcunat vcunat linked an issue Apr 22, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jesec-libtorrent failing test
2 participants