Conversation
|
cc @nh2 as usual |
|
Tests are throwing a lot of And at this point I absolutely do not know whether 20.2.0 actually includes all the funky shenanigans written prior to its release to make stuff work regardless. |
|
Addendum: it seems the functionality was merged into the tentacle branch but hasn't actually hit a release yet. |
|
I just found #455889.… in which I already commented.… because I remembered mentioning something about inheriting the src proper for the Python package subbuild, which is exactly the thing this suffers from right now, and apparently I've just coded out the absolute mess that I had in mind back then. So this PR solves the minor issue of needing the patches for both the Python and C++ codebase, however it's become a bit convoluted and unreadable and probably doesn't adhere to the best-practices which hopefully exist for this exact use-case. Since I'm also apparently duplicating an entire PR because I forgot that the other PR already exists, I'm not sure what to do with this tbh ^^" |
|
Just to be clear; the code prior to the upcoming force-push (227d874) does work in most regards except for the mgr, since the mgr requires the patches for s3select which in that version were only passed to the C++ build, not the Python build. The force push (which should show right below this) is still in the process of being built on my local machine, and given the weird code changes I've made I'm not sure it will build, however if it does, it should fix a whole host of issues by allowing patches and src override to propagate, but I also think there should be a cleaner variant of that by building multiple packages (one of which being the source package) in a fixpoint which isn't the package itself. |
|
Quick reminder that fetchpatch and fetchpatch2 do funky processing which isn't always perfect; for instance the PyO3 patch includes an |
|
Okay, so the idea of being able to do something akin to |
|
Currently the ceph NixOS tests pass. |
|
I've been working on the same upgrade based on the work from nh2's branch. I got all the test to passing too and am running into the same ImportErrors. My approach has been using a backport of the patch from ceph/ceph#66794. It doesn't look like we took the same approach to this, but here's my work on this so far if it helps: github.com/James-1701/nixpkgs/tree/ceph-20 |
This PR uses the same patch, however notably this PR fetches the patches since they are already merged upstream (just not in a release yet). The branch also does not include the two dependencies I mentioned earlier (xmltodict and jsmepath), meaning there are likely more missing dependencies (getting a fresh list of all dependencies is on my list for this PR). To sum it up, while I appreciate the work, the Ceph package requires some changes which have yet to be made, most importantly a unified handling of patches (which this PR here actually has, although in a very awkward way) for C++ and Python code. Footnotes |
173018a to
3755723
Compare
|
Thanks for looking at my branch and explaining the issues! While I've been using Nix and NixOS for a while, I'm just now getting started with contributing, so feedback like this is very helpful and appreciated. When I started working on this your PR wasn't up yet, so I figured I'd get something passing tests out there to help move things forward, so when I finished my branch and saw yours I figured I'd just leave a comment hoping it might help a bit. Good luck with the refactor, hope it gets merged soon! |
|
The latest push includes the jmespath and xmltodict libs, and also fixes the python path meaning the libs actually load again. Other than that the tests are running smooth as butter, and personally I think the (Nix) code changes significantly improve the code quality in terms of being able to maintain the package (given how many overrides and removed packages Ceph usually needs in nixpkgs). On that note, feel free to nitpick all of the code to your heart's content, the structure as it is now is sort of what I was eluding to earlier, now it's just about untangling the smaller messes in there that could be streamlined or which aren't according to our best practices. I'll mark the PR as ready for review, but please note that it is not ready for merging just yet. |
|
For review purposes, here's a diff of the prior package.nix to the current (in this PR) ceph.nix because GitHub does display a different (much less useful) diff for the file. Ceph package diffHow to generateGenerated with: # the latter blob hash can be replaced by the file in the tree to diff the most current one
git diff -w 0309d5af5d9564d64a7a1f686ec9407c4146ad50 89e1e6db01312fe23f408b398209a812f1f6e88bYou can get the blob hashes from the header in a diff: diff --git a/pkgs/by-name/ce/ceph/arrow-cpp-19.nix b/pkgs/by-name/ce/ceph/arrow-cpp-19.nix
deleted file mode 100644
index 549ca5e77f17..000000000000 <- these here are the blob hashes
--- a/pkgs/by-name/ce/ceph/arrow-cpp-19.nix
+++ /dev/null(Note: there's a second collapsible HTML thing right above, easy to miss) diff --git a/0309d5af5d9564d64a7a1f686ec9407c4146ad50 b/89e1e6db01312fe23f408b398209a812f1f6e88b
index 0309d5af5d95..89e1e6db0131 100644
--- a/0309d5af5d9564d64a7a1f686ec9407c4146ad50
+++ b/89e1e6db01312fe23f408b398209a812f1f6e88b
@@ -1,52 +1,31 @@
{
lib,
stdenv,
- runCommand,
- fetchurl,
- fetchFromGitHub,
- fetchPypi,
- fetchpatch2,
- callPackage,
+ overrideScope,
+ ceph-meta,
+ ceph-src,
# Build time
autoconf,
automake,
cmake,
ensureNewerSourcesHook,
- # To see which `fmt` version Ceph upstream recommends, check its `src/fmt` submodule.
- #
- # Ceph does not currently build with `fmt_10`; see https://github.com/NixOS/nixpkgs/issues/281027#issuecomment-1899128557
- # If we want to switch for that before upstream fixes it, use this patch:
- # https://github.com/NixOS/nixpkgs/pull/281858#issuecomment-1899648638
- fmt_9,
+ fmt,
git,
libtool,
makeWrapper,
nasm,
pkg-config,
which,
- openssl,
# Tests
nixosTests,
# Runtime dependencies
- # Remove once Ceph supports arrow-cpp >= 20, see:
- # * https://tracker.ceph.com/issues/71269
- # * https://github.com/NixOS/nixpkgs/issues/406306
- ceph-arrow-cpp ? callPackage ./arrow-cpp-19.nix { },
+ arrow-cpp,
babeltrace,
- # Note when trying to upgrade boost:
- # * When upgrading Ceph, it's recommended to check which boost version Ceph uses on Fedora,
- # and default to that.
- # * The version that Ceph downloads if `-DWITH_SYSTEM_BOOST:BOOL=ON` is not given
- # is declared in `cmake/modules/BuildBoost.cmake` line `set(boost_version ...)`.
- #
- # If you want to upgrade to boost >= 1.86, you need a Ceph version that
- # has this PR in:
- # https://github.com/ceph/ceph/pull/61312
- boost183,
+ ceph-boost,
bzip2,
cryptsetup,
cunit,
@@ -73,9 +52,9 @@
oath-toolkit,
openldap,
parted,
- python311, # to get an idea which Python versions are supported by Ceph, see upstream `do_cmake.sh` (see `PYBUILD=` variable)
+ ceph-python-env,
rdkafka,
- rocksdb,
+ ceph-rocksdb,
snappy,
openssh,
sqlite,
@@ -84,9 +63,6 @@
zlib,
zstd,
- # Dependencies of overridden Python dependencies, hopefully we can remove these soon.
- rustPlatform,
-
# Optional Dependencies
curl ? null,
expat ? null,
@@ -147,17 +123,6 @@ let
optLibxfs = shouldUsePkg libxfs;
optZfs = shouldUsePkg zfs;
- # Downgrade rocksdb, 7.10 breaks ceph
- rocksdb' = rocksdb.overrideAttrs {
- version = "7.9.2";
- src = fetchFromGitHub {
- owner = "facebook";
- repo = "rocksdb";
- tag = "v7.9.2";
- hash = "sha256-5P7IqJ14EZzDkbjaBvbix04ceGGdlWBuVFH/5dpD5VM=";
- };
- };
-
hasRadosgw = optExpat != null && optCurl != null && optLibedit != null;
# Malloc implementation (can be jemalloc, tcmalloc or null)
@@ -181,263 +146,25 @@ let
none = [ ];
};
- getMeta = description: {
- homepage = "https://ceph.io/en/";
- inherit description;
- license = with lib.licenses; [
- lgpl21
- gpl2Only
- bsd3
- mit
- publicDomain
- ];
- maintainers = with lib.maintainers; [
- adev
- ak
- johanot
- krav
- nh2
- benaryorg
- ];
- platforms = [
- "x86_64-linux"
- "aarch64-linux"
- ];
- };
-
- ceph-common =
- with python.pkgs;
- buildPythonPackage {
- pname = "ceph-common";
- format = "setuptools";
- inherit src version;
-
- sourceRoot = "ceph-${version}/src/python-common";
-
- propagatedBuildInputs = [
- pyyaml
- ];
-
- nativeCheckInputs = [
- pytestCheckHook
- ];
-
- disabledTests = [
- # requires network access
- "test_valid_addr"
- ];
-
- meta = getMeta "Ceph common module for code shared by manager modules";
- };
-
- # Watch out for python <> boost compatibility
- python = python311.override {
- self = python;
- packageOverrides =
- self: super:
- let
- bcryptOverrideVersion = "4.0.1";
- in
- {
- # Ceph does not support the following yet:
- # * `bcrypt` > 4.0
- # * `cryptography` > 40
- # See:
- # * https://github.com/NixOS/nixpkgs/pull/281858#issuecomment-1899358602
- # * Upstream issue: https://tracker.ceph.com/issues/63529
- # > Python Sub-Interpreter Model Used by ceph-mgr Incompatible With Python Modules Based on PyO3
- # * Moved to issue: https://tracker.ceph.com/issues/64213
- # > MGR modules incompatible with later PyO3 versions - PyO3 modules may only be initialized once per interpreter process
-
- bcrypt = super.bcrypt.overridePythonAttrs (old: rec {
- pname = "bcrypt";
- version = bcryptOverrideVersion;
- src = fetchPypi {
- inherit pname version;
- hash = "sha256-J9N1kDrIJhz+QEf2cJ0W99GNObHskqr3KvmJVSplDr0=";
- };
- cargoRoot = "src/_bcrypt";
- cargoDeps = rustPlatform.fetchCargoVendor {
- inherit
- pname
- version
- src
- cargoRoot
- ;
- hash = "sha256-8PyCgh/rUO8uynzGdgylAsb5k55dP9fCnf40UOTCR/M=";
- };
- });
-
- # We pin the older `cryptography` 40 here;
- # this also forces us to pin other packages, see below
- cryptography = self.callPackage ./old-python-packages/cryptography.nix { };
-
- # This is the most recent version of `pyopenssl` that's still compatible with `cryptography` 40.
- # See https://github.com/NixOS/nixpkgs/pull/281858#issuecomment-1899358602
- # and https://github.com/pyca/pyopenssl/blob/d9752e44127ba36041b045417af8a0bf16ec4f1e/CHANGELOG.rst#2320-2023-05-30
- pyopenssl = super.pyopenssl.overridePythonAttrs (old: rec {
- version = "23.1.1";
- src = fetchPypi {
- pname = "pyOpenSSL";
- inherit version;
- hash = "sha256-hBSYub7GFiOxtsR+u8AjZ8B9YODhlfGXkIF/EMyNsLc=";
- };
- disabledTests = old.disabledTests or [ ] ++ [
- "test_export_md5_digest"
- ];
- disabledTestPaths = old.disabledTestPaths or [ ] ++ [
- "tests/test_ssl.py"
- ];
- propagatedBuildInputs = old.propagatedBuildInputs or [ ] ++ [
- self.flaky
- ];
- # hack: avoid building docs due to incompatibility with current sphinx
- nativeBuildInputs = [ openssl ]; # old.nativeBuildInputs but without sphinx*
- outputs = lib.filter (o: o != "doc") old.outputs;
- });
-
- # This is the most recent version of `trustme` that's still compatible with `cryptography` 40.
- # See https://github.com/NixOS/nixpkgs/issues/359723
- # and https://github.com/python-trio/trustme/commit/586f7759d5c27beb44da60615a71848eb2a5a490
- trustme = self.callPackage ./old-python-packages/trustme.nix { };
-
- fastapi = super.fastapi.overridePythonAttrs (old: {
- # Flaky test:
- # ResourceWarning: Unclosed <MemoryObjectSendStream>
- # Unclear whether it's flaky in general or only in this overridden package set.
- doCheck = false;
- });
-
- # Ceph does not support `kubernetes` >= 19, see:
- # https://github.com/NixOS/nixpkgs/pull/281858#issuecomment-1900324090
- kubernetes = super.kubernetes.overridePythonAttrs (old: rec {
- version = "18.20.0";
- src = fetchFromGitHub {
- owner = "kubernetes-client";
- repo = "python";
- rev = "v${version}";
- sha256 = "1sawp62j7h0yksmg9jlv4ik9b9i1a1w9syywc9mv8x89wibf5ql1";
- fetchSubmodules = true;
- };
- });
-
- };
- };
-
- boost' = boost183.override {
- enablePython = true;
- inherit python;
- };
-
- # TODO: split this off in build and runtime environment
- ceph-python-env = python.withPackages (
- ps: with ps; [
- ceph-common
-
- # build time
- cython_0
-
- # debian/control
- bcrypt
- cherrypy
- influxdb
- jinja2
- kubernetes
- natsort
- numpy
- pecan
- prettytable
- pyjwt
- pyopenssl
- python-dateutil
- pyyaml
- requests
- routes
- scikit-learn
- scipy
- setuptools
- sphinx
- virtualenv
- werkzeug
-
- # src/cephadm/zipapp-reqs.txt
- markupsafe
-
- # src/pybind/mgr/requirements-required.txt
- cryptography
- jsonpatch
-
- # src/tools/cephfs/shell/setup.py
- cmd2
- colorama
- ]
- );
- inherit (ceph-python-env.python) sitePackages;
-
- version = "19.2.3";
- src = fetchurl {
- url = "https://download.ceph.com/tarballs/ceph-${version}.tar.gz";
- hash = "sha256-zlgp28C81SZbaFJ4yvQk4ZgYz4K/aZqtcISTO8LscSU=";
- };
in
stdenv.mkDerivation {
pname = "ceph";
- inherit src version;
-
- patches = [
- ./boost-1.85.patch
-
- (fetchpatch2 {
- name = "ceph-boost-1.86-uuid.patch";
- url = "https://github.com/ceph/ceph/commit/01306208eac492ee0e67bff143fc32d0551a2a6f.patch?full_index=1";
- hash = "sha256-OnDrr72inzGXXYxPFQevsRZImSvI0uuqFHqtFU2dPQE=";
- })
-
- # See:
- # * <https://github.com/boostorg/python/issues/394>
- # * <https://aur.archlinux.org/cgit/aur.git/commit/?h=ceph&id=8c5cc7d8deec002f7596b6d0860859a0a718f12b>
- # * <https://github.com/ceph/ceph/pull/60999>
- ./boost-1.86-PyModule.patch
-
- (fetchpatch2 {
- name = "ceph-cmake-4.patch";
- url = "https://gitlab.alpinelinux.org/ashpool/aports/-/raw/d22b70eafe33c3daabe4eea6913c5be87d9463ad/community/ceph19/cpp_redis.patch";
- hash = "sha256-wxPIsYt25CjXhJ6kmr/MXwFD58Sl4y4W+r9jAMND+uw=";
- })
-
- # See:
- # * <https://github.com/ceph/ceph/pull/55560>
- # * <https://github.com/ceph/ceph/pull/60575>
- (fetchpatch2 {
- name = "ceph-systemd-sans-cluster-name.patch";
- url = "https://github.com/ceph/ceph/commit/5659920c7c128cb8d9552580dbe23dd167a56c31.patch?full_index=1";
- hash = "sha256-Uch8ZghyTowUvSq0p/RxiVpdG1Yqlww9inpVksO6zyk=";
- })
- (fetchpatch2 {
- name = "ceph-systemd-prefix.patch";
- url = "https://github.com/ceph/ceph/commit/9b38df488d7101b02afa834ea518fd52076d582a.patch?full_index=1";
- hash = "sha256-VcbJhCGTUdNISBd6P96Mm5M3fFVmZ8r7pMl+srQmnIQ=";
- })
- (fetchpatch2 {
- name = "ceph-19.2.2-gcc15.patch";
- url = "https://github.com/ceph/ceph/commit/830925f0dd196f920893b1947ae74171a202e825.patch";
- hash = "sha256-bs+noyjiyAjwqfgSHDxdZJnZ/kptOOcz75KMqAaROpg=";
- })
- ];
+ inherit (ceph-src) version;
+ src = ceph-src;
nativeBuildInputs = [
autoconf # `autoreconf` is called, e.g. for `qatlib_ext`
automake # `aclocal` is called, e.g. for `qatlib_ext`
cmake
- fmt_9
+ fmt
git
makeWrapper
libtool # used e.g. for `qatlib_ext`
nasm
pkg-config
- python
- python.pkgs.python # for the toPythonPath function
- python.pkgs.wrapPython
+ ceph-python-env
+ ceph-python-env.pkgs.python # for the toPythonPath function
+ ceph-python-env.pkgs.wrapPython
which
(ensureNewerSourcesHook { year = "1980"; })
# for building docs/man-pages presumably
@@ -448,9 +175,9 @@ stdenv.mkDerivation {
buildInputs =
cryptoLibsMap.${cryptoStr}
++ [
- ceph-arrow-cpp
+ arrow-cpp
babeltrace
- boost'
+ ceph-boost
bzip2
# Adding `ceph-python-env` here adds the env's `site-packages` to `PYTHONPATH` during the build.
# This is important, otherwise the build system may not find the Python deps and then
@@ -479,7 +206,7 @@ stdenv.mkDerivation {
optYasm
parted # according to `debian/control` file, used by `src/ceph-volume/ceph_volume/util/disk.py`
rdkafka
- rocksdb'
+ ceph-rocksdb
snappy
openssh # according to `debian/control` file, `ssh` command used by `cephadm`
sqlite
@@ -509,6 +236,8 @@ stdenv.mkDerivation {
optLibedit
];
+ inherit (ceph-python-env) sitePackages;
+
# Picked up, amongst others, by `wrapPythonPrograms`.
pythonPath = [
ceph-python-env
@@ -530,10 +259,10 @@ stdenv.mkDerivation {
unset AS
substituteInPlace src/common/module.c \
- --replace "char command[128];" "char command[256];" \
- --replace "/sbin/modinfo" "${kmod}/bin/modinfo" \
- --replace "/sbin/modprobe" "${kmod}/bin/modprobe" \
- --replace "/bin/grep" "${gnugrep}/bin/grep"
+ --replace-fail "char command[128];" "char command[256];" \
+ --replace-fail "/sbin/modinfo" "${kmod}/bin/modinfo" \
+ --replace-fail "/sbin/modprobe" "${kmod}/bin/modprobe" \
+ --replace-fail "/bin/grep" "${gnugrep}/bin/grep"
# Patch remount to use full path to mount(8), otherwise ceph-fuse fails when run
# from a systemd unit for example.
@@ -548,7 +277,7 @@ stdenv.mkDerivation {
--replace-fail "GETOPT=getopt" "GETOPT=${getopt}/bin/getopt"
# The install target needs to be in PYTHONPATH for "*.pth support" check to succeed
- export PYTHONPATH=$PYTHONPATH:$lib/${sitePackages}:$out/${sitePackages}
+ export PYTHONPATH=$PYTHONPATH:$lib/$sitePackages:$out/$sitePackages
patchShebangs src/
'';
@@ -618,18 +347,18 @@ stdenv.mkDerivation {
! grep -F -r 'GETOPT=getopt' $out
! grep -F -r 'GETOPT=/usr/local/bin/getopt' $out
- mkdir -p $client/{bin,etc,${sitePackages},share/bash-completion/completions}
+ mkdir -p $client/{bin,etc,$sitePackages,share/bash-completion/completions}
cp -r $out/bin/{ceph,.ceph-wrapped,rados,rbd,rbdmap} $client/bin
cp -r $out/bin/ceph-{authtool,conf,dencoder,rbdnamer,syn} $client/bin
cp -r $out/bin/rbd-replay* $client/bin
cp -r $out/sbin/mount.ceph $client/bin
cp -r $out/sbin/mount.fuse.ceph $client/bin
ln -s bin $client/sbin
- cp -r $out/${sitePackages}/* $client/${sitePackages}
+ cp -r $out/$sitePackages/* $client/$sitePackages
cp -r $out/etc/bash_completion.d $client/share/bash-completion/completions
# wrapPythonPrograms modifies .ceph-wrapped, so lets just update its paths
- substituteInPlace $client/bin/ceph --replace $out $client
- substituteInPlace $client/bin/.ceph-wrapped --replace $out $client
+ substituteInPlace $client/bin/ceph --replace-fail $out $client
+ substituteInPlace $client/bin/.ceph-wrapped --replace-fail $out $client
'';
outputs = [
@@ -646,12 +375,13 @@ stdenv.mkDerivation {
# Takes 7+h to build with 2 cores.
requiredSystemFeatures = [ "big-parallel" ];
- meta = getMeta "Distributed storage system";
+ meta = ceph-meta "Distributed storage system";
passthru = {
- inherit version;
- inherit python; # to be able to test our overridden packages above individually with `nix-build -A`
- arrow-cpp = ceph-arrow-cpp;
+ inherit (ceph-src) version;
+ inherit overrideScope;
+ inherit arrow-cpp;
+ pythonEnv = ceph-python-env;
tests = {
inherit (nixosTests)
ceph-multi-node |
|
I did not know treefmt did in fact fmt the content of markdown code blocks. Good to know. |
|
sorry i'm a bit of out of the loop, but was fuse updated to be fuse 3 here ? |
@glados-creator Planned for later. |
Very nice! |
|
I added only 1 remaining understanding question to the README; good to merge otherwise! If you need me to click Merge, let me know! |
Moving ceph to its own scope allows separate packages for individual components. Among other benefits this makes overriding a single package for the entire build easier. The primary reason however is to allow calling code to access *overrideScope* to change internal packages such as *ceph-src*. Since the source code is used by multiple packages, notably the C++ and the Python code – independently – any patches that touch both (such as the PyO3 patch introduced herein) need to apply to both. Using just packages with *patches* set makes this harder to control. Material changes: - Python: 3.11 -> 3.12 - Boost: 1.83 -> 1.87 - Ceph: 19.2.3 -> 20.2.0 - fmt: 9 -> 12 - changes in patches (new and old) due to the above updates - Python dependencies on *jmespath* and *xmltodict* - `replace-fail` for *substituteInPlace* - UADK disabled by default (otherwise aarch64 fails to compile) - *ceph-mgr* wrapper now adds ceph binaries to *PATH* - ZFS integration on the packaging level has been removed In-depth information for most of this can be found in [NixOS#494583](https://redirect.github.com/NixOS/nixpkgs/pull/494583). Signed-off-by: benaryorg <binary@benary.org>
Signed-off-by: benaryorg <binary@benary.org>
Signed-off-by: benaryorg <binary@benary.org>
Signed-off-by: benaryorg <binary@benary.org>
Signed-off-by: benaryorg <binary@benary.org>
Currently we're still at fuse 2. In that vein, I think everything is ready for the big merge \o/
I'd say with 54h until the end of the merge window1 we can let this sit until tomorrow just in case anything is raised, after that, feel free to merge away at your leisure. And once more, thanks to everyone. Footnotes
|
I found some more details: The Ceph |
|
Thanks everyone! |
Moving ceph to its own scope allows separate packages for individual components. Among other benefits this makes overriding a single package for the entire build easier. The primary reason however is to allow calling code to access *overrideScope* to change internal packages such as *ceph-src*. Since the source code is used by multiple packages, notably the C++ and the Python code – independently – any patches that touch both (such as the PyO3 patch introduced herein) need to apply to both. Using just packages with *patches* set makes this harder to control. Material changes: - Python: 3.11 -> 3.12 - Boost: 1.83 -> 1.87 - Ceph: 19.2.3 -> 20.2.0 - fmt: 9 -> 12 - changes in patches (new and old) due to the above updates - Python dependencies on *jmespath* and *xmltodict* - `replace-fail` for *substituteInPlace* - UADK disabled by default (otherwise aarch64 fails to compile) - *ceph-mgr* wrapper now adds ceph binaries to *PATH* - ZFS integration on the packaging level has been removed In-depth information for most of this can be found in [NixOS#494583](https://redirect.github.com/NixOS/nixpkgs/pull/494583). Signed-off-by: benaryorg <binary@benary.org>
Ceph update including some major restructuring to allow easier overrides and patching, as well as use of said patching to backport Ceph's PyO3 patches.
Tests1 are passing as of time of writing.
A comment further down has a more basic diff as well as instructions on how to generate one, since GitHub (and even git this time) insist on diffing different files making the diff unhelpful.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Footnotes
nom build --no-link --print-out-paths --print-build-logs --impure --expr '(import ./. {}).callPackage ({ linkFarm, ceph }: linkFarm "ceph-tests" ceph.tests) {}'↩