fix patchShebangs in the presence of readonly scripts

[bmillwood]

• Fixes patchShebangs: fails with "chmod: [script]: new permissions are [...], not [...]" #462298
• Replaces chmod -w with chmod u-w (and likewise +w) in patch-shebangs.sh, so that the only permission we touch is the one that we tested (and the one that we need). Restoring the mode just wasn't working correctly before, because -w isn't an inverse of +w (except in special cases). As documented in the issue, this is necessary to fix working on scripts with mode 575 (though who uses those?)
  • Added a test case that catches this.
• Fixes restoreReadOnly stale value by resetting it to an empty string on every loop iteration. This was a much more serious problem, leading the hook to break on scripts with mode 777 as long as you'd already seen a readonly script earlier this invocation.
  • Testing this requires a single invocation of patchShebangs that visits a readonly file before a read-write one. In general the order of patchShebangs is the order of the embedded find operation, which is nondeterministic, but I think find processes its command line arguments in order, so you could do something like patchShebangs test-555 test-777 and I think that would do it. This would feel very specific to this one bug, though, and unlikely to be useful in future.

Testing this comprehensively is made more tricky by the fact that modifying this script forces a rebuild of everything. I worked around this by copying the fixed script to another location and replacing the line in the test that sources the real script with one that sources my fixed script (and cherry-picking all my test changes). The test passes in this setup (and fails on the original script).

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/6107

[DavHau]

You can actually prevent the problem of too many rebuilds on each change by using bootstrapTools for the builder, as I did here: https://github.com/NixOS/nixpkgs/pull/411981/files#diff-d762697454f1be50b0e3ba17fe71962baef159923f79b79e2471b0060408fa08

[doronbehar]

Changes look good to me in general, though I haven't tested or verified the modified test is fixed by the latter commits.

[doronbehar]

Hmm maybe GitHub is fooling me?

[doronbehar]

It is not. Checked out locally the commits and saw the same issue.

[bmillwood]

oh, but you are reviewing the individual commits, not just the PR diff? I'm not sure what the convention is here but I've been assuming that only the final state is important (e.g. if someone doesn't like something about my PR, it is sufficient to add a new commit addressing their comment, rather than rewriting old commits to do so)

[doronbehar]

oh, but you are reviewing the individual commits, not just the PR diff?

Correct.

I'm not sure what the convention is here

This is under debate:

• CONTRIBUTING: Clarify commit separation convention #400934

And I should admit that I'm reviewing according to my own conventions which are a pretty similar to what's suggested there :).

but I've been assuming that only the final state is important (e.g. if someone doesn't like something about my PR, it is sufficient to add a new commit addressing their comment, rather than rewriting old commits to do so)

I think that there is value to multiple commits in a PR, because often some commits are justified no matter what are the other commits. However sometimes the latter commits are the main goal of the PR.

In this PR, the separation makes sense too because your are fixing multiple issues that are related. I just think that when doing that there's value in minimizing diff noise such as noted above.

[doronbehar]

@bmillwood I fixed the merge conflicts and fixed the comments I had before.

[doronbehar]

This style fits the style used for other variables, that are not necessarily declared as local.

[doronbehar]

This style fits the style used for other variables, that are not necessarily declared as local.

@bmillwood what do you think about using explicit true & false strings for restoreReadOnly?

[bmillwood]

I chose the fix that involved a smaller diff to the code I was modifying. The thing you propose would work too, but I don't see a strong enough reason to prefer it.

[bmillwood]

@bmillwood I fixed the merge conflicts and fixed the comments I had before.

I didn't realise you were even able to push commits to my fork!

Anyway, I appreciate you trying to be helpful but:

• the previous state of the PR had already been approved by someone else, so I don't know if I want to change it to match your desired style,
• your new push contains changes that you wrote, but attributed to me; if you do this please attribute them to yourself instead.

[bmillwood]

(I pushed back my own changes in the meantime, but possibly once I've seen yours I'll restore them)

[bmillwood]

I rebased my changes and applied one of your comments. The other one I don't think is an improvement. [edit: and you rightly pointed out you'd proposed a third one that I'd forgotten about; commented on that one as well.]

(To be honest, I think the other one was also probably too minor to be spending either of our time on, but I did it anyway to avoid spending any more time on it.)

[doronbehar]

@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?