Skip to content

fix patchShebangs in the presence of readonly scripts #462319

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.

Sign up for GitHub

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

Open
bmillwood wants to merge 4 commits into
base: staging
Choose a base branch
from
Open

fix patchShebangs in the presence of readonly scripts #462319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1235e7b
patchShebangs: update read-only-script test to catch #462298
bmillwood Nov 16, 2025
0274f94
patchShebangs: chmod only user permissions
bmillwood Nov 16, 2025
ff8fbe1
patchShebangs: Test 777 as well
bmillwood Nov 16, 2025
560f431
patchShebangs: Reset restoreReadOnly to empty every loop
bmillwood Nov 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkgs/build-support/setup-hooks/patch-shebangs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,9 @@ patchShebangs() {
sed -e "1 s|.*|#\!$escapedInterpreterLine|" "$f" > "$tmpFile"

# Make original file writable if it is read-only
local restoreReadOnly
local restoreReadOnly=
if [[ ! -w "$f" ]]; then
chmod +w "$f"
chmod u+w "$f"
restoreReadOnly=true
fi

Expand All @@ -144,7 +144,7 @@ patchShebangs() {
cat "$tmpFile" > "$f"
rm "$tmpFile"
if [[ -n "${restoreReadOnly:-}" ]]; then
chmod -w "$f"
chmod u-w "$f"
fi

touch --date "$timestamp" "$f"
Expand Down
40 changes: 29 additions & 11 deletions pkgs/test/stdenv/patch-shebangs.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,19 +164,37 @@ let
. ${../../stdenv/generic/setup.sh}
. ${../../build-support/setup-hooks/patch-shebangs.sh}
mkdir -p $out/bin
echo "#!/bin/bash" > $out/bin/test
echo "echo -n hello" >> $out/bin/test
chmod 555 $out/bin/test
original_perms=$(stat -c %a $out/bin/test)
patchShebangs $out/bin/test
new_perms=$(stat -c %a $out/bin/test)
if ! [ "$original_perms" = "$new_perms" ]; then
echo "Permissions changed from $original_perms to $new_perms"
exit 1
fi
modes=(555 575 755 775 777)
Copy link
Contributor

@doronbehar doronbehar Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmillwood Reading this again, I was thinking maybe the test should be renamed to preserve-permissions to reflect it doesn't only test read-only scripts?

for mode in "''${modes[@]}"
do
target=$out/bin/test-$mode
echo "#!/bin/bash" > "$target"
echo "echo -n hello" >> "$target"
chmod $mode "$target"
if ! [ "$mode" = "$(stat -c %a "$target")" ]; then
echo "chmod didn't set up test permissions as expected"
exit 1
fi
done
# This is structured as two loops + one patchShebangs so that at
# least one test exercises patchShebangs on multiple scripts in one
# invocation.
patchShebangs $out/bin
for mode in "''${modes[@]}"
do
target=$out/bin/test-$mode
new_perms=$(stat -c %a "$target")
if ! [ "$mode" = "$new_perms" ]; then
echo "Permissions changed from $mode to $new_perms"
exit 1
fi
done
''
];
assertion = "grep '^#!${lib.getExe earlyBash}' $out/bin/test > /dev/null";
# This tests that any of the test files have this line, when we should
# really be testing all of them. But it's a little redundant with other
# tests anyway.
assertion = "grep '^#!${lib.getExe earlyBash}' $out/bin/test-* > /dev/null";
})
// {
meta = { };
Expand Down
Loading