Skip to content

CXX-2988 Use Bash and shebangs whenever able #1179

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

Merged
merged 9 commits into from
Jul 26, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jul 24, 2024

Resolves CXX-2988. Verified by this patch.

Companion PR to mongodb/mongo-c-driver#1680.

Additionally removes some stray env vars and a long-broken sed command in the uninstall_check tasks due to a combination of CXX-2753 (directory restructure) and CXX-2809 (removal of legacy CMake packages).

The etc/generate_uninstall.sh script is excluded from the shebang update to bash. It remains an sh script due to incompatibilities with how Bash escapes a sequence of backslashes \\\\ in strings. This results in incorrect contents of the generated uninstall.sh file. The following diffs are examples of incorrect changes produced by sh -> bash:

Missing escape backslashes:

- printf "Removing file \"include/bsoncxx/fwd.hpp\""
+ printf "Removing file "include/bsoncxx/fwd.hpp""

Failure to expand newlines prior to sort and iteration:

- printf "Removing directory \"share/mongo-cxx-driver\""
+ printf "Removing directory "include/bsoncxx\ninclude\n[...]"

Refactoring the etc/generate_uninstall.sh script to be Bash-compatible (or revisiting its method of generation entirely) is deferred to CXX-3073.

@eramongodb
Copy link
Contributor Author

Forgot to push updated commits prior to posting PR: rebased accordingly.

Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

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

LGTM, with a few minor tweaks (consistent spacing before opening parenthesis)


# Ensure generated uninstall script has executable permissions.
if (\"${CMAKE_VERSION}\" VERSION_GREATER_EQUAL \"3.19.0\")
file(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file(
file (

)
else ()
# Workaround lack of file(CHMOD).
file(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file(
file (

GROUP_READ GROUP_EXECUTE
WORLD_READ WORLD_EXECUTE
)
file(RENAME \"${UNINSTALL_PROG}.d/${UNINSTALL_PROG}\" \"${UNINSTALL_PROG}\")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
file(RENAME \"${UNINSTALL_PROG}.d/${UNINSTALL_PROG}\" \"${UNINSTALL_PROG}\")
file (RENAME \"${UNINSTALL_PROG}.d/${UNINSTALL_PROG}\" \"${UNINSTALL_PROG}\")

@eramongodb eramongodb merged commit 750c02d into mongodb:master Jul 26, 2024
1 check was pending
@eramongodb eramongodb deleted the cxx-2988 branch July 26, 2024 14:28
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.

3 participants