fix(lib_install): Disable file system translation in checkinstall during dependency installation (fixes #642).
#644
Conversation
…uring dependency installation.
WalkthroughThe pull request involves modifications to multiple library installation scripts in the Changes
Possibly Related PRs
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
components/core/tools/scripts/lib_install/mongocxx.sh (1)
88-90: The changes effectively address the filesystem translation issue.The addition of these checkinstall flags aligns well with the PR objectives:
--fstrans=noproperly disables filesystem translation--defaultimproves automation--nodocreduces package sizeConsider adding a comment to explain the purpose of these flags:
install_cmd_args+=( checkinstall + # Accept defaults, disable filesystem translation, and skip documentation --default --fstrans=no --nodoccomponents/core/tools/scripts/lib_install/fmtlib.sh (2)
78-87: Document the purpose of disabling filesystem translationWhile the implementation is correct, please add a comment explaining why
--fstrans=nois necessary. This will help future maintainers understand the rationale behind this specific configuration.Add a comment like:
install_cmd_args+=( checkinstall --default + # Disable filesystem translation to prevent path manipulation issues during installation --fstrans=no --nodoc
89-92: Add error handling for installation commandConsider adding error handling to provide better feedback if the installation fails.
Here's a suggested improvement:
install_cmd_args+=( make install ) -"${install_cmd_args[@]}" +if ! "${install_cmd_args[@]}"; then + echo "Error: Installation failed" + exit 1 +ficomponents/core/tools/scripts/lib_install/zstandard.sh (1)
89-92: Consider adding error handling for installation commandWhile the command construction is clean, consider adding error handling for the make install command. This could help diagnose installation failures more effectively.
install_cmd_args+=( make install ) -"${install_cmd_args[@]}" +if ! "${install_cmd_args[@]}"; then + echo "Error: Installation failed" + exit 1 +ficomponents/core/tools/scripts/lib_install/msgpack.sh (2)
81-86: Consider adding error handling for failed buildsWhile the command construction is correct, consider adding error handling to capture and report build failures more gracefully.
Add error handling like this:
-"${install_cmd_args[@]}" +if ! "${install_cmd_args[@]}"; then + echo "Error: Installation failed" >&2 + exit 1 +fi
89-89: Consider adding trap-based cleanupTo ensure cleanup occurs even if the script fails, consider adding a trap-based cleanup mechanism.
Add this near the start of the script:
+cleanup() { + rm -rf "$temp_dir" +} +trap cleanup EXITcomponents/core/tools/scripts/lib_install/mariadb-connector-c.sh (1)
93-98: Add comment explaining the rsync operationThe command construction is correct, but it would be helpful to document why rsync is used here instead of a simple copy operation.
Add a comment before the command:
+# Use rsync to preserve file attributes and perform an atomic copy install_cmd_args+=( rsync -a . "${install_dir}/" )components/core/tools/scripts/lib_install/spdlog.sh (2)
85-94: Consider enhancing package metadata handlingWhile the checkinstall configuration is good, consider these improvements:
- Add version format validation before using it in --pkgversion
- Consider adding alternative package names in --provides for better compatibility
Example validation:
+ if ! [[ "${version}" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + echo "Error: Version must be in format X.Y.Z" + exit 1 + fi install_cmd_args+=( checkinstall --default --fstrans=no --nodoc --pkgname "${package_name}" --pkgversion "${version}" - --provides "${package_name}" + --provides "${package_name}, libspdlog" --pakdir "${deb_output_dir}" )
96-99: Add error handling for the installation commandWhile the command execution is correct, consider adding error handling to catch and report installation failures more gracefully.
+ set +e "${install_cmd_args[@]}" + install_status=$? + set -e + if [ $install_status -ne 0 ]; then + echo "Error: Installation failed with status $install_status" + exit $install_status + ficomponents/core/tools/scripts/lib_install/lz4.sh (1)
86-89: Consider adding error handling for installation commandWhile the array expansion is implemented correctly, consider adding error handling to capture and report installation failures more gracefully.
+set +e "${install_cmd_args[@]}" +install_status=$? +set -e +if [ $install_status -ne 0 ]; then + echo "Installation failed with status $install_status" + exit $install_status +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/core/tools/scripts/lib_install/fmtlib.sh(2 hunks)components/core/tools/scripts/lib_install/libarchive.sh(2 hunks)components/core/tools/scripts/lib_install/lz4.sh(2 hunks)components/core/tools/scripts/lib_install/mariadb-connector-c.sh(2 hunks)components/core/tools/scripts/lib_install/mongocxx.sh(1 hunks)components/core/tools/scripts/lib_install/msgpack.sh(2 hunks)components/core/tools/scripts/lib_install/spdlog.sh(2 hunks)components/core/tools/scripts/lib_install/zstandard.sh(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
components/core/tools/scripts/lib_install/mariadb-connector-c.sh
[warning] 44-44: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🔇 Additional comments (15)
components/core/tools/scripts/lib_install/mongocxx.sh (1)
88-90: Verify the impact of disabling filesystem translation.
Let's ensure that disabling filesystem translation doesn't cause issues with package installation paths.
✅ Verification successful
Disabling filesystem translation is a consistent pattern across library installations
The --fstrans=no flag is consistently used across multiple library installation scripts in the codebase, including spdlog, zstandard, mariadb-connector-c, lz4, msgpack, libarchive, fmtlib, and mongocxx. This standardized approach suggests it's a deliberate design choice for the build system. No installation path issues related to this flag were found in the logs or codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing filesystem translation issues or related bugs
# in the MongoDB C++ driver installation
# Search for related issues in the codebase
rg -i "fstrans|filesystem.translation"
# Check if other packages use filesystem translation
fd -t f . components/core/tools/scripts/lib_install/ -x grep -l "fstrans"
# Look for any installation path related issues
rg -i "installation.path|install.prefix|prefix" components/core/tools/scripts/lib_install/
Length of output: 1784
components/core/tools/scripts/lib_install/fmtlib.sh (1)
41-44: LGTM: Improved command construction using array
The refactoring to use an array for command construction is a good practice. It's more maintainable and safer than string concatenation, especially when dealing with arguments that might contain spaces.
components/core/tools/scripts/lib_install/libarchive.sh (3)
40-43: Excellent improvement in privilege handling!
The transition to using an array for command construction is a robust improvement over string concatenation. The privilege check is properly implemented with a fallback mechanism.
89-92: Well-structured command execution!
The array-based command construction and execution using proper expansion syntax "${install_cmd_args[@]}" is a robust approach that correctly handles arguments with spaces.
78-87: Verify documentation requirements and filesystem translation impact
The addition of --fstrans=no aligns with the PR objective to disable filesystem translation. However, two points need verification:
- The
--nodocflag skips documentation installation. Please verify this complies with your project's packaging guidelines. - Ensure disabling filesystem translation doesn't affect package installation on systems with non-standard filesystem layouts.
Run this verification script:
✅ Verification successful
Checkinstall configuration is consistent across the codebase
The --nodoc and --fstrans=no flags are used consistently across all library installation scripts in the codebase, including zstandard, spdlog, msgpack, mongocxx, lz4, fmtlib, mariadb-connector-c, and libarchive. This demonstrates a standardized approach to package creation where:
- Documentation is consistently omitted across all packages
- Filesystem translation is disabled uniformly
- The same checkinstall configuration pattern is followed project-wide
Since this is an established pattern across the entire codebase, both flags align with the project's packaging standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any other packages in the repository use different checkinstall flags
# or handle documentation differently
rg -g '*.sh' 'checkinstall.*--nodoc' .
rg -g '*.sh' 'checkinstall.*--fstrans' .
Length of output: 83
Script:
#!/bin/bash
# Let's try a broader search for checkinstall usage patterns
rg -g '*.sh' 'checkinstall' -B 2 -A 5 .
# Also search for any package installation or documentation related patterns
rg -g '*.sh' 'install.*doc' -B 2 -A 2 .
Length of output: 14212
components/core/tools/scripts/lib_install/zstandard.sh (2)
41-44: Excellent improvement to command handling!
The transition from string-based command prefix to array-based command construction is a robust improvement. The privilege check is well-implemented with proper fallback to id -u.
78-87: Verify the impact of disabling filesystem translation
The addition of --fstrans=no to checkinstall addresses the core issue. The command construction is well-organized using array append operations.
Let's verify if this change is consistent across other library installation scripts:
✅ Verification successful
The --fstrans=no flag is consistently implemented across all installation scripts
The verification shows that all library installation scripts that use checkinstall have been updated with the --fstrans=no flag, including:
- fmtlib.sh
- libarchive.sh
- lz4.sh
- mariadb-connector-c.sh
- mongocxx.sh
- msgpack.sh
- spdlog.sh
- zstandard.sh
The command construction is identical across all scripts, ensuring consistent behaviour.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other installation scripts also disable filesystem translation
rg --type bash "checkinstall.*--fstrans" components/core/tools/scripts/lib_install/
Length of output: 114
Script:
#!/bin/bash
# Search for checkinstall usage in installation scripts
rg "checkinstall" components/core/tools/scripts/lib_install/
Length of output: 3626
Script:
#!/bin/bash
# Let's examine the checkinstall command construction in other library installation scripts
for file in components/core/tools/scripts/lib_install/*.sh; do
echo "=== $file ==="
rg -A 10 "install_cmd_args\+=" "$file" || true
done
Length of output: 8080
components/core/tools/scripts/lib_install/msgpack.sh (2)
39-42: LGTM: Improved command construction approach
The switch to an array-based command construction is a more robust approach, properly handling privilege elevation while maintaining security checks.
70-79: Verify the impact of disabling filesystem translation
The addition of --fstrans=no aligns with the PR objective. The array-based command construction improves readability and maintainability.
Let's verify this change doesn't affect other dependent packages:
✅ Verification successful
The --fstrans=no flag is consistently used across all installation scripts
The verification shows that all library installation scripts in the codebase use the same --fstrans=no flag with checkinstall. This consistent usage across fmtlib, spdlog, msgpack, mongocxx, mariadb-connector-c, libarchive, lz4, and zstandard installations indicates this is an established pattern and the change aligns with the existing codebase standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing packages that might be affected by --fstrans=no
# Search for other checkinstall usage in installation scripts
rg -l 'checkinstall' --type sh
# Look for any existing .deb packages that might have been created with different fstrans settings
fd -e deb . | xargs -I {} dpkg -I {} | grep -B2 'msgpack'
Length of output: 886
Script:
#!/bin/bash
# Let's check the usage of checkinstall in other installation scripts to see if they handle fstrans differently
# Check the content of other installation scripts that use checkinstall
for file in $(rg -l 'checkinstall' --type sh); do
echo "=== $file ==="
rg -A 5 'checkinstall' "$file"
done
Length of output: 6342
components/core/tools/scripts/lib_install/mariadb-connector-c.sh (1)
82-91: LGTM! Verify documentation for --fstrans flag
The addition of --fstrans=no aligns with the PR objective to disable filesystem translation. The command construction is clean and well-structured.
Let's verify if this flag is documented:
components/core/tools/scripts/lib_install/spdlog.sh (2)
48-51: LGTM: Robust privilege checking and command array initialization
The implementation uses a clean array-based approach for command construction and includes a reliable privilege check with proper fallback.
88-88: Verify consistent --fstrans=no usage across installation scripts
Let's ensure this change is applied consistently across all library installation scripts.
✅ Verification successful
Let me check the actual usage of checkinstall in these scripts to verify if they need the --fstrans=no flag.
The --fstrans=no flag is already consistently used across all installation scripts
The verification shows that all library installation scripts that use checkinstall already include the --fstrans=no flag:
- zstandard.sh
- mongocxx.sh
- mariadb-connector-c.sh
- lz4.sh
- libarchive.sh
- fmtlib.sh
- msgpack.sh
- spdlog.sh
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other installation scripts need similar changes
echo "Checking other installation scripts for checkinstall usage..."
rg -l 'checkinstall' components/core/tools/scripts/lib_install/
Length of output: 800
Script:
#!/bin/bash
# Check checkinstall command usage in installation scripts
echo "Checking checkinstall command usage in installation scripts..."
rg -B2 -A2 'checkinstall' --type sh components/core/tools/scripts/lib_install/ | grep -v "ubuntu-.*install-prebuilt-packages.sh"
Length of output: 8450
components/core/tools/scripts/lib_install/lz4.sh (3)
41-44: Excellent improvement to privilege handling!
The switch from string concatenation to an array-based approach for command construction is a solid improvement. This makes the code more maintainable and less prone to shell injection issues.
75-84: Verify impact of checkinstall flags
The addition of --fstrans=no aligns with the PR objective. However, let's verify the implications of using --default with these other flags.
✅ Verification successful
Checkinstall configuration is consistent across the codebase
The checkinstall flags used in lz4.sh (--default, --fstrans=no, --nodoc) are consistently used across all installation scripts in the codebase, including zstandard, mariadb-connector-c, libarchive, mongocxx, spdlog, fmtlib, and msgpack. This standardized approach confirms that the configuration is well-established and safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any other installation scripts in the codebase use different checkinstall flags
# that might conflict with or complement our configuration
# Search for checkinstall usage patterns
rg --type bash 'checkinstall.*--' components/core/tools/scripts/lib_install/
Length of output: 107
Script:
#!/bin/bash
# Let's try without specifying file type since rg doesn't recognize 'bash'
rg 'checkinstall.*--' components/core/tools/scripts/lib_install/
# Also search for any other shell scripts that might contain checkinstall configurations
fd . components/core/tools/scripts/lib_install/ -e sh -x grep -l "checkinstall"
Length of output: 750
Script:
#!/bin/bash
# Let's examine the checkinstall configurations in these files
for file in zstandard.sh mariadb-connector-c.sh libarchive.sh mongocxx.sh spdlog.sh fmtlib.sh msgpack.sh; do
echo "=== $file ==="
grep -A 5 -B 5 "checkinstall" "components/core/tools/scripts/lib_install/$file"
done
Length of output: 4456
Line range hint 1-89: Verify security considerations
The script performs several sensitive operations. Let's verify:
- The integrity of downloaded content
- The safety of make commands
- The implications of disabling filesystem translation
| install_cmd_args=() | ||
| if [ ${EUID:-$(id -u)} -ne 0 ] ; then | ||
| sudo echo "Script can elevate privileges." | ||
| privileged_command_prefix="${privileged_command_prefix} sudo" | ||
| privileged_command_prefix="sudo" | ||
| install_cmd_args+=(${privileged_command_prefix}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor privilege handling for consistency
The privilege handling needs improvement:
- The script still uses
privileged_command_prefixvariable in multiple places (e.g., lines 78 and 102) after switching toinstall_cmd_args - There's a potential word splitting vulnerability when adding sudo to the array
Apply these changes:
install_cmd_args=()
if [ ${EUID:-$(id -u)} -ne 0 ] ; then
sudo echo "Script can elevate privileges."
- privileged_command_prefix="sudo"
- install_cmd_args+=(${privileged_command_prefix})
+ install_cmd_args+=("sudo")Then replace remaining ${privileged_command_prefix} usages with "${install_cmd_args[@]}".
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 44-44: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
Description
checkinstallduring dependency installation.Validation performed
Summary by CodeRabbit
New Features
checkinstallwith structured command arguments across multiple scripts.Bug Fixes
Refactor
privileged_command_prefixwithinstall_cmd_argsarray in multiple scripts for better command management.