Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve scripts and tool configurations #1693
Improve scripts and tool configurations #1693
Changes from all commits
8c4df3c
f3be76f
7dd8add
21875b5
0920371
e973f52
be53823
e604b46
19dfbd8
7110bf8
c7cdaf4
f6dbba2
5060c9d
d5479b2
52f9a68
b88d07e
d36818c
4ba5ad1
5d8ddd9
a872d9c
9b9de11
5d15063
6de86a8
f094909
fc96980
b98f15e
7cca7d2
e4e009d
e16e4c0
62c024e
9e245d0
c2472e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This conveys spacing much better thanks to the separate
echo
line, even though I have a feeling this was done for portability as$'magic'
might not be allowed everywhere.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 did it for spacing, and to to take fuller advantage of
echo
to have fewer\n
sequences. Although it's true that$'
'
is not POSIX and shouldn't be used in a#!/bin/sh
script, this script is abash
script and$'
'
is a available inbash
on all systems. (ksh
,zsh
, and probably some other shells also have$'
'
, but not all POSIX-compatible shells have it.)Regarding spacing, the table is currently formatted using
printf
and this has the advantage that its own output spacing can be adjusted by, for example, changing14
to15
. But I originally usedprintf
for it because I was preferringprintf
toecho
, because originally I was doing this inline inMakefile
, and aMakefile
runs commands using/bin/sh
(unlessSHELL
is assigned in theMakefile
itself). This was to follow the practice of avoidingecho
in portablesh
scripts, at least when displaying data read from files.But that is not necessary in a
#!/bin/bash
script, where we know howecho
works and whereecho
doesn't expand escape sequences by default. So it would actually be okay, at this point, to go back to displaying the table this way:This is arguably more readable, and arguably conveys spacing the best. I'd be happy to change it to this if you like.
(Another option could be to go in the opposite direction and make this
check-version.sh
script, and maybe thebuild-release.sh
script as well, a#!/bin/sh
script, which is feasible.)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.
Since
trap
is supported in POSIX shells and thussh
, making these scripts more portable seems valuable, even though I think that as the project currently is setup there isn't any need as it's only me using them.Alternatives involve running these on CI which also supports
bash
everywhere (at least so it seems).If you think there is benefits to portability assuming that one day CI can perform trusted publishes, I think it's fair to adjust for portability next time you touch them. Otherwise, it would be just as fair to change
printf
withecho
as shown above. It's perfectly optional as well.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.
The desired interaction of
trap
andset -e
may be achievable insh
. Even if not, we don't have to usetrap
; it can be replaced even without making the control-flow logic more complicated or error prone. For example, the script could accept-q
to not append the failure message, and otherwise invoke itself with-q
, check the result, print the message if it failed, and exit with the original failing exit code. (There is also an argument to be made thatset -e
shouldn't be relied on at all.)There are not many systems where
bash
doesn't exist and can't be made available. However, due to the problem described in 729372f--which only happens when MinGWmake
is being used on a native Windows system that has also WSL installed, and does not happen outsidemake
nor on other systems--the scripts'bash
hashbangs are written as#!/bin/bash
rather than#!/usr/bin/env bash
, and thus assumebash
is in/bin
.Making them
#!/bin/sh
scripts would also fix that. It is even safer to assumesh
is inbin
than to assumeenv
is in/usr/bin
, so#!/usr/bin/env sh
is rarely used. (Anyway, there is no corresponding WSL-relatedsh.exe
inSystem32
.) So, when combined with the other minor reasons for using#!/bin/sh
, that is probably worth doing.As you've suggested, next time I work on those scripts I'll look into making them POSIX
sh
scripts. However, this would be unrelated to facilitating trusted publishing. CI does supportbash
everywhere, at least on runners hosted by GitHub Actions.