Explicitly pass cmake arguments to build. #3086
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors PostgreSQL version build configuration by moving CMake argument logic from bash script into Python, enabling explicit PostgreSQL version selection via command-line arguments.
Key Changes:
- Added
--pg-versionsflag tobuild_pg_ext.pyfor explicit PostgreSQL version control - Removed bash-based CMake flag logic from GitHub workflow in favor of Python argument passing
- Enhanced CLI argument parsing to support multiple flags (
--deeplake-shared/static,--pg-versions)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/build_pg_ext.py | Added --pg-versions argument parsing, updated run() function to accept and process PostgreSQL versions list, refactored argument parsing to handle multiple flags |
| .github/workflows/pg-extension-build.yaml | Simplified workflow by delegating PostgreSQL version logic to Python script via --pg-versions flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| deeplake_link_type = "static" | ||
| i += 1 | ||
| elif arg == "--pg-versions": | ||
| if i + 1 >= len(sys.argv): |
There was a problem hiding this comment.
The boundary check should use > instead of >= because valid indices go up to len(sys.argv) - 1. When i + 1 equals len(sys.argv), it's the first invalid index. The current condition would incorrectly allow access when i + 1 == len(sys.argv), causing an IndexError on line 239.
|
|
||
| if len(sys.argv) > 3: | ||
| raise Exception("Too many command line arguments") | ||
| raise Exception(f"Invalid option '{arg}'. Use --deeplake-shared, --deeplake-static, or --pg-versions") |
There was a problem hiding this comment.
The error message does not account for unknown arguments that might follow valid flags. Consider including information that flags requiring values (like --pg-versions) must be followed by their value, to help users diagnose issues when arguments are in the wrong order.
| raise Exception(f"Invalid option '{arg}'. Use --deeplake-shared, --deeplake-static, or --pg-versions") | |
| raise Exception( | |
| f"Invalid option '{arg}'. Use --deeplake-shared, --deeplake-static, or --pg-versions.\n" | |
| "Note: Flags like --pg-versions require a value (e.g., '16,17,18' or 'all') immediately after the flag.\n" | |
| "If you provided another flag after --pg-versions, it may have been interpreted as its value. Please check the order of your arguments." | |
| ) |
* initial * lint * add missing env * test * edit * test * debug * edits * edits * edits * edits * edits * edits * edits * edits * edits * debug * debug * Explicitly pass cmake arguments to build. (#3086) * edits * edits * edits * test * typo * test * Fixed merge issue. * Update .github/workflows/pg-extension-build.yaml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: vladbabayan05 <vlad@activeloop.dev> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context