Fix _get_virtualenv_hash function #2883
Conversation
|
Hi @uranusjr, |
|
@kennethreitz, @uranusjr, @techalchemy |
|
Hello @Jamim -- of course we care about the library we maintain, but I would suggest that the phrasing is probably not the most effective way to have a positive interaction. I appreciate your contribution, however, the pr template is there to suggest that you should open and report an issue if there is not yet one on the topic, which is how we typically triage problems. This helps to avoid contributing a fix before it has been determined that it is necessary. This way, we can assess that the fix indeed is desired before you put effort into attempting it. Before we merge any PR related to this, I would need to see a failing test and an issue explaining the problem. Thanks for your efforts |
|
Thank you for the feedback, @techalchemy! I understand that you have some workflow that you prefer to follow, but meanwhile I believe there is too much effort required for a single-line fix. |
|
I still need to know exactly what is broken, preferably via a test case (prevents regression) but if you don't feel like it at least I need to see an example of what you're saying doesn't work properly, because it's hard to decipher that just from reading over the text provided I.e. what do I need to type in to have a problem? |
I found that since I submitted this PR the problem was "fixed" by #2877, but I believe it doesn't work properly. So, you need to revert one commit to see what exactly goes wrong. git checkout master
git pull origin master
git checkout -b tmp
git revert 65ccd765b6d0b78e602d4b233ca8e31e1a6b8adb
docker rm pipenv_pipenv-tests_1 # you must delete cached image
docker-compose upI expect that you'll get something like this: # docker rm pipenv_pipenv-tests_1; docker-compose up |
|
@techalchemy By the way, you asked me for some suitable test, but the main goal of this PR (and this one as well) was to make the tests works in local environment. |
|
Hi @techalchemy, I created a corresponding issue and I need you to confirm that I should not create any Thank you! |
|
ping @uranusjr |
|
ping @techalchemy |
|
will look this over again before i cut the next release, sorry about that |
|
Based on the commit you referenced it seems like we just need to call And we should be normalizing it before checking as well? |
To be honest, I have no idea. Sorry, I didn't investigate this issue deeper. By the way, did you read the
That's it. I just know that the condition was inverted by mistake and it causes the problem for me. |
Look, you opened a PR with code changes and you keep telling me a solution, but honestly it is not possible for me to assess a solution until I know the problem. You showed me a commit you say I need to revert in order to cause the problem, but you also say this is still a problem. The commit you showed me just looks for the virtualenv directory but doesn't do any case normalization. Ultimately, I really don't know how to ask for this differently, I need you to show me what is wrong by showing me an example of something that is currently behaving incorrectly. Telling me that someone forgot to put a Here is an example of what I am looking for: Hi. I am a user of pipenv, and I ran this command: But as you can see, that directory should really have been found because it exists here: It's going to be hard to move forward on this until I can see something showing me why the current code is wrong. |
|
Hi there, I'm ill-fated user of mkdir pipenv-tests/{test,TEST} -p
cd pipenv-tests/test
pipenv shell
pipenv --venv
exit
cd ../TEST
pipenv shell
pipenv --venvHere is what I've got: ~/projects $ mkdir pipenv-tests/{test,TEST} -p
~/projects $ cd pipenv-tests/test
~/projects/pipenv-tests/test $ pipenv shell
Creating a virtualenv for this project…
Pipfile: /home/mim/projects/pipenv-tests/test/Pipfile
Using /usr/bin/python3.7 (3.7.0) to create virtualenv…
⠼Already using interpreter /usr/bin/python3.7
Using base prefix '/usr'
/home/mim/.local/lib/python3.7/site-packages/virtualenv.py:1041: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
import imp
New python executable in /home/mim/.local/share/virtualenvs/test-Ic8GqeXz/bin/python3.7
Also creating executable in /home/mim/.local/share/virtualenvs/test-Ic8GqeXz/bin/python
Installing setuptools, pip, wheel...done.
Virtualenv location: /home/mim/.local/share/virtualenvs/test-Ic8GqeXz
Creating a Pipfile for this project…
Launching subshell in virtual environment…
~/projects/pipenv-tests/test $ . /home/mim/.local/share/virtualenvs/test-Ic8GqeXz/bin/activate
(test)~/projects/pipenv-tests/test $ pipenv --venv
/home/mim/.local/share/virtualenvs/test-Ic8GqeXz
(test)~/projects/pipenv-tests/test $ exit
exit
~/projects/pipenv-tests/test $ cd ../TEST
~/projects/pipenv-tests/TEST $ pipenv shell
Creating a Pipfile for this project…
Launching subshell in virtual environment…
~/projects/pipenv-tests/TEST $ . /home/mim/.local/share/virtualenvs/test-Ic8GqeXz/bin/activate
(test)~/projects/pipenv-tests/TEST $ pipenv --venv
/home/mim/.local/share/virtualenvs/test-Ic8GqeXz
(test)~/projects/pipenv-tests/TEST $ There should be two different virtual environments for I'm expecting that (test)~/projects/pipenv-tests/TEST $ exit
exit
~/projects/pipenv-tests/TEST $ pipenv --rm
Removing virtualenv (/home/mim/.local/share/virtualenvs/test-Ic8GqeXz)…
~/projects/pipenv-tests/TEST $ pipenv shell
Creating a virtualenv for this project…
Pipfile: /home/mim/projects/pipenv-tests/TEST/Pipfile
Using /usr/bin/python3.7m (3.7.0) to create virtualenv…
⠹Running virtualenv with interpreter /usr/bin/python3.7m
Using base prefix '/usr'
/home/mim/.local/lib/python3.7/site-packages/virtualenv.py:1041: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
import imp
New python executable in /home/mim/.local/share/virtualenvs/TEST-TKzGZNiV/bin/python3.7m
Also creating executable in /home/mim/.local/share/virtualenvs/TEST-TKzGZNiV/bin/python
Installing setuptools, pip, wheel...done.
Virtualenv location: /home/mim/.local/share/virtualenvs/TEST-TKzGZNiV
Launching subshell in virtual environment…
~/projects/pipenv-tests/TEST $ . /home/mim/.local/share/virtualenvs/TEST-TKzGZNiV/bin/activate
(TEST)~/projects/pipenv-tests/TEST $ |
|
@techalchemy Should I explain why does it happen? |
There was a problem hiding this comment.
I think this is correct. Could you modify the news fragment to be more targeted to the user? The fragment is used to format our release notes, and “fix an internal function” doesn’t really mean anything to an end user. Something like “Fix hash comparison logic on case insensitive system so projects with different casing do not reuse the same virtual environment” would be better (you don’t need to use my version, I am terrible at writing sentences like this).
Case-sensitive filesystems were handled like case-insensitive and vice versa. Close #3151 This changes also: - Add PyCharm's config directory to .gitignore
techalchemy
added
the
PR: awaiting-merge
The issue
This PR closes #3151.
Case-sensitive filesystems are handled like case-insensitive and vice versa.
This bug was introduced in #2513 and it affects
onlymasterbranch and does not affect previous release2018.10.9and2018.10.13releases.The fix
This PR adds
notbefore the related condition.Additional changes
.gitignoreCC
The checklist
news/directory to describe this fix with the extension.bugfix.