Skip to content
This repository has been archived by the owner on Dec 18, 2022. It is now read-only.

Script Overhaul #421

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Script Overhaul #421

wants to merge 4 commits into from

Conversation

emabrey
Copy link
Member

@emabrey emabrey commented Jul 31, 2021

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
Copy link
Contributor

nbsp commented Jul 31, 2021

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

@emabrey
Copy link
Member Author

emabrey commented Aug 2, 2021

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.

Comment on lines +15 to +17
input_dirs[0]="${BASH_SOURCE[0]}/../../images/EditButtons"
input_dirs[1]="${BASH_SOURCE[0]}/../../images/TranscriptionImages"
input_dirs[2]="${BASH_SOURCE[0]}/../../images/ControlButtons"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See lines 3-8.

Copy link
Contributor

@Semisol Semisol Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Comment on lines +15 to +17
input_dirs[0]="${BASH_SOURCE[0]}/../../images/EditButtons"
input_dirs[1]="${BASH_SOURCE[0]}/../../images/TranscriptionImages"
input_dirs[2]="${BASH_SOURCE[0]}/../../images/ControlButtons"
Copy link
Contributor

@Semisol Semisol Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

emabrey commented Aug 4, 2021

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
Copy link
Contributor

Semisol commented Aug 4, 2021

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

@emabrey
Copy link
Member Author

emabrey commented Aug 4, 2021

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.

Copy link
Contributor

@nbsp nbsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Semisol commented Aug 8, 2021

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
Copy link
Member Author

emabrey commented Aug 8, 2021

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.

@n0toose n0toose marked this pull request as draft September 19, 2021 14:24
@nbsp
Copy link
Contributor

nbsp commented Oct 14, 2021

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

@n0toose
Copy link
Member

n0toose commented Oct 15, 2021

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
Copy link
Contributor

Be-ing commented Oct 15, 2021

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

@nbsp
Copy link
Contributor

nbsp commented Dec 1, 2021

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 <[email protected]>
Signed-off-by: Emily Mabrey <[email protected]>
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 <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants