docker: add script help message for macOS #5779

wihobbs · 2024-03-08T21:52:22Z

Recently, several core developers have noted that the docker-run-checks.sh script borks when command-line options that specify values do so with an equals sign rather than a space. Since this is not super intuitive, update the usage/help message for that script accordingly.

I pulled out all the = signs from the usage message and added a note, but maybe one or the other is more desirable.

wihobbs · 2024-03-08T21:58:58Z

I should note that I was working on a patch (but haven't finished yet) as @trws recommended in Slack that would make strip = from options in lines 62-70 of the docker-run-checks.sh script. Maybe that's more desirable behavior than this...

grondo · 2024-03-08T22:01:54Z

src/test/docker/docker-run-checks.sh

- -p, --platform=NAME           Run on alternate platform (if supported)\n\
- -S, --flux-security-version=N Install flux-security vers N (default=$FLUX_SECURITY_VERSION)\n
- -j, --jobs=N                  Value for make -j (default=$JOBS)\n
+ -t, --tag TAG                 If checks succeed, tag image as NAME\n\


Thanks!

The commit message should be updated to use the Problem: statement form and wrapped at 72 chars.

Maybe we should move the test:

# check if running in OSX if [[ "$(uname)" == "Darwin" ]]; then # BSD getopt

to above where the help string is generated and make the use of = and MacOS warning conditional on that?

Another idea would be to detect BSD getopt and suggest installing GNU getopt instead.

These are just suggestions besides the commit message changes.

grondo · 2024-03-08T22:02:58Z

I should note that I was working on a patch (but haven't finished yet) as @trws recommended in Slack that would make strip = from options in lines 62-70 of the docker-run-checks.sh script. Maybe that's more desirable behavior than this...

Not a bad idea! I'm guessing that would work for either GNU or BSD getopt?

grondo · 2024-03-08T22:05:04Z

Also the prefix should probably be docker: or test: instead of scripts:, though it doesn't really matter that much

wihobbs · 2024-03-10T18:42:33Z

Not a bad idea! I'm guessing that would work for either GNU or BSD getopt?

My plan was to add a conditional under this line:

# check if running in OSX
if [[ "$(uname)" == "Darwin" ]]; then
    # BSD getopt

to strip out all of the equals signs in $* only on macOS before passing them to getopt. I'm assuming this would work with GNU getopt as well as BSD. Adding equals signs currently doesn't work even when forcing gnu getopt on mac:

$ PATH=/opt/homebrew/opt/gnu-getopt/bin/:$PATH ./docker-run-checks.sh --imagei=el8 -I --
WARNING: on Darwin/macOS be sure not to use equals signs (=) for specifying options; instead use
spaces. This is a known issue with macOS's default version of getopt.
docker-run-checks.sh: Invalid option '--imagei=el8'

I can look into making this more robust and portable in a separate PR (might need a bit of help with that).

Would you review again @grondo and let me know if this help message is okay? I used echo but this will show up every time someone runs this on a mac. That might clutter things unnecessarily...

I addressed the commit message changes.

grondo · 2024-03-11T16:23:51Z

I used echo but this will show up every time someone runs this on a mac. That might clutter things unnecessarily...

Yeah, I'd rather not see that when I run the script on Linux, or every time in our CI. Would it work to add the warning to the usage message on MacOS?

grondo · 2024-04-12T16:20:05Z

src/test/docker/docker-run-checks.sh

@@ -27,7 +27,7 @@ die() { echo -e "$prog: $@"; exit 1; }
 #
 declare -r long_opts="help,quiet,interactive,image:,flux-security-version:,jobs:,no-cache,no-home,distcheck,tag:,build-directory:,install-only,no-poison,recheck,unit-test-only,quick-check,inception,platform:,workdir:,system"
 declare -r short_opts="hqIdi:S:j:t:D:Prup:"
-declare -r usage="


The commit message should indicate the problem is only obvserved on MacOS.

grondo · 2024-04-12T21:45:53Z

src/test/docker/docker-run-checks.sh

+    export EXTRA_MACOS_STUFF="\n\
+    You are using BSD getopt on macOS. If you have gnu-getopt in your PATH, \n\
+    force the script to use that by setting FORCE_GNU_GETOPT=1.\n"
+    declare usage=${usage}${EXTRA_MACOS_STUFF}


I don't think you need to use declare again here since usage has already been declared. In fact, if this were in a function it might declare usage as local and the modifications would be lost, so I would remove use of declare here.

Also, I thought this issue was about BSD getopt not supporting =. Should that be mentioned in the extra help message? i.e. something like "either avoid using '=' between options and values or force the use of GNU getopt with FORCE_GNU_GETOPT=1"?

"either avoid using '=' between options and values

This message isn't necessary with the proposed changes which strip out all = in the GETOPT variable (line 65) which is effectively all = signs before the --. I can add it if you still want it but I just wanted to make sure you didn't miss line 65:

GETOPTS=`getopt $short_opts -- $(echo $* | sed "s/=/ /g")`

So I guess this PR title needs to be updated because it's not just a help message change anymore.

Oh, and the commit message too. Whoops.

Ok ignore (basically) everything I said above. This should be addressed now.

wihobbs · 2024-04-12T23:24:31Z

Ok, this is the behavior now when an equals sign w/ an option is passed to the script on macOS:

jupiter13 ~/flux-core (docker-macos-script)$  ./src/test/docker/docker-run-checks.sh --image=focal
docker-run-checks.sh: Invalid option '--image=focal'

Usage: docker-run-checks.sh [OPTIONS] -- [CONFIGURE_ARGS...]
Build docker image for CI builds, then run tests inside the new
container as the current user and group.

Uses the current git repo for the build.

Options:
 -h, --help                    Display this message
     --no-cache                Disable docker caching
     --no-home                 Skip mounting the host home directory
     --install-only            Skip make check, only make install
     --inception               Run tests as flux jobs
     --system                  Run under system instance
 -q, --quiet                   Add --quiet to docker-build
 -t, --tag=TAG                 If checks succeed, tag image as NAME
 -i, --image=NAME              Use base docker image NAME (default=bookworm)
 -p, --platform=NAME           Run on alternate platform (if supported)
 -S, --flux-security-version=N Install flux-security vers N (default=0.11.0)

 -j, --jobs=N                  Value for make -j (default=2)

 -d, --distcheck               Run 'make distcheck' instead of 'make check'
 -r, --recheck                 Run 'make recheck' after failure
 -u, --unit-test-only          Only run unit tests
     --quick-check             Only run check-prep and one basic test
 -P, --no-poison               Do not install poison libflux and flux(1)
 -D, --build-directory=DIRNAME Name of a subdir to build in, will be made
     --workdir=PATH            Use PATH as working directory for build
 -I, --interactive             Instead of running ci build, run docker
                                image with interactive shell.

    You are using BSD getopt on macOS. BSD getopt does not recognize '='
    between options. Use a space instead. If gnu-getopt is first in your
    PATH, force the script to use that by setting FORCE_GNU_GETOPT=1.

or this:

jupiter13 ~/flux-core (docker-macos-script)$ PATH="/opt/homebrew/opt/gnu-getopt/bin:$PATH" FORCE_GNU_GETOPT=1 ./src/test/docker/docker-run-checks.sh --image=focal  --
Building image focal for user hobbs17 60943 group=20
[+] Building 0.5s (16/16) FINISHED

grondo · 2024-04-13T01:42:29Z

src/test/docker/docker-run-checks.sh

+    export EXTRA_MACOS_STUFF="\n\
+    You are using BSD getopt on macOS. BSD getopt does not recognize '='\n\
+    between options. Use a space instead. If gnu-getopt is first in your\n\
+    PATH, force the script to use that by setting FORCE_GNU_GETOPT=1.\n"
+    usage=${usage}${EXTRA_MACOS_STUFF}


It shouldn't be necessary to use export here since EXTRA_MACOS_STUFF does not need to be exported to the environment of future commands. How about just:

usage="${usage}\n\ You are using BSD getopt on macOS. BSD getopt does not recognize '='\n\ between options. Use a space instead. If gnu-getopt is first in your\n\ PATH, force the script to use that by setting FORCE_GNU_GETOPT=1.\n"

Good catch @grondo, fixed.

Problem: recently, several core developers have noted that the docker-run-checks.sh script borks when command-line options that specify values do so with an equals sign rather than a space on macOS. Add a help message to show up on macOS reminding devs to strip all '=' and replace them with spaces in their command-line arguments if BSD getopt is used. Also, give users the option to set an environment variable to tell the script on macOS that it should assume GNU getopt is being used ('FORCE_GNU_GETOPT=1').

grondo

Looks great! Thanks!

codecov · 2024-04-15T22:43:03Z

Codecov Report

Merging #5779 (3d19d99) into master (7d61efd) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5779      +/-   ##
==========================================
- Coverage   83.30%   83.28%   -0.02%     
==========================================
  Files         514      514              
  Lines       82774    82774              
==========================================
- Hits        68955    68941      -14     
- Misses      13819    13833      +14

see 12 files with indirect coverage changes

wihobbs force-pushed the docker-macos-script branch from 35c4123 to a00110e Compare March 8, 2024 21:52

grondo reviewed Mar 8, 2024

View reviewed changes

wihobbs force-pushed the docker-macos-script branch 2 times, most recently from 4dee58e to f102622 Compare March 10, 2024 18:31

wihobbs force-pushed the docker-macos-script branch 2 times, most recently from 62f5f25 to 510ecfd Compare April 10, 2024 17:09

grondo reviewed Apr 12, 2024

View reviewed changes

wihobbs force-pushed the docker-macos-script branch 5 times, most recently from 20f61f8 to 35ce0e8 Compare April 12, 2024 23:19

wihobbs changed the title ~~scripts: include help message about docker-run-checks on macOS~~ docker: add script help message for macOS Apr 12, 2024

grondo reviewed Apr 13, 2024

View reviewed changes

wihobbs force-pushed the docker-macos-script branch from 35ce0e8 to e765692 Compare April 13, 2024 02:09

wihobbs force-pushed the docker-macos-script branch from e765692 to 3d19d99 Compare April 13, 2024 02:12

grondo approved these changes Apr 13, 2024

View reviewed changes

wihobbs added the merge-when-passing label Apr 13, 2024

mergify bot merged commit 64ae355 into flux-framework:master Apr 15, 2024
35 checks passed

wihobbs deleted the docker-macos-script branch April 15, 2024 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker: add script help message for macOS #5779

docker: add script help message for macOS #5779

wihobbs commented Mar 8, 2024

wihobbs commented Mar 8, 2024 •

edited

Loading

grondo Mar 8, 2024

grondo commented Mar 8, 2024

grondo commented Mar 8, 2024

wihobbs commented Mar 10, 2024

grondo commented Mar 11, 2024

grondo Apr 12, 2024

grondo Apr 12, 2024

wihobbs Apr 12, 2024

wihobbs Apr 12, 2024

wihobbs Apr 12, 2024

wihobbs Apr 12, 2024

wihobbs commented Apr 12, 2024 •

edited

Loading

grondo Apr 13, 2024

wihobbs Apr 13, 2024

grondo left a comment

codecov bot commented Apr 15, 2024

docker: add script help message for macOS #5779

docker: add script help message for macOS #5779

Conversation

wihobbs commented Mar 8, 2024

wihobbs commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

grondo commented Mar 8, 2024

grondo commented Mar 8, 2024

wihobbs commented Mar 10, 2024

grondo commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wihobbs commented Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grondo left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 15, 2024

Codecov Report

wihobbs commented Mar 8, 2024 •

edited

Loading

wihobbs commented Apr 12, 2024 •

edited

Loading