-
Notifications
You must be signed in to change notification settings - Fork 29
fix: Fix build script bugs #137
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
… process (#137) Add `build_and_run.sh` convenience script for one-command environment setup and server startup, update README with quick start instructions, fix virtual environment path naming to use hyphens instead of equals signs, and reorder package installation to install deephaven-server before ibapi to improve dependency resolution.
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.
Pull request overview
This PR fixes several build script issues to improve the installation process. The changes include fixing virtual environment path naming to use hyphens instead of equals signs for better shell compatibility, reordering package installation to install deephaven-server before ibapi for improved dependency resolution, adding a convenience script for quick one-command setup, and correcting a pip command in the README documentation.
Key changes:
- Fixed venv path naming from
venv-release-dhib={version}tovenv-release-dhib-{version}for better shell compatibility - Reordered package installation to install deephaven-server before building/installing ibapi
- Added
build_and_run.shconvenience script for automated environment setup and server startup
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| dhib_env.py | Fixed virtual environment path naming to use hyphens instead of equals signs, and reordered package installation to install deephaven-server before ibapi in both dev() and release() functions |
| build_and_run.sh | New convenience script that automates the process of creating a temporary installer venv, building the release environment, and starting the Deephaven server |
| README.md | Added quick start documentation for the new convenience script, and fixed incorrect pip upgrade command from pip --upgrade pip to python -m pip install --upgrade pip |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Create temporary installer virtual environment | ||
| # This small venv is only used to run the dhib_env.py script | ||
| python3.12 -m venv .venv-installer |
Copilot
AI
Jan 8, 2026
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.
The script hardcodes python3.12 but doesn't verify that this version is available on the system. Consider adding a check to verify python3.12 exists, or use a more flexible approach like python3 with a version check, consistent with the README documentation which mentions python3.
build_and_run.sh
Outdated
| #!/bin/bash | ||
|
|
||
| # Convenience script to build a release virtual environment and start the Deephaven server. | ||
| # This script automates the process of: | ||
| # 1. Creating a temporary installer virtual environment | ||
| # 2. Building the release virtual environment with deephaven-ib, deephaven-server, and ibapi | ||
| # 3. Starting the Deephaven server | ||
| # | ||
| # Press Ctrl-C to stop the server when done. | ||
|
|
||
| # Display Java home (required for Deephaven) | ||
| echo $JAVA_HOME | ||
|
|
||
| # Clean up any existing virtual environments | ||
| deactivate 2>/dev/null || true # Deactivate if already in a venv | ||
| rm -rf .venv-installer | ||
| rm -rf venv-release-dhib* | ||
|
|
||
| # Create temporary installer virtual environment | ||
| # This small venv is only used to run the dhib_env.py script | ||
| python3.12 -m venv .venv-installer | ||
| source .venv-installer/bin/activate | ||
|
|
||
| # Install dependencies needed to run dhib_env.py | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements_dhib_env.txt | ||
|
|
||
| # Build the release virtual environment | ||
| # This creates venv-release-dhib-<version> with all required packages | ||
| python ./dhib_env.py release |
Copilot
AI
Jan 8, 2026
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.
The script lacks error handling and will continue executing even if critical steps fail (e.g., if creating the virtual environment fails, or if the dhib_env.py script fails). Consider adding set -e at the beginning of the script to exit on any command failure, or add explicit error checking after critical commands.
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.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- [x] Add `set -e` to the script to exit on any command failure - [x] Add `set -u` to catch undefined variables - [x] Add `set -o pipefail` to catch errors in piped commands - [ ] Run code review - [ ] Reply to the comment with the commit hash <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: chipkent <5250374+chipkent@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rm -rf .venv-installer | ||
|
|
||
| # Activate the release virtual environment and start Deephaven server | ||
| source ./venv-release-dhib*/bin/activate |
Copilot
AI
Jan 8, 2026
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.
Using a wildcard pattern in the source command could fail if multiple matching directories exist or if no matching directory exists. Consider capturing the venv path from the dhib_env.py output or failing explicitly if the expected venv directory doesn't exist. For example, you could check for the existence of the directory first or use a more robust path resolution.
| source ./venv-release-dhib*/bin/activate | |
| venv_activate_candidates=( ./venv-release-dhib*/bin/activate ) | |
| # Handle cases where the glob does not match or matches multiple environments | |
| if [ "${#venv_activate_candidates[@]}" -eq 1 ] && [ "${venv_activate_candidates[0]}" = "./venv-release-dhib*/bin/activate" ]; then | |
| echo "Error: No release virtual environment found matching ./venv-release-dhib*/bin/activate" >&2 | |
| exit 1 | |
| fi | |
| if [ "${#venv_activate_candidates[@]}" -eq 0 ]; then | |
| echo "Error: No release virtual environment found matching ./venv-release-dhib*/bin/activate" >&2 | |
| exit 1 | |
| elif [ "${#venv_activate_candidates[@]}" -gt 1 ]; then | |
| echo "Error: Multiple release virtual environments found. Please remove duplicates:" >&2 | |
| printf ' %s\n' "${venv_activate_candidates[@]}" >&2 | |
| exit 1 | |
| fi | |
| if [ ! -f "${venv_activate_candidates[0]}" ]; then | |
| echo "Error: Expected activate script not found at ${venv_activate_candidates[0]}" >&2 | |
| exit 1 | |
| fi | |
| source "${venv_activate_candidates[0]}" |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add convenience script for quick setup and improve installation process (#137)
Add
build_and_run.shconvenience script for one-command environment setup and server startup, update README with quick start instructions, fix virtual environment path naming to use hyphens instead of equals signs, and reorder package installation to install deephaven-server before ibapi to improve dependency resolution.