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

feat: make build scripts compatible with Bash strict mode #329

Merged
merged 7 commits into from
May 29, 2024

Conversation

KenDJohnson
Copy link
Contributor

@KenDJohnson KenDJohnson commented May 21, 2024

First off, this is an awesome project and exactly what I was looking for; thanks!

I made a change to allow argc to work with scripts written for "Bash strict mode" (link), specifically set -u, which will cause a script to exit with an error when referencing an undefined variable.

The only requirement for this is to change $maybe_undefined to ${maybe_undefined:-}, to provide a default value (empty string) when the variable is null/undefined.

Bumped the version to 1.18.0, all cargo test tests are passing locally.

Edit: it looks like there may be some more instances of this, I am tracking them down and converting this to a draft in the meantime. As an alternative approach, the argc generated functions could start by saving the state of nounset, disabling it with set +u, and then restoring it at the end of the function. Let me know if you'd prefer this approach and I can change the implementation.

@sigoden
Copy link
Owner

sigoden commented May 21, 2024

  1. Don't dump the version of argc. This has nothing to do with the PR. Normally we only upgrade versions when the next version is released.

  2. You can not simply add ${:-} to all variables. No one writes scripts like this. Can you specialize only variables that may be undefined?

@KenDJohnson KenDJohnson marked this pull request as draft May 21, 2024 14:30
@KenDJohnson
Copy link
Contributor Author

Don't dump the version of argc. This has nothing to do with the PR. Normally we only upgrade versions when the next version is released.

Reverting; sorry about that, habit from internal practices.

You can not simply add ${:-} to all variables. No one writes scripts like this. Can you specialize only variables that may be undefined?

This isn't adding ${:-} to all the variables, only the ones that may be legitimately undefined. For example, with env_name as FOO the [[ -n "${env_name}" ]] check would be written as [[ -n "$FOO" ]]. Up to this point, FOO is never assigned a value by the script, it is checking whether it was set externally. If the nounset option is enabled it has to be checked with [[ -n "${FOO:-} ]] to prevent the errors. Other variables (e.g. $_argc_index, $params, $arg_diff, etc.) are not potentially undefined and I did not add a default expansion.

It may be cleaner to save/restore the nounset state at argc generated function boundaries, which would eliminate the need for :- and doesn't risk missing instances that should have a default expansion with nounset.

@sigoden
Copy link
Owner

sigoden commented May 21, 2024

In principle, avoid adding ${:-} to variables unless necessary.

Variables requiring ${:-}, usually only apply to variables similar to $_argc_* and are defined in other functions.

[[ $_argc_dash -gt 0 ]] &&

For variables defined within the function, we should set default values during their initialization with local.

@KenDJohnson
Copy link
Contributor Author

I've pushed up some additional changes that should address everything here.

I think the minimum scenarios where ${:-} is required are:

  1. the $_argc_* possibly defined in other functions that you mentioned
  2. the ${!name} references where the referenced var may not exist
  3. indexing arrays such as argc__args, argc__positionals, where there may not be a value at that index
  4. the ${{{var_name}}} instances that are potentially set outside of the generated script

I've changed others like $concatenated_choices and $missed_envs to set a default in the local definition.

I also added -u to the test runner so that it runs all test scripts with the nounset option set. This required a couple of changes in the tests in tests/validate.rs, but is otherwise working with the src/build.rs changes noted above.

@KenDJohnson KenDJohnson marked this pull request as ready for review May 28, 2024 17:15
@sigoden sigoden changed the title Provide default expansion to work with Bash strict mode feat: make build scripts compatible with Bash strict mode May 29, 2024
@sigoden sigoden merged commit b073e36 into sigoden:main May 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants