Skip to content

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

Merged
AndersonTorres merged 4 commits intoNixOS:masterfrom
winterqt:bump-jesec-rtorrent
Apr 22, 2022
Merged

jesec-rtorrent: 0.9.8-r15 -> 0.9.8-r16#169236
AndersonTorres merged 4 commits intoNixOS:masterfrom
winterqt:bump-jesec-rtorrent

Conversation

@winterqt
Copy link
Copy Markdown
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 winterqt force-pushed the bump-jesec-rtorrent branch from 275e630 to 8ce3acc Compare April 18, 2022 22:24
@winterqt winterqt requested a review from AndersonTorres April 18, 2022 22:24
@winterqt
Copy link
Copy Markdown
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 a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 18, 2022
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.

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
Copy Markdown
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
Copy Markdown
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.

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 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@AndersonTorres
Copy link
Copy Markdown
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...

@winterqt winterqt force-pushed the bump-jesec-rtorrent branch from 8ce3acc to 5fe5016 Compare April 21, 2022 23:49
@winterqt winterqt requested a review from AndersonTorres April 21, 2022 23:50
@winterqt winterqt force-pushed the bump-jesec-rtorrent branch from 5fe5016 to 73ff43b Compare April 21, 2022 23:53
Copy link
Copy Markdown
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
Copy Markdown
Member Author

@ofborg eval

@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package labels Apr 22, 2022
@ofborg ofborg bot requested a review from AndersonTorres April 22, 2022 00:19
@ofborg ofborg bot added the 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. label Apr 22, 2022
@winterqt winterqt force-pushed the bump-jesec-rtorrent branch from 2715bc0 to 0892af9 Compare April 22, 2022 00:39
@winterqt winterqt force-pushed the bump-jesec-rtorrent branch from 0892af9 to b9e02e7 Compare April 22, 2022 00:43
@winterqt
Copy link
Copy Markdown
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 This PR removes packages or removes other cruft 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 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

jesec-libtorrent failing test

2 participants