-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
ceph: Fix build with GCC 13 by using fmt_8 -> fmt_9. Fixes #281027 #281858
Conversation
My local build is currently running, should be done in a few minutes. |
NixOS tests are hanging, but that's unlikely to be caused by this change alone (formatting library upgrade). Hanging at:
Further up in the output, a couple
|
@lejonet or anybody who knows: How does one debug these tests? E.g. because of nixpkgs/nixos/tests/ceph-single-node.nix Line 41 in 0d9f8df
One cannot start the VM with QEMU_NET_OPTS="hostfwd=tcp:127.0.0.1:2222-:22" $(NIX_PATH=nixpkgs=. nix-build --no-out-link -A ceph.tests.ceph-single-node.driverInteractive)/bin/nixos-test-driver to get SSH into the machine to investigate |
I found that this will help connect in conveniently if the |
I resorted now to just delete the It gives:
So indeed the |
Related: https://lists.ceph.io/hyperkitty/list/[email protected]/thread/FB7XE6WYDK3EBJYPABSPX5B2LEILWWJA/ But the "just upgrade" advice is not useful; on Ceph 18.2.1 (#281474) the same issue happens. |
This is more useful: https://forum.proxmox.com/threads/ceph-warning-post-upgrade-to-v8.129371/page-5#post-610330 (In this thread is also the person who posted the above From there:
And a very useful summary: So this effectively breaks the |
We likely want to merge this anyway, because it fixes the build of Ceph and packages that depend on it. But the tests still fail because the Python versions recently upgraded in nixpkgs Maybe we can add back some older Python libraries that Ceph can use until this is fixed? |
@nh2, so should we track down whoever maintains these python libraries and ask them to add older pins? |
Also, no-one was tagged for review -- who is aware of this PR even? |
FYI builds ok with aur fmt10 patch diff --git a/pkgs/tools/filesystems/ceph/default.nix b/pkgs/tools/filesystems/ceph/default.nix
index f38cd4be880c..4625c7e3eeae 100644
--- a/pkgs/tools/filesystems/ceph/default.nix
+++ b/pkgs/tools/filesystems/ceph/default.nix
@@ -4,6 +4,7 @@
, fetchurl
, fetchFromGitHub
, fetchPypi
+, fetchpatch
# Build time
, cmake
@@ -294,6 +295,13 @@ in rec {
optLibedit
];
+ patches = [
+ (fetchpatch {
+ url = "https://aur.archlinux.org/cgit/aur.git/plain/ceph-18.2.0-fmt10-fixes.patch?h=ceph";
+ hash = "sha256-j9xycE3NR54C82tbpMnySMEFNqcTZO31H+kQCQhhOKg=";
+ })
+ ];
+
pythonPath = [ ceph-python-env "${placeholder "out"}/${ceph-python-env.sitePackages}" ];
# replace /sbin and /bin based paths with direct nix store paths
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 8667c3d9cd2b..c4316b02f0c7 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -6817,7 +6817,6 @@ with pkgs;
libceph = ceph.lib;
inherit (callPackages ../tools/filesystems/ceph {
lua = lua5_4;
- fmt = fmt_8;
})
ceph
ceph-client; |
I am currently trying if this works: Adding pins for Will report in a few minutes.
Ideally the people from #281027, I posted there. |
@qubitnano Thanks for checking, that's great to know! I think while we have |
That seems to indeed get rid of the mentioned Python issues. But this one still remains:
Looks like this:
But I don't understand how this worked so far, given that this is 2 years old, and nixpkgs already had the Python package Edit: A related isssue is #241482. So probably this has been an issue for a while, now |
I could fix the |
8764cc8
to
dd31464
Compare
Tests are passing now on Ceph 18.2.1. Now switching back to 18.2.0, as that upgrade is separate. Will notify here when it's done. Please somebody review my addition of the old Python packages. |
It's working. But the ofborg eval failed: https://gist.github.com/GrahamcOfBorg/8b4e2ab113b7b1f5b634299c41402fea
I can see in
How comes this alias seems to work in my local build, but not for ofborg? |
dd31464
to
7c202e3
Compare
@lilyinstarlight Explained:
I fixed that now by using |
8659335
to
08c3105
Compare
…1858. Fixes NixOS#241482. Also fix test putting cluster in unhealthy `POOL_APP_NOT_ENABLED` state; this seems to be the default state with Ceph 18.2.1 at least, and it does not hurt to fix it now already in the way the Ceph docs say. Also revert "nixosTests.ceph-single-node: remove dashboard check" This reverts commit 41b27d7.
6dce5d5
to
084a5b7
Compare
@dotlambda Please take another look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I think you can split out the test changes into something like nixos/tests/ceph: re-enable dashboard tests
.
They were disabled only because of the |
Now newly ofborg complains
Not sure what that is yet. |
Aha, commit ffbf6f2 I force-pushed a fix. |
084a5b7
to
14302b7
Compare
cryptography-vectors = (self.callPackage ../../../development/python-modules/cryptography/vectors.nix { }).overridePythonAttrs (old: { | ||
src = fetchPypi { | ||
pname = "cryptography_vectors"; | ||
version = cryptographyOverrideVersion; | ||
hash = "sha256-hGBwa1tdDOSoVXHKM4nPiPcAu2oMYTPcn+D1ovW9oEE="; | ||
}; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just disable tests in cryptography instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SuperSandro2000 Is there a drawback of running the tests? Given that they are not slow, it seems nice to check that the version/src override doesn't add any bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a drawback of running the tests?
Ah, I see, because it requires the overridability of cryptography-vectors
which you want to avoid in #281858 (comment).
OK, I've disabled tests as requested in any case with the latest force-push of this PR.
@@ -166,7 +169,73 @@ let | |||
|
|||
# Watch out for python <> boost compatibility | |||
python = python310.override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already dropped python 3.10 support, so the situation here will only get much worse over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ceph 19 will address this, with Python 3.13 support:
let | ||
cryptography-vectors = callPackage ./vectors.nix { }; | ||
in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please revert this. I do not want people to use this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…1858. Fixes NixOS#241482. Also fix test putting cluster in unhealthy `POOL_APP_NOT_ENABLED` state; this seems to be the default state with Ceph 18.2.1 at least, and it does not hurt to fix it now already in the way the Ceph docs say. Also revert "nixosTests.ceph-single-node: remove dashboard check" This reverts commit 41b27d7.
14302b7
to
506b215
Compare
After rebasing on
But it is a flaky test, re-running So not related to this PR. |
Ceph tests are passing again. |
I believe I addressed all feedback, merging. |
Description of changes
Fixes #281027
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.