Page MenuHomePhabricator

Make script shebangs POSIX friendly
ClosedPublic

Authored by jon on Apr 19 2022, 9:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 2:01 AM
Unknown Object (File)
Sat, Nov 2, 2:01 AM
Unknown Object (File)
Sat, Nov 2, 2:01 AM
Unknown Object (File)
Sat, Nov 2, 2:01 AM
Unknown Object (File)
Fri, Nov 1, 8:15 PM
Unknown Object (File)
Mon, Oct 28, 2:33 AM
Unknown Object (File)
Fri, Oct 25, 8:38 PM
Unknown Object (File)
Sat, Oct 19, 12:04 AM

Details

Summary

Sourcing bash from /bin/bash doesn't work on NixOS, and on other platforms it ends up using the system-default bash rather than the one sourced by Nix.

/usr/bin/env is a utility which will find the related command by searching PATH, this allows for nix to introduce other interpreters. It's also considered a "best practice" as some systems such as android, solaris, and other may also not put their default interpreters in /bin

Depends on D3274

Test Plan

This shouldn't invalidate previous behavior of scripts, as most invocations will resolve to /bin/bash

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This seems uncontroversial, so I'll accept. But it would've been good to provide more context about why this change is necessary. I Googled and found this and this, which explain that #!/usr/bin/env bash uses whatever bash is in $PATH. But there is still some missing context here. I assume the issue is that #!/bin/bash circumvents Nix by using a direct path to something outside of Nix.

Ideally I wouldn't need to Google here at all, and ideally I wouldn't have to make assumptions about the purpose of the diff. As a diff author, your job should be to make diffs as easy as possible to review. Work should be shifted away from the reviewer, to the author. The fact that I wrote multiple sentences here, compared to <10 words in the diff description and test plan... it should be a strong signal to you that you aren't doing enough to explain your changes, and forcing a lot of work on your reviewer.

Ideally, I accept without any comments at all because you've done the work to anticipate my questions ahead of time.

This revision is now accepted and ready to land.Apr 20 2022, 10:49 AM

(Separately, I should never be a first-pass reviewer like this – @varun would've probably been the right choice here, as he is working with you to help on the Nix stuff)

Update reference to nix diff

jon edited the test plan for this revision. (Show Details)

Should we do this for all of the scripts in keyserver/bash as well?

It shouldn't affect anything, but I can amend this

Also make keyserver/bash scripts more POSIX friendly

This revision was automatically updated to reflect the committed changes.