-
-
Notifications
You must be signed in to change notification settings - Fork 10
Tidy some of the bash scripts #183
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
Conversation
Co-authored-by: Glenn Jackman <[email protected]>
bin/bootstrap.sh
Outdated
| mapfile -t files < <(git grep --files-with-matches replace-this-with-the-track-slug) | ||
| sed -i "s/replace-this-with-the-track-slug/${SLUG}/g" "${files[@]}" |
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.
On second thought, xargs might be better:
| mapfile -t files < <(git grep --files-with-matches replace-this-with-the-track-slug) | |
| sed -i "s/replace-this-with-the-track-slug/${SLUG}/g" "${files[@]}" | |
| git grep --null --files-with-matches replace-this-with-the-track-slug) \ | |
| | xargs --null sed -i "s/replace-this-with-the-track-slug/${SLUG}/g" |
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'm not generally a huge fan of xargs. Is there anything wrong with passing the files to sed? I really don't think we need to protect against newlines in the file names here.
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.
and it's very unlikely we'll find thousands of filenames.
|
I've tried running the bootstrap script on my Mac (which Jeremy and I both use), and I got: |
Is bash 3 the target shell here? What about the Docker scripts -- should those target POSIX sh? Or can those target modern bash if they run on a modern Linux image? |
|
Working on a Mac, we'll also have trouble with We do need to establish a baseline set of tools. |
Anything that maintainers like Jeremy and I use must work on Macs. The scripts that run in Docker images can likely use more modern version of bash. 95% of the time, the base Docker image that people use is Alpine or Ubuntu. If we can target the scripts to run there, we'll be fine. |
@ErikSchierboom are you OK being told to go install a modern bash from homebrew? @IsaacG we can just replace perl -i "s/replace-this-with-the-track-slug/${SLUG}/g" "${files[@]}" |
|
I'm fine with that if Jeremy is too |
|
Sounds like we need input from @iHiD here :) I haven't written Perl in about a decade and I'm not a fan but we can totally swap |
|
@ErikSchierboom Any last thoughts before merging? |
|
A thought about mapfile and bash 3.2 on the Mac: we can add a check into the script for the minimum required version: if (( "${BASH_VERSINFO[0]}${BASH_VERSINFO[1]}" < 44 )); then
echo "This script requires bash version 4.4 at minimum." >&2
echo "You can install a modern bash from Homebrew - https://brew.sh" >&2
exit 1
fi |
I need to test that it works :) |
ErikSchierboom
left a comment
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.
One last suggestion and then we're good
Co-authored-by: Erik Schierboom <[email protected]>
No description provided.