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

Inconsistent drive letter on Windows when using tox and pytest #1381

Closed
eine opened this issue Aug 7, 2020 · 16 comments
Closed

Inconsistent drive letter on Windows when using tox and pytest #1381

eine opened this issue Aug 7, 2020 · 16 comments
Assignees
Labels
documentation Improvements or additions to documentation investigate Collect additional information, like space on disk, other tool incompatibilities etc. OS: Windows

Comments

@eine
Copy link

eine commented Aug 7, 2020

I'm finding that the same branch/commit works in a repo but fails in a fork. The difference I found is that drive letters are uppercase in the former and lowercase in the latter:

The command that is being tested is tox -e py36-unit -- --color=yes from VUnit/vunit, which executes {envpython} -m pytest -v -ra tests/unit {posargs}.

AFAIU, Windows is not case sensitive, hence I'd expect drive letters to always be uppercase.

@al-cheb al-cheb added documentation Improvements or additions to documentation investigate Collect additional information, like space on disk, other tool incompatibilities etc. OS: Windows and removed needs triage labels Aug 10, 2020
@al-cheb
Copy link
Contributor

al-cheb commented Aug 10, 2020

Hello, @eine
Could you please provide a simple test to reproduce this behavior? Does it reproduce with default shell(pwsh) instead of bash?

@eine
Copy link
Author

eine commented Aug 10, 2020

@al-cheb, unfortunately, I don't know how to simplify it further. The workflows linked above do only:

  • Checkout.
  • Setup Python.
  • Install deps through pip.
  • Execute tox.

The first thing tox does, which is to setup the environment, is already inconsistent: GLOB sdist-make: D:\a\vunit\vunit\setup.py vs GLOB sdist-make: d:\a\vunit\vunit\setup.py. If I remove any of the previous steps, tox cannot be executed.

I will try with powershell.

@al-cheb
Copy link
Contributor

al-cheb commented Aug 10, 2020

@eine,
I can't reproduce the issue from my side using 'VUnit/vunit' / 'eine/vunit' repoes.

steps:
    - uses: actions/checkout@v2
      with:
        repository: 'VUnit/vunit'
    - uses: actions/setup-python@v1
      with:
        python-version: 3.8
    - name: install dependencies
      run: |
        pip install -U pip --progress-bar off
        pip install -U virtualenv tox --progress-bar off
    - name: run job
      shell: bash
      run: |
        export PATH=$PATH:$(pwd)/../ghdl-v0.37/bin
        tox -e py36-unit -- --color=yes 

image

Second variant:

    - uses: actions/checkout@v2
      with:
        repository: 'eine/vunit'

image

@eine
Copy link
Author

eine commented Aug 10, 2020

@al-cheb see this run: https://github.com/eine/vunit/runs/966666285?check_suite_focus=true It is a stripped down version of the master workflow: eine/vunit@a1378eb As you see, it works with powershell, not with bash. Compare the following lines:

@eine
Copy link
Author

eine commented Aug 10, 2020

@al-cheb, please note that the error seems not to be consistent. That is, sometimes bash will use capital letters (which is what it has happened consistently for several months), but sometimes it will use lowercase. I feel this might be a regression that has been deployed to some machines but not all of them. Hence, it might be difficult to track/reproduce in a new repo. However, once a repo starts failing, it seems to keep failing consistently. Dunno whether this makes any sense to you.

@al-cheb
Copy link
Contributor

al-cheb commented Aug 10, 2020

@eine,
I agree very strange behavior:
https://github.com/eine/vunit/runs/959031527?check_suite_focus=true (all in lowercase)

GLOB sdist-make: d:\a\vunit\vunit\setup.py
py38-unit create: d:\a\vunit\vunit\.tox\py38-unit
py38-unit installdeps: pytest
py38-unit inst: d:\a\vunit\vunit\.tox\.tmp\package\1\vunit_hdl-4.4.1rc0.zip 

https://github.com/eine/vunit/runs/966666285?check_suite_focus=true (only first one in lowercase)

GLOB sdist-make: d:\a\vunit\vunit\setup.py
py38-unit recreate: D:\a\vunit\vunit\.tox\py38-unit
py38-unit installdeps: pytest
py38-unit inst: D:\a\vunit\vunit\.tox\.tmp\package\1\vunit_hdl-4.4.1rc0.zip

@eine
Copy link
Author

eine commented Aug 10, 2020

I'm pinging @gaborbernat (maintainer of tox). Bernát, can you help us understand whether this is an issue with tox, with Python or with GitHub's virtual environment? Basically, GLOB sdist-make sometimes gets a lowercase drive letter, and sometimes it gets an uppercase.

FTR, not the same issue, but related to another bash on Windows: tox-dev/tox#1648

@gaborbernat
Copy link

I'll check but first thing first, no one should be using the sdist mode anymore, and users are encouraged to switch to the isoalted build mode https://tox.readthedocs.io/en/latest/example/package.html?highlight=isolated_build

@al-cheb al-cheb self-assigned this Aug 10, 2020
@eine
Copy link
Author

eine commented Aug 10, 2020

@gaborbernat, I was not aware of isolated builds or pyproject.toml. I changed that: eine/vunit@50efb65. Then, it produced an error because it could not find some internal resources (https://github.com/eine/vunit/runs/967036508?check_suite_focus=true#step:5:22). I worked around it: eine/vunit@a4334d6. Still, the original issue (drive letter case mismatch) remains. Compare the rootdir in powershell or bash:

EDIT

Maybe rootdir belongs to pytest?

@eine
Copy link
Author

eine commented Aug 10, 2020

It seems that pytest v5 was able to handle this inconsistency, i.e., it uppercased the drive regardless of how it was retrieved. However, v6 preserves it as received from the environment, which is not correct (dunno if due to tox or GitHub, yet). See:

pinging @fabioz, @bluetech and @nicoddemus, who have been modifying path related features in pytest: pytest-dev/pytest#6523, pytest-dev/pytest#7619.

@nicoddemus
Copy link

Hi folks, sorry for the delay.

Note that you can get upper case/lower case paths depending on how you change cd into a path:

D:\>cd d:\projects

d:\projects>cd D:\

D:\>cd D:\projects

D:\projects>

I see the inconsistent drive letters are being seen right at the start of the tox run, so I suspect is something related to how it the environment cds into the directory.

@eine
Copy link
Author

eine commented Aug 12, 2020

@nicoddemus thanks for stepping by! I'm not sure I understand which is "the environment" in your comment. tox seems to report uppercases (after the changes suggested by @gaborbernat). I believe the first lowercase log is generated by pytest (https://github.com/eine/vunit/runs/967060891?check_suite_focus=true#step:6:18). How does pytests read/get rootdir?

Note that with v5.4.3 pytest will report rootdir with lowercase, but it will internally uppercase it (thus, execution is successful): https://github.com/eine/vunit/runs/967200957?check_suite_focus=true#step:6:18. So, this is (maybe unwantedly) properly worked around in pytest v5, but it fails in v6. I believe that, regardless of the underlaying issue being fixed in either tox or the virtual-environment, it would be interesting for pytest v6 to handle it.

@fabioz
Copy link

fabioz commented Aug 12, 2020

What change was that pytest resolved the path and it doesn't anymore (because, if you had some subst x: pointing to c:\x it'd not only fix the drive letter, it'd actually change the path to be c:\x instead of x:).

Now it just accepts and keeps what's passed to it.

So, in this case it's probably the caller of pytest that needs fixing to keep it consistent (apparently tox?).

Although I must say that if a test fails due to how you call it having an uppercase or lowercase drive letter (which is pretty inconsistent on windows itself) I'd probably fix the test...

@eine
Copy link
Author

eine commented Aug 12, 2020

Now it just accepts and keeps what's passed to it.

Thanks for clarifying.

So, in this case it's probably the caller of pytest that needs fixing to keep it consistent (apparently tox?).

This is what I'm trying to understand. I'd say that VUnit preserves what's passed too. Hence, I don't know where is the inconsistency being created.

Although I must say that if a test fails due to how you call it having an uppercase or lowercase drive letter (which is pretty inconsistent on windows itself) I'd probably fix the test...

These are some unit tests for some Python code. It "just" calls some functions, generates some logs and it then compares the output. Aanyway, I will look deeper into it.

@nicoddemus
Copy link

nicoddemus commented Aug 12, 2020

I'm not sure I understand which is "the environment" in your comment.

In this case I meant the GH actions envirionment, sorry for not being clearer.

So, this is (maybe unwantedly) properly worked around in pytest v5, but it fails in v6. I believe that, regardless of the underlaying issue being fixed in either tox or the virtual-environment, it would be interesting for pytest v6 to handle it.

As @fabioz commented, now pytest won't try to resolve the paths anymore (resolve meaning to follow symlinks/subst drives), so it should keep the same paths internally as given by it externally. If that's not happening then I agree it is probably a bug in pytest, but I'm not sure that's what is happenning here because the different uppercase/lowercase drive letters show up as early as the start of the tox run.

eine added a commit to eine/vunit that referenced this issue Aug 19, 2020
@eine
Copy link
Author

eine commented Aug 19, 2020

I tried to investigate this a bit more, but it seems to be fixed magically. Even though neither of the dependencies changed (tox 3.19.0, pytest 6.0.1) it's successful now. Hence, I'm closing this issue. I'll reopen if I find the problem again.

@eine eine closed this as completed Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation investigate Collect additional information, like space on disk, other tool incompatibilities etc. OS: Windows
Projects
None yet
Development

No branches or pull requests

5 participants