Skip to content
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

Logical checks should be in if blocks #3307

Open
DavidHuber-NOAA opened this issue Feb 6, 2025 · 2 comments · May be fixed by #3339
Open

Logical checks should be in if blocks #3307

DavidHuber-NOAA opened this issue Feb 6, 2025 · 2 comments · May be fixed by #3339
Assignees
Labels
bug Something isn't working

Comments

@DavidHuber-NOAA
Copy link
Contributor

DavidHuber-NOAA commented Feb 6, 2025

What is wrong?

There are several places in the env/* files and possibly elsewhere that have half-logical statements. For example,

[[ ${NTHREADS_GEMPAK} -gt ${max_threads_per_task} ]] && export NTHREADS_GEMPAK=${max_threads_per_task}

If the logical returns False, then this will cause a failure if set -e is enabled as it return a non-zero status.

What should have happened?

These logical checks should be replaced with if blocks or have || true appended to them.

What machines are impacted?

All or N/A

What global-workflow hash are you using?

3d6c13e

Steps to reproduce

Run a gempak job on Hera.

Additional information

Reported by @AntonMFernando-NOAA.

Do you have a proposed solution?

For the case above, this line should be either

[[ ${NTHREADS_GEMPAK} -gt ${max_threads_per_task} ]] && export NTHREADS_GEMPAK=${max_threads_per_task} || true

expanded to

   if  [[ ${NTHREADS_GEMPAK} -gt ${max_threads_per_task} ]]; then
      export NTHREADS_GEMPAK=${max_threads_per_task}
   fi

All such examples should be addressed in the code.

@DavidHuber-NOAA DavidHuber-NOAA added bug Something isn't working triage Issues that are triage labels Feb 6, 2025
@WalterKolczynski-NOAA
Copy link
Contributor

Personally, I'd prefer removing all the short-circuits and only use if/else, even though it is more verbose. See also https://mywiki.wooledge.org/BashPitfalls#cmd1_.26.26_cmd2_.7C.7C_cmd3

@WalterKolczynski-NOAA WalterKolczynski-NOAA removed the triage Issues that are triage label Feb 6, 2025
@DavidHuber-NOAA
Copy link
Contributor Author

Updated the description and title to reflect that.

@DavidHuber-NOAA DavidHuber-NOAA changed the title Logical checks should be in if blocks or appended with || true Logical checks should be in if blocks Feb 6, 2025
@DavidHuber-NOAA DavidHuber-NOAA self-assigned this Feb 11, 2025
@DavidHuber-NOAA DavidHuber-NOAA linked a pull request Feb 20, 2025 that will close this issue
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants