Script Overhaul #421

emabrey · 2021-07-31T17:07:53Z

As referenced in https://github.com/tenacityteam/tenacity/pull/407 we needed to overhaul our contributor scripts and cleanup their directory layout.

Checklist

I have signed off my commits using -s or Signed-off-by* (See: Contributing § DCO)
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code*
I made sure the title of the PR reflects the core meaning of the issue you are solving*
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

* indicates required

nbsp · 2021-07-31T18:42:42Z

I think it might make more sense to separate the user scripts entirely, and put them all in contrib/{images/locale/etc.}.

emabrey · 2021-08-02T19:16:32Z

Doing that makes having a default location for where the scripts are modifying things more awkward. Right now the scripts are immediately next to the directories they modify.

fossdd · 2021-08-03T16:46:10Z

images/scripts/degimpify.sh

+    input_dirs[0]="${BASH_SOURCE[0]}/../../images/EditButtons"
+    input_dirs[1]="${BASH_SOURCE[0]}/../../images/TranscriptionImages"
+    input_dirs[2]="${BASH_SOURCE[0]}/../../images/ControlButtons"


See lines 3-8.

I tried this syntax on a few of my systems and it sometimes does not work.
Also, collapsed it to $DIRNAME in my patch

Suggested change

input_dirs[0]="${BASH_SOURCE[0]}/../../images/EditButtons"

input_dirs[1]="${BASH_SOURCE[0]}/../../images/TranscriptionImages"

input_dirs[2]="${BASH_SOURCE[0]}/../../images/ControlButtons"

DIRNAME=$(dirname "${BASH_SOURCE[0]}")

input_dirs[0]="$DIRNAME/../../images/EditButtons"

input_dirs[1]="$DIRNAME/../../images/TranscriptionImages"

input_dirs[2]="$DIRNAME/../../images/ControlButtons"

Semisol · 2021-08-04T23:19:03Z

images/scripts/degimpify.sh

+    input_dirs[0]="${BASH_SOURCE[0]}/../../images/EditButtons"
+    input_dirs[1]="${BASH_SOURCE[0]}/../../images/TranscriptionImages"
+    input_dirs[2]="${BASH_SOURCE[0]}/../../images/ControlButtons"


I tried this syntax on a few of my systems and it sometimes does not work.
Also, collapsed it to $DIRNAME in my patch

Suggested change

input_dirs[0]="${BASH_SOURCE[0]}/../../images/EditButtons"

input_dirs[1]="${BASH_SOURCE[0]}/../../images/TranscriptionImages"

input_dirs[2]="${BASH_SOURCE[0]}/../../images/ControlButtons"

DIRNAME=$(dirname "${BASH_SOURCE[0]}")

input_dirs[0]="$DIRNAME/../../images/EditButtons"

input_dirs[1]="$DIRNAME/../../images/TranscriptionImages"

input_dirs[2]="$DIRNAME/../../images/ControlButtons"

emabrey · 2021-08-04T23:24:05Z

Yeah BASH_SOURCE isn't perfect. It doesn't take symlinks into account for example. https://stackoverflow.com/questions/421772/how-can-a-bash-script-know-the-directory-it-is-installed-in-when-it-is-sourced-w

Semisol · 2021-08-04T23:25:13Z

dirname is sort of the best thing we can get that works with most systems I assume?

emabrey · 2021-08-04T23:32:42Z

dirname is sort of the best thing we can get that works with most systems I assume?

It should be sufficient. There are ways to support symlinks but I don't think it's needed.

nbsp

Could we change the scripts to use /bin/sh instead of /bin/bash? Not every system has bash installed on it.

This might not be as simple as changing the shebang; we'll need to check for bashisms throughout the scripts and remove them.

Semisol · 2021-08-08T22:52:59Z

we'll need to check for bashisms throughout the scripts and remove them

BASH_SOURCE, and ${BLABLA[idx]} does not work on sh I believe

emabrey · 2021-08-08T22:54:03Z

Could we change the scripts to use /bin/sh instead of /bin/bash? Not every system has bash installed on it.

This might not be as simple as changing the shebang; we'll need to check for bashisms throughout the scripts and remove them.

Yeah I can do that.

nbsp · 2021-10-14T18:59:09Z

@emabrey mind if I take a crack at converting the files to /bin/sh?

n0toose · 2021-10-15T11:54:50Z

Considering that she is currently absent from the project, I see no reason for that to not happen. It's an already agreed approach, we better do something than nothing. @solfisher

Be-ing · 2021-10-15T11:56:35Z

If you want to make the scripts more portable, sure, why not?

nbsp · 2021-12-01T15:10:35Z

removing bashisms was simple, but I might've messed up the rebase -- if someone could please check to see I've not added/removed anything important

Update `msgfmt.py` to recent python 3 version. Instruct CMake to load `Gettext.Tools` version `0.21.0.1` when using NuGet. Cleanup `regen_POT_file.sh`. Correct list of languages in `LanguageNames.txt` Cleanup various locale Perl scripts. Make `find-PO-no-Id.sh` use glob file access. Cleanup output of `locale_diagnostics.sh` Cleanup bash scripts and add double-quoting to prevent string issues. Move various locale scripts to `locale/scripts` directory. Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

some Unix-like operating systems, namely Alpine, don't come with bash preinstalled. scripts in the lib-src/ directory were left untouched. Signed-off-by: Sol Fisher Romanoff <sol@solfisher.com>

fossdd suggested changes Aug 3, 2021

View reviewed changes

Semisol suggested changes Aug 4, 2021

View reviewed changes

nbsp suggested changes Aug 8, 2021

View reviewed changes

n0toose marked this pull request as draft September 19, 2021 14:24

nbsp force-pushed the script-overhaul branch from 67cc777 to 78cfac9 Compare December 1, 2021 15:08

nbsp force-pushed the script-overhaul branch from 78cfac9 to fc6498c Compare December 2, 2021 07:33

emabrey added 3 commits December 2, 2021 09:34

Improve locale_diagnostics.sh

8b0bf90

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Return msgfmt.py to version 1.2

d118b79

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

nbsp force-pushed the script-overhaul branch from fc6498c to 3128318 Compare December 2, 2021 07:34

Change bash scripts to be POSIX-compatible

f395687

some Unix-like operating systems, namely Alpine, don't come with bash preinstalled. scripts in the lib-src/ directory were left untouched. Signed-off-by: Sol Fisher Romanoff <sol@solfisher.com>

nbsp force-pushed the script-overhaul branch from 3128318 to f395687 Compare December 2, 2021 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script Overhaul #421

Script Overhaul #421

emabrey commented Jul 31, 2021

nbsp commented Jul 31, 2021

emabrey commented Aug 2, 2021

fossdd Aug 3, 2021

Semisol Aug 4, 2021 •

edited

Semisol Aug 4, 2021 •

edited

emabrey commented Aug 4, 2021

Semisol commented Aug 4, 2021

emabrey commented Aug 4, 2021

nbsp left a comment

Semisol commented Aug 8, 2021

emabrey commented Aug 8, 2021

nbsp commented Oct 14, 2021

n0toose commented Oct 15, 2021

Be-ing commented Oct 15, 2021

nbsp commented Dec 1, 2021

-    input_dirs[0]="${BASH_SOURCE[0]}/../../images/EditButtons"
-    input_dirs[1]="${BASH_SOURCE[0]}/../../images/TranscriptionImages"
-    input_dirs[2]="${BASH_SOURCE[0]}/../../images/ControlButtons"
+    DIRNAME=$(dirname "${BASH_SOURCE[0]}")
+    input_dirs[0]="$DIRNAME/../../images/EditButtons"
+    input_dirs[1]="$DIRNAME/../../images/TranscriptionImages"
+    input_dirs[2]="$DIRNAME/../../images/ControlButtons"

Script Overhaul #421

Are you sure you want to change the base?

Script Overhaul #421

Conversation

emabrey commented Jul 31, 2021

nbsp commented Jul 31, 2021

emabrey commented Aug 2, 2021

fossdd Aug 3, 2021

Choose a reason for hiding this comment

Semisol Aug 4, 2021 • edited

Choose a reason for hiding this comment

Semisol Aug 4, 2021 • edited

Choose a reason for hiding this comment

emabrey commented Aug 4, 2021

Semisol commented Aug 4, 2021

emabrey commented Aug 4, 2021

nbsp left a comment

Choose a reason for hiding this comment

Semisol commented Aug 8, 2021

emabrey commented Aug 8, 2021

nbsp commented Oct 14, 2021

n0toose commented Oct 15, 2021

Be-ing commented Oct 15, 2021

nbsp commented Dec 1, 2021

Semisol Aug 4, 2021 •

edited

Semisol Aug 4, 2021 •

edited