Update bin path and other install fixes
#119
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The bin path for virtual env is now `../env/local/bin` Here is the relavant change: https://github.com/pypa/virtualenv/blob/50b4b6b72a0709501b36f714e317b07c3d765833/tests/unit/discovery/py_info/test_py_info.py#L331
Using `exec` to import the version is anti-pattern, unpythonic, and fragile. What's more, it does not work, because the Python interpreter is looking in the CWD for the `bashhub/version.py` whereas it actually needs to look in the script location. The script location is found using the ugly method below: ```python3 import os import sys exec (open(os.path.join(os.path.dirname(sys.argv[0]),'./bashhub/version.py')).read()) ```
|
@mavaddat thanks for the PR! Is the installer itself actually broken? Or is this PR about updating to always use the latest virtualenv and updating the appropriate paths for that? The implementation was using a specific version intentionally and I haven't heard any other reports of the installer being broken. The change to setup.py makes sense and LGTM. |
rcaloras
reviewed
Feb 25, 2023
| @@ -102,11 +102,9 @@ download_and_install_env() { | |||
| PYTHON=$(which $python_command) | |||
| echo "Using Python path $PYTHON" | |||
|
|
|||
| VERSION=20.10.0 | |||
| VERSION_URL="https://github.com/pypa/get-virtualenv/raw/$VERSION/public/virtualenv.pyz" | |||
There was a problem hiding this comment.
Using this version and the specific version url is intentional. Is the update needed to support newer versions of Python?
|
@rcaloras , yes the installer itself is broken. Actually, this PR doesn't solve all the issues, which I'm still trying to diagnose and fix. I will update this PR later today or tomorrow. Please standby! |
|
Can you share the stack trace or log as well as any system information it’s
failing on? Or how to reproduce? I’m unable to reproduce the issue on my
Linux box or mac. Assuming maybe python versions/installs may be an issue.
…On Sat, Feb 25, 2023 at 1:17 PM Mavaddat Javid ***@***.***> wrote:
@rcaloras <https://github.com/rcaloras> , yes the installer itself is
broken. Actually, this PR doesn't solve all the issues, which I'm still
trying to diagnose and fix. I will update this PR later today or tomorrow.
Please standby!
—
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABESOVVC6VXDSSD2G7LSLLWZJSD5ANCNFSM6AAAAAAVG6WPPU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces multiple fixups for the broken installer.
Binaries path
The bin path for virtual env is now
../env/local/bin.Here is one line in virtualenv tests that confirm this change:
/tests/unit/discovery/py_info/test_py_info.py#L331Robustify virtualenv installer
The recommended zipapp way to install the latest virtualenv is given here.
Fix
__version__injectionUsing
execto import the version is anti-pattern, unpythonic, and fragile. What's more, it does not work, because the Python interpreter is looking in the CWD for thebashhub/version.pywhereas it actually needs to look in the script location. The script location is found using the ugly method below: