-
Notifications
You must be signed in to change notification settings - Fork 15
1398 pin python version #1405
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
1398 pin python version #1405
Conversation
…nterpreter with version number attached
Reviewer's GuideThis pull request pins the Python version used across various shell scripts by replacing hardcoded 'python3' commands and package names with the variable '"python${DATAFED_PYTHON_VERSION}"' to ensure consistent dependency installation and script execution. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @JoshuaSBrown - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
Possible typo in Dockerfile package list. (link)
-
Consider clarifying why
python3
is removed from theweb/docker/Dockerfile
without explicitly addingpython${DATAFED_PYTHON_VERSION}
as done in other dependency scripts. -
The
init_python
function now upgrades pip usingensurepip --upgrade
; the subsequentpip install --upgrade pip
calls in various install scripts appear redundant.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 1 other issue
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
LD_LIBRARY_PATH="$LD_LIBRARY_PATH" PATH="$PATH" python3 -m pip install numpy | ||
LD_LIBRARY_PATH="$LD_LIBRARY_PATH" PATH="$PATH" python3 setup.py build | ||
LD_LIBRARY_PATH="$LD_LIBRARY_PATH" PATH="$PATH" python3 setup.py test | ||
LD_LIBRARY_PATH="$LD_LIBRARY_PATH" PATH="$PATH" python${DATAFED_PYTHON_VERSION} -m pip install numpy |
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.
suggestion: Consider consistent quoting of the python command.
Some invocations are missing quotes around python${DATAFED_PYTHON_VERSION}. Quote them all to avoid potential word splitting issues.
Suggested implementation:
LD_LIBRARY_PATH="$LD_LIBRARY_PATH" PATH="$PATH" "python${DATAFED_PYTHON_VERSION}" -m pip install numpy
LD_LIBRARY_PATH="$LD_LIBRARY_PATH" PATH="$PATH" "python${DATAFED_PYTHON_VERSION}" setup.py build
LD_LIBRARY_PATH="$LD_LIBRARY_PATH" PATH="$PATH" "python${DATAFED_PYTHON_VERSION}" setup.py test
* updated requirements * updated requirements * updated protobuf submodule * update unit test ci version * updated protobuf version * updated setuptools version * fixed condition * changed where packages are installed * used python3.11 to create the virtual environment instead of latest * reverted protobuf version * testing specificying version * removed pyvenv in ci job for consistency * installed python3.11 in dependencies docker file * added missing -y flag * removed unification of install scripts * added missing python3.11 install * updated ci job to install python3.11 * changed installed python version to 3.9 since protobuf does not officially support 3.11 * changed installed python version to 3.9 since protobuf does not officially support 3.11 * missed version * updated includes for newer gcc versions * updated versions * updated nlohmann json * updated json schema version * changed library search path * hopefully fixed json dep issues * updating function signature to match newer version of base class * updated protobuf * updated protobuf and cmake * downgraded cmake * fixed protobuf version * fixed protobuf version * 1398 pin python version (#1405) * Pin python version * fixed incorrect path * Switch to using .tar file install of libsodium (#1414) * 1413 libsodium build refactor (#1415) * Switch to using .tar file install of libsodium * Switch bad option in wget command from -C to -P * Update scripts/dependency_install_functions.sh * Update scripts/dependency_install_functions.sh * Update dependency_install_functions.sh Libsodium folder version number. * Update dependency_install_functions.sh Make paths explicit. * cleaned up comments * Update scripts/dependency_install_functions.sh Co-authored-by: Joshua S Brown <[email protected]> * Update cmake/JSONSchema.cmake Co-authored-by: Joshua S Brown <[email protected]> * Update scripts/dependency_install_functions.sh Co-authored-by: Joshua S Brown <[email protected]> * Update cmake/JSONSchema.cmake Co-authored-by: Joshua S Brown <[email protected]> * add changes from review * Changed install scripts to be consistent with new python installation method (#1417) * changed install scripts to be consistent * added sudo check * add apt sources check --------- Co-authored-by: Blake Nedved <[email protected]> Co-authored-by: nedvedba <[email protected]>
PR Description
Pin the python version to avoid problems with compatibility.
Tasks
Summary by Sourcery
Pin the Python version across installation and dependency scripts to improve compatibility and consistency
Enhancements:
Chores: