Skip to content

Conversation

@DSMitten
Copy link
Contributor

@DSMitten DSMitten commented Apr 4, 2025

The issue prompting this PR is #1341, where a positional argument following a switch will cause the switch to be swallowed and not evaluated.

This PR changes how commands are parsed so that:

  • All positional arguments are parsed before any switches, in a loop
  • Any number of positional arguments will be evaluated
  • If there is a conflict (such as 'release' and 'debug', or 'arm64' and 'x86_64'), the error will be detected and the script will terminate
  • All parsing occurs before any work (mainly affects the clean step)
  • If any arguments follow the final switch, the error will be detected and the script will terminate

Additionally, there was a conflict between the usage statement and logic, in that the usage statement referred to a positional argument CUSTOM_CMAKE_CXX_FLAGS=x, but the actual parser looked for an argument CUSTOM_BUILD_FLAGS=x and set a variable CUSTOM_CMAKE_CXX_FLAG. The usage statement is updated to reflect the argument.

Testing performed includes:

  • All arguments and the variables they set
  • Conflicting arguments result in errors
  • Default values are used if no value is set for BUILD_TYPE or MAC_ARCH
  • Non-switch arguments following switches result in errors
  • ./build.sh noroot -DBUILD_UNIT_TESTS=NO -DBUILD_FUNC_TESTS=NO builds successfully

@DSMitten DSMitten requested a review from a team as a code owner April 4, 2025 03:05
Copy link
Contributor

@ThomsonTan ThomsonTan left a comment

Choose a reason for hiding this comment

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

:shipit:

@lalitb
Copy link
Contributor

lalitb commented Apr 4, 2025

@DSMitten - while looks unlikely, can you ensure that this doesn't break the existing scripts which internally invoke build.sh . As here -

docker run -v `pwd`:/build -w /build $IMAGE_NAME ./build.sh $BUILD_TYPE

@ThomsonTan ThomsonTan merged commit 4132adc into main Apr 7, 2025
24 of 28 checks passed
@ThomsonTan ThomsonTan deleted the u/smitten/buildsh branch April 7, 2025 17:08
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.

4 participants