erinxocon · GitHub

@erinxocon

Copy link Copy Markdown

Contributor

Path('R:\repo').resolve() seems to resolve to `\computername\repos\repo' when using a mounted drive. The path in a form of R:\repo\ is technically an absolute path already so it doesn't need converting.

@erinxocon

Closed

@ghost

Copy link Copy Markdown

@erinxocon Thank you very much for your help Erin! I'm trying to figure out how to pull your changes and rerun my tests to verify that it resolves my original issue.

@erinxocon

Copy link Copy Markdown

Contributor Author

@ahendry2688 Just pull from master and then checkout the unc-paths branch. I do see an error in CI, but think it might just be transient as it didn't fail the other windows test.

@ghost

Copy link Copy Markdown

@erinxocon Do you mean the following?:

git pull origin master, then
git checkout -b unc-paths.

@erinxocon

Copy link Copy Markdown

Contributor Author

@ahendry2688 indeed. Then ensure pipenv is uninstalled and set it up as a development dependency by doing pip install -e .

@ghost

ghost commented

Jun 9, 2018

edited by ghost

Loading

Copy link Copy Markdown

@erinxocon Thanks for your help. I'm still getting errors. Here are the commands I issued:

pip uninstall pipenv
pip install pytest-xdist
git pull origin master
git checkout -b unc-paths
pip install -e .
pipenv run pytest tests -n 24

And here is the output (see attached .txt file):

pytest_output_PR2331.txt

(EDIT: I realized I'm running on a different machine. I should have used -n auto above. I'll rerun test and give you output in another comment)

@ghost

Copy link Copy Markdown

@ghost

Copy link Copy Markdown

@ghost

Copy link Copy Markdown

@techalchemy

Copy link Copy Markdown

Member

All you should need to run is set PYPI_VENDOR_DIR=".\tests\pypi\" && set PYTHONIOENCODING="utf-8" && cd pipenv && pip install -e . && pipenv install --dev && pipenv run pytest -v -n 4 --ignore=".\\pipenv\\patched" --ignore=".\\pipenv\\vendor" tests

@ghost

Copy link Copy Markdown

@erinxocon Even with techalchemy's suggestions, my tests still report errors. I will continue to look into this. Please let me know if you do not want my text file outputs anymore and I will stop sending them.

pytest_output_PR2331_run003.txt

@erinxocon

Copy link Copy Markdown

Contributor Author

@techalchemy pipfile checks if the file is exists and if not it returns none, so it handles checking if the files exists and it's caught by normalize if it's none. Probably don't need to use resolve as pipfile will always pass it an absolute path, but if someone sets the env variable to change the pipfile location I suppose resolve is necessary so I'll keep the PR as it is.

@techalchemy

Copy link Copy Markdown

Member

@erinxocon there was a concern around it accurately writing to a Pipfile if the pipfile itself is stored on a share, I don't currently have a way to test this scenario but I suspect if we aren't calling normalize_path everywhere it might be a source of problems. We might need to brainstorm a unit test for an edge case or something

@erinxocon

Copy link Copy Markdown

Contributor Author

@techalchemy so far this is the only way I can get it to write to the pipfile right now with my mounted drives. Do you know how to mount shares on app veyor? I'll have to do some research to see about setting up a test like that

@ghost

Copy link Copy Markdown

Thank you both so much for all your hard work! If it's any consolation, my company uses all shared drives to collaborate with colleagues as we have yet to purchase a private GitHub repo. This PR would be a huge help to us, and I'm hoping to other users as well. Thank you!

@erinxocon

Copy link Copy Markdown

Contributor Author

I ran the test suite from a mounted drive and got no errors with the fix I put in place. Thought of a cleanup idea, but this appears to be working. Now for a test case on app veyor..

@techalchemy

Copy link Copy Markdown

Member

@erinxocon when you get a chance can you uncomment the appveyor matrix?

@erinxocon

@erinxocon

Copy link Copy Markdown

Contributor Author

@techalchemy done, sorry bout that, was messing around with app veyor. Squashed the commits out of existence.

techalchemy

if loc.is_absolute():
return normalize_drive(str(loc))
else:
return normalize_drive(str(loc.resolve()))

Copy link Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on our findings in the other PR we've been hammering away at, can you make this second part into:

    else:
        try:
            loc = loc.resolve()
        except OSError:
            loc = loc.absolute()
        return normalize_drive(str(loc))

In order to handle Ramdisks...

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@techalchemy

Copy link Copy Markdown

Member

🍰

@techalchemy

@uranusjr

Closed

Read the original on github.com ↗