-
Notifications
You must be signed in to change notification settings - Fork 455
CDRIVER-5509 Use Bash and shebangs whenever able #1680
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
Conversation
Fixed the install-uninstall-check task by ensuring the uninstall.sh script is always executable even prior to installation. Verified by this patch. |
There was a problem hiding this 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 spacing tweaks.
@@ -72,24 +70,46 @@ function(append_line line) | |||
file(APPEND "${UNINSTALL_WRITE_FILE}" "${line}\n") | |||
endfunction() | |||
|
|||
# Ensure generated uninstall script has executable permissions. | |||
if ("${CMAKE_VERSION}" VERSION_GREATER_EQUAL "3.19.0") | |||
file( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file( | |
file ( |
else () | ||
# Workaround lack of file(CHMOD). | ||
get_filename_component(_script_filename "${UNINSTALL_WRITE_FILE}" NAME) | ||
file( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file( | |
file ( |
GROUP_READ GROUP_EXECUTE | ||
WORLD_READ WORLD_EXECUTE | ||
) | ||
file(RENAME "${_script_filename}.d/${_script_filename}" "${_script_filename}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file(RENAME "${_script_filename}.d/${_script_filename}" "${_script_filename}") | |
file (RENAME "${_script_filename}.d/${_script_filename}" "${_script_filename}") |
Additionally removed the |
Summary
Addresses CDRIVER-5509. Verified by this patch.
For the most part, our scripts all use Bash already following prior on EVG config refactoring and script hygiene improvements such as in #1193 and #1187.
This PR continues the prior work to ensure consistent use of shebangs and script invocation in our EVG, build, and utility scripts.
Shebangs
This PR applies
#!/usr/bin/env bash
to all our shebangs for consistency with the pattern being used by DET scripts: mongodb-labs/drivers-evergreen-tools#458Scripts related to Debian packaging are left unmodified due to the special environment requirements. These may also be updated for consistency if it turns out not to be an issue.
Script Invocation
This PR adds the executable permission bit to all shell scripts and invokes them directly (e.g.
./script.sh
instead ofbash script.sh
) to respect shebangs. Otherwise our invocation risks overriding the preferred shell for which the script is intended to be used.License Notice in Uninstall Script
#1655 overlooked some minor formatting issues with the license notice embedded in the generated uninstall script.
This PR tidies it up a bit to look as follows:
Similarly for the Windows CMD script, before:
and after: