Skip to content
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

Fix activator path in cygwin and msys2 #1946

Closed
wants to merge 1 commit into from
Closed

Fix activator path in cygwin and msys2 #1946

wants to merge 1 commit into from

Conversation

danyeaw
Copy link
Contributor

@danyeaw danyeaw commented Sep 22, 2020

Closes #1940. In cygwin and MSYS2, the path is in POSIX format. This PR converts the Windows path to POSIX format using the cygpath utility so that it is added to the bash activation script in the correct format.

Thanks for contributing, make sure you address all the checklists (for details on how see

development documentation)!

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #1946 into master will decrease coverage by 8.34%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1946      +/-   ##
==========================================
- Coverage   93.92%   85.57%   -8.35%     
==========================================
  Files          86       86              
  Lines        4228     4237       +9     
==========================================
- Hits         3971     3626     -345     
- Misses        257      611     +354     
Flag Coverage Δ
#tests 85.57% <77.77%> (-8.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/virtualenv/activation/via_template.py 96.15% <77.77%> (-3.85%) ⬇️
src/virtualenv/discovery/windows/pep514.py 0.00% <0.00%> (-100.00%) ⬇️
src/virtualenv/discovery/windows/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
src/virtualenv/util/subprocess/_win_subprocess.py 0.00% <0.00%> (-95.09%) ⬇️
src/virtualenv/util/path/_pathlib/via_os_path.py 0.00% <0.00%> (-91.51%) ⬇️
src/virtualenv/create/via_global_ref/store.py 50.00% <0.00%> (-40.00%) ⬇️
src/virtualenv/activation/batch/__init__.py 66.66% <0.00%> (-33.34%) ⬇️
.../create/via_global_ref/builtin/cpython/cpython3.py 67.39% <0.00%> (-28.27%) ⬇️
src/virtualenv/info.py 77.50% <0.00%> (-22.50%) ⬇️
...nv/create/via_global_ref/builtin/cpython/common.py 86.84% <0.00%> (-13.16%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aed87ac...7b7c0e3. Read the comment docs.

Closes #1940. In cygwin and MSYS2, the path is in POSIX format.
This PR converts the Windows path to POSIX format using a regex
so that it is added to the bash activation script in the correct
format.
@danyeaw
Copy link
Contributor Author

danyeaw commented Sep 22, 2020

@gaborbernat I have updated the PR with the regex 👍
I noticed on the tests side that the activator isn't run in Windows because of WSL on GitHub Actions. Any thoughts on if additional tests could be added somehow?

@gaborbernat
Copy link
Contributor

Any thoughts on if additional tests could be added somehow?

Write a uni test. Mock sysconfig.get_platform, check generated scripts contain correct path.

@danyeaw
Copy link
Contributor Author

danyeaw commented Sep 25, 2020

@gaborbernat Thanks for the feedback, I have been exploring how to structure mock tests since I don't have much experience mocking things out. I'm hoping to finish it this weekend. Can we reopen this PR?

@gaborbernat
Copy link
Contributor

@danyeaw I did not close this PR on purpose. Was a side-effect of renaming the default branch to main. Please reopen the PR against the main branch. Sorry.

@danyeaw
Copy link
Contributor Author

danyeaw commented Sep 26, 2020

@gaborbernat No problem, I like the main branch rename! I have submitted #1952.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cygwin and msys2 bash activator compatibility
2 participants