nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 443671
Commit: 3df23a82c966e957bd039d2305099b00408b33a0

x86_64-linux

@ccicnce113424 I have a feeling that this won't be accepted as is because of the inclusion of the ceph-s3select-arrow-20-compat.patch. I've been using that since your #426609 and while I understand the caution, I'd love your opinion since carrying a bunch of out of tree packages has it's own maintenance overhead (with the obvious caveat that I've done the same thing with rocksdb 7.9 here).

hello? how about reacting in any way dear five! reviewers?

I don't think I'll get around to taking a closer look at this one soon, however it would be great if you could split the "move to pkgs/by-name" off into a separate commit.
With everything in the same commit the diff is really hard to review.
Separate commits would allow to review the code changes of either commit properly without having to cross-reference files, and it would allow proper comments using the GitHub review feature, which currently recognises all files as deleted and new, none as moved, as far as I can tell.

Edit: please note also the nixpkgs commit conventions for pkgs

Yeah, I'm also a volunteer.

It's easier for me to carry this overlay than go back and redo commits because it would fit the GitHub workflow better or because I ignored the commit convention. Originally I opened this as a draft, but after 2 weeks, no one even attempted to fix the ceph build, which is a pretty important package for NixOS to be considered a serious system. I'm not saying I won't go back and redo this PR, but I also don't think I'll get around to taking a closer look anytime soon. Feel free to use this work as you see fit. It's open source, after all.

@djds I don't quite understand if/how it addresses the PyO3 issue (#380823).

Doesn't that need ceph/ceph#62951 which is not in an upstream Ceph release?

@djds I don't quite understand if/how it addresses the PyO3 issue (#380823).

Doesn't that need ceph/ceph#62951 which is not in an upstream Ceph release?

Yeah I think I'm just wrong about this.

@djd

It's easier for me to carry this overlay than go back and redo commits because it would fit the GitHub workflow better

Note this is not about any workflow imposed by Github; it is that it makes human review harder when one cannot easily discern refactorings (which can be trivially be provent to be absent of bugs by checking that the nix eval produces the same .drv) from actual changes one has to review and test carefully. That turns reviewing a few lines very carefully into having to review hundreds of lines carefully.

or because I ignored the commit convention.

That's not a blocker at all (anybody can easily fix that), but you just won't get the nice automated ofborg CI (the packages it attempts to build are determined by the text in the commit message, thus the convention).

I'm not saying I won't go back and redo this PR, but I also don't think I'll get around to taking a closer look anytime soon. Feel free to use this work as you see fit.

A code drop of something that is working is of course already very useful, thank you for that!

But somebody will have to pick the commits apart to merge it (you, one of us, or anybody else).

Cross‐linking #451118. (That one is presumably much easier to land.)

can we expect this in 25.11?

With PR

• pkgs/ceph: move to by-name #455186

merged, the diff is smaller:

# git diff --stat origin/master HEAD pkgs/by-name/ce/ceph/  
 pkgs/by-name/ce/ceph/arrow-cpp-19.nix                                           | 346 --------------------------------------
 pkgs/by-name/ce/ceph/old-python-packages/cryptography-vectors.nix               |  36 ----
 pkgs/by-name/ce/ceph/old-python-packages/cryptography.nix                       | 140 ---------------
 ...hon-cryptography-Cherry-pick-fix-for-CVE-2023-49083-on-cryptography-40.patch |  44 -----
 pkgs/by-name/ce/ceph/old-python-packages/trustme.nix                            |  52 ------
 pkgs/by-name/ce/ceph/package.nix                                                | 255 ++++++----------------------
 pkgs/by-name/ce/ceph/{ => patches}/boost-1.85.patch                             |   0
 pkgs/by-name/ce/ceph/{ => patches}/boost-1.86-PyModule.patch                    |   0
 pkgs/by-name/ce/ceph/rocksdb_7_9/default.nix                                    | 138 +++++++++++++++
 pkgs/by-name/ce/ceph/rocksdb_7_9/patches/fix-findliburing.patch                 |  29 ++++
 10 files changed, 222 insertions(+), 818 deletions(-)

Side note: Ceph 20 will have Python 3.13 support:

ceph/ceph@ddb4721

But somebody will have to pick the commits apart to merge it (you, one of us, or anybody else).

• @benaryorg pulled out the by-name refactor in PR pkgs/ceph: move to by-name #455186
• I created a PR with the rest:
  • ceph: Build with Python 3.12 #455889

(Hopefully I got exactly the difference right after the PRs merged meanwhile: #440224 and #440226)

@djds please have a review whether that new PR still looks right to you.

Once that is done, we can close this PR here as well.