ceph: patch getopt path at build time by josh · Pull Request #440224 · NixOS/nixpkgs

josh · 2025-09-04T18:41:59Z

This patches the getopt path directly into Ceph during package build, so that ceph-osd-prestart.sh no longer depends on the NixOS module adding getopt via systemd’s path.

Things done

Add a 👍 reaction to pull requests you find important.

Copilot

Pull Request Overview

This PR patches the getopt path directly into Ceph's shell scripts during package build time, eliminating the dependency on the NixOS module to provide getopt via systemd's path.

Adds getopt as a build dependency to the Ceph package
Patches shell scripts to use absolute paths to getopt binary
Removes getopt from the systemd service path in the NixOS module

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pkgs/tools/filesystems/ceph/default.nix	Adds getopt dependency and patches shell scripts with absolute getopt paths
nixos/modules/services/network-filesystems/ceph.nix	Removes getopt from systemd service path since it's now hardcoded in scripts

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-04T18:42:29Z

pkgs/tools/filesystems/ceph/default.nix

+      substituteInPlace \
+        src/ceph-osd-prestart.sh \
+        src/ceph-post-file.in \
+        src/init-ceph.in \
+        --replace-fail "GETOPT=/usr/local/bin/getopt" "GETOPT=${getopt}/bin/getopt" \
+        --replace-fail "GETOPT=getopt" "GETOPT=${getopt}/bin/getopt"
+


[nitpick] The substituteInPlace command spans multiple files and replacements. Consider splitting this into separate commands for each file or group of related files to improve readability and make it easier to debug if one of the files doesn't exist.

Suggested change

substituteInPlace \

src/ceph-osd-prestart.sh \

src/ceph-post-file.in \

src/init-ceph.in \

--replace-fail "GETOPT=/usr/local/bin/getopt" "GETOPT=${getopt}/bin/getopt" \

--replace-fail "GETOPT=getopt" "GETOPT=${getopt}/bin/getopt"

substituteInPlace src/ceph-osd-prestart.sh \

--replace-fail "GETOPT=/usr/local/bin/getopt" "GETOPT=${getopt}/bin/getopt" \

--replace-fail "GETOPT=getopt" "GETOPT=${getopt}/bin/getopt"

substituteInPlace src/ceph-post-file.in \

--replace-fail "GETOPT=/usr/local/bin/getopt" "GETOPT=${getopt}/bin/getopt" \

--replace-fail "GETOPT=getopt" "GETOPT=${getopt}/bin/getopt"

substituteInPlace src/init-ceph.in \

--replace-fail "GETOPT=/usr/local/bin/getopt" "GETOPT=${getopt}/bin/getopt" \

--replace-fail "GETOPT=getopt" "GETOPT=${getopt}/bin/getopt"

nh2 · 2025-09-22T13:35:56Z

pkgs/tools/filesystems/ceph/default.nix

@@ -503,6 +504,10 @@ rec {
      substituteInPlace src/client/fuse_ll.cc \
        --replace-fail "mount -i -o remount" "${util-linux}/bin/mount -i -o remount"

+      substituteInPlace src/{ceph-osd-prestart.sh,ceph-post-file.in,init-ceph.in} \


@josh Compared to the current state, where getopt is on PATH for all Ceph services, this approach runs the risk of missing programs that use it and getting outdated.

I think if we want to go this way (which has the benefit that Ceph binaries also work standalone, when not running under NixOS systemd services), we should at least have a grep for exhaustiveness in the source repo.

For example:

$ git grep GETOPT= src/ceph-osd-prestart.sh: GETOPT=/usr/local/bin/getopt src/ceph-osd-prestart.sh: GETOPT=getopt src/ceph-post-file.in: GETOPT=/usr/local/bin/getopt src/ceph-post-file.in: GETOPT=getopt src/init-ceph.in: GETOPT=/usr/local/bin/getopt src/init-ceph.in: GETOPT=getopt src/script/run-make.sh: GETOPT=/usr/local/bin/getopt src/script/run-make.sh: GETOPT=getopt src/script/run_tox.sh: GETOPT=/usr/local/bin/getopt src/script/run_tox.sh: GETOPT=getopt src/tools/ceph-monstore-update-crush.sh: GETOPT=/usr/local/bin/getopt src/tools/ceph-monstore-update-crush.sh: GETOPT=getopt src/tools/setup-virtualenv.sh: GETOPT="/usr/local/bin/getopt" src/tools/setup-virtualenv.sh: GETOPT=getopt

finds a couple more files (unclear so far if relevant).

The other scripts were only used at build time and don't exist in the final output. Maybe adding a grep assertion on $out that there are no missed GETOPT=getopt be good? Or should we just patch src more aggressively even for build time scripts?

Maybe adding a grep assertion on $out that there are no missed GETOPT=getopt be good?

@josh Yes, I think such a grep would be good enough.

josh · 2025-09-22T21:10:20Z

@nh2 pushed the assertion. Good idea for sure.

I assume we might be blocked on merging until #442652 is resolved. This PR's merge base is a little older so it's building fine, but can't test once rebased against master.

@nh2 If you have a chance, any thoughts on #440226? This PR came out while working on that idea. Thanks!

benaryorg

I'm not personally using the scripts in question (I think), so I can't really test much here, however if the regular tests pass then this LGTM.

benaryorg · 2025-09-29T12:56:26Z

pkgs/tools/filesystems/ceph/default.nix

+      ! grep -R 'GETOPT=getopt' $out
+      ! grep -R 'GETOPT=/usr/local/bin/getopt' $out


please use -F too, not only does it make sure no funny regex stuff happens (in case the string changes in the future), but it also improves speed quite a bit (at least last time I checked)

do we really need -R? Unless I'm mistaken -r should suffice since the replaced files are part of the output, not just symlinks. That'd help avoiding to grep any potential linked-in dependencies (again saving some time)

That's mostly nit-picking tho.

josh · 2025-09-29T16:56:13Z

Tweaked the grep flags as suggested.

The primary ceph-osd-prestart.sh script used by the osd service is the one I encountered. What is nice is that the existing ceph NixOS vm tests excerise this path. I can confirm there will be a test failure there starting a osd if getopt isn’t patched correctly. Thanks to whoever wrote those!

josh · 2025-10-21T22:21:16Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 440224
Commit: e59c784a9626c70166d94a4664d640978a8bf2f3

`x86_64-linux`

⏩ 2 packages blacklisted:

nixos-install-tools
tests.nixos-functions.nixos-test

✅ 14 packages built:

ceph (ceph-dev)
ceph-client
ceph-csi
ceph.dev (ceph-dev.dev)
ceph.doc (ceph-dev.doc)
libceph (ceph.lib, libceph.dev, libceph.doc, libceph.lib, libceph.man)
ceph.man (ceph-dev.man)
qemu_full
qemu_full.debug
qemu_full.doc
qemu_full.ga
sambaFull (samba4Full)
sambaFull.dev (samba4Full.dev)
sambaFull.man (samba4Full.man)

`aarch64-linux`

⏩ 2 packages blacklisted:

nixos-install-tools
tests.nixos-functions.nixos-test

✅ 10 packages built:

ceph (ceph-dev)
ceph-client
ceph-csi
ceph.dev (ceph-dev.dev)
ceph.doc (ceph-dev.doc)
libceph (ceph.lib, libceph.dev, libceph.doc, libceph.lib, libceph.man)
ceph.man (ceph-dev.man)
qemu_full
qemu_full.doc
qemu_full.ga

nh2

All feedback was addressed, merging.

Tests pass fo rme (but https://discourse.nixos.org/t/path-is-not-in-the-nix-store-during-nixos-rebuild-switch/62719/14 made me extra effort for testing them).

josh · 2025-10-25T00:46:15Z

@nh2 thank you!

nh2 · 2025-10-25T10:45:51Z

@josh Thank you for the contribution!

Copilot AI review requested due to automatic review settings September 4, 2025 18:42

Copilot AI reviewed Sep 4, 2025

View reviewed changes

josh requested review from benaryorg and nh2 September 4, 2025 18:42

nix-owners bot requested review from adevress, johanot and krav September 4, 2025 18:48

josh force-pushed the ceph-getopt branch from e69d4dc to 9013c71 Compare September 4, 2025 18:52

nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 4, 2025

nh2 reviewed Sep 22, 2025

View reviewed changes

josh force-pushed the ceph-getopt branch from 9013c71 to 648dae8 Compare September 22, 2025 20:21

josh requested a review from nh2 September 26, 2025 18:20

benaryorg reviewed Sep 29, 2025

View reviewed changes

josh force-pushed the ceph-getopt branch from 648dae8 to bc4d2b7 Compare September 29, 2025 16:55

josh force-pushed the ceph-getopt branch from bc4d2b7 to e59c784 Compare October 21, 2025 00:29

ceph: patch getopt path at build time

175a0fc

josh force-pushed the ceph-getopt branch from e59c784 to 175a0fc Compare October 24, 2025 16:43

nh2 approved these changes Oct 25, 2025

View reviewed changes

nh2 added this pull request to the merge queue Oct 25, 2025

Merged via the queue into NixOS:master with commit 039fab1 Oct 25, 2025
26 of 30 checks passed

josh deleted the ceph-getopt branch October 25, 2025 00:46

benaryorg mentioned this pull request Oct 25, 2025

pkgs/ceph: move to by-name #455186

Merged

13 tasks

nh2 mentioned this pull request Oct 26, 2025

Build ceph with Python 3.12; move to pkgs/by-name #443671

Open

		! grep -R 'GETOPT=getopt' $out
		! grep -R 'GETOPT=/usr/local/bin/getopt' $out

Uh oh!

Conversation

josh commented Sep 4, 2025

Things done

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

nh2 Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

josh Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

nh2 Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

josh commented Sep 22, 2025

Uh oh!

benaryorg left a comment

Choose a reason for hiding this comment

Uh oh!

benaryorg Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

josh commented Sep 29, 2025

Uh oh!

josh commented Oct 21, 2025

nixpkgs-review result

x86_64-linux

aarch64-linux

Uh oh!

nh2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josh commented Oct 25, 2025

Uh oh!

nh2 commented Oct 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`nixpkgs-review` result

`x86_64-linux`

`aarch64-linux`