chore(wsl1): Fix consistent FDB deps install LS-379#769
Conversation
Previously, the installation of FoundationDB was skipped on WSL1, which led to the need of manually installing packages dependencies for project compilation on WSL1, due to no complete header files being present. This commit fixes that issue by ensuring only FDB related libraries and header files are available on WSL1. Signed-off-by: rolysr <rolysr@leil.io>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where FoundationDB dependencies were not being consistently installed on WSL1 environments. By modifying the installation scripts, this PR ensures that the necessary libraries and header files are available, resolving a manual installation requirement for project compilation on WSL1. Highlights
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR aims to fix WSL1 developer/CI setup by ensuring FoundationDB-related dependencies are installed consistently so the project can compile without manual package installation on WSL1.
Changes:
- Always invoke the FoundationDB install script from
tests/setup_machine.sh(removing the WSL1 skip there). - Move WSL1 handling into
tests/ci_build/install-foundationdb.shby adding a WSL1 detection branch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/setup_machine.sh |
Always runs the FoundationDB install helper during machine setup. |
tests/ci_build/install-foundationdb.sh |
Adds WSL1 detection around the dpkg/apt install/verify steps. |
| if grep -q Microsoft /proc/version && ! grep -q microsoft-standard /proc/version; then | ||
| echo "Running on WSL1: skipping FoundationDB installation." | ||
| else | ||
| echo "FoundationDB CLI installation failed." | ||
| exit 1 | ||
| fi | ||
| echo "Installing FoundationDB packages..." | ||
| dpkg -i "${workspace}/${client_pkg}" "${workspace}/${server_pkg}" || die "Failed to install FoundationDB packages" |
There was a problem hiding this comment.
The new WSL1 branch skips all FoundationDB installation, which means the FoundationDB headers/libs still won’t be present on WSL1 (the original compilation problem remains). If WSL1 can’t run the server, consider installing only the client package (or extracting just headers/libs) rather than skipping everything, and avoid downloading the server .deb in that case.
| if grep -q Microsoft /proc/version && ! grep -q microsoft-standard /proc/version; then | ||
| echo "Running on WSL1: skipping FoundationDB installation." | ||
| else |
There was a problem hiding this comment.
In the WSL1 branch you skip installation but still leave the downloaded files in the temporary workspace (cleanup only runs in the non-WSL1 branch or via die()). Add cleanup for the WSL1 path (or use a trap to always remove the workspace), and consider skipping downloads when you’re not installing anything.
| echo ; echo 'Install FoundationDB' | ||
| if grep -q Microsoft /proc/version && ! grep -q microsoft-standard /proc/version; then | ||
| echo "Running on WSL1: skipping FoundationDB installation." | ||
| else | ||
| "$script_dir/ci_build/install-foundationdb.sh" | ||
| fi | ||
| "$script_dir/ci_build/install-foundationdb.sh" |
There was a problem hiding this comment.
setup_machine.sh now always runs install-foundationdb.sh, but that script currently still skips installation on WSL1. If the goal is to ensure WSL1 has FoundationDB headers/libs for compilation, this change won’t achieve it unless install-foundationdb.sh installs (at least) the client package on WSL1.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix the FoundationDB dependency installation on WSL1. However, the current implementation still skips the installation on WSL1, which contradicts the PR's description. Additionally, the refactoring introduces an issue where the temporary workspace is not cleaned up when running on WSL1. I've added a specific comment with a suggestion to address these points.
| if grep -q Microsoft /proc/version && ! grep -q microsoft-standard /proc/version; then | ||
| echo "Running on WSL1: skipping FoundationDB installation." | ||
| else | ||
| echo "FoundationDB CLI installation failed." | ||
| exit 1 | ||
| fi | ||
| echo "Installing FoundationDB packages..." | ||
| dpkg -i "${workspace}/${client_pkg}" "${workspace}/${server_pkg}" || die "Failed to install FoundationDB packages" | ||
|
|
||
| # Fix any missing dependencies | ||
| echo "Fixing missing dependencies..." | ||
| apt-get install -f -y || die "Failed to fix missing dependencies" | ||
|
|
||
| # Verify the installation | ||
| echo "Verifying FoundationDB installation..." | ||
| if command -v fdbcli &> /dev/null; then | ||
| echo "FoundationDB CLI installed successfully." | ||
| fdbcli --version | ||
| else | ||
| echo "FoundationDB CLI installation failed." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Clean up temporary workspace | ||
| echo "Removing temporary workspace..." | ||
| rm -r "${workspace:?}" | ||
| # Clean up temporary workspace | ||
| echo "Removing temporary workspace..." | ||
| rm -r "${workspace:?}" | ||
|
|
||
| echo "FoundationDB installation completed successfully." | ||
| echo "FoundationDB installation completed successfully." | ||
| fi |
There was a problem hiding this comment.
There are two issues with this block of code:
- Contradiction with PR description: The code skips the FoundationDB installation on WSL1, but the pull request description states that it should install the FDB libraries and header files.
- Resource leak: The temporary workspace (
${workspace}) is only cleaned up in theelsebranch. This means on WSL1, the workspace directory and its contents are never removed.
I suggest refactoring this section to correctly handle the WSL1 case as described, and to move the common steps like dependency fixing, verification, and cleanup outside the conditional block to avoid code duplication and fix the resource leak.
if grep -q Microsoft /proc/version && ! grep -q microsoft-standard /proc/version; then
echo "Running on WSL1: installing FoundationDB client package..."
dpkg -i "${workspace}/${client_pkg}" || die "Failed to install FoundationDB client package"
else
echo "Installing FoundationDB packages..."
dpkg -i "${workspace}/${client_pkg}" "${workspace}/${server_pkg}" || die "Failed to install FoundationDB packages"
fi
# Fix any missing dependencies
echo "Fixing missing dependencies..."
apt-get install -f -y || die "Failed to fix missing dependencies"
# Verify the installation
echo "Verifying FoundationDB installation..."
if command -v fdbcli &> /dev/null; then
echo "FoundationDB CLI installed successfully."
fdbcli --version
else
echo "FoundationDB CLI installation failed."
exit 1
fi
# Clean up temporary workspace
echo "Removing temporary workspace..."
rm -r "${workspace:?}"
echo "FoundationDB installation completed successfully."
rolysr
changed the title
Previously, the installation of FoundationDB was skipped on WSL1, which led to the need of manually installing packages dependencies for project compilation on WSL1, due to no complete header files being present. This commit fixes that issue by ensuring only FDB related libraries and header files are available on WSL1.