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

added support for mounted drives via unc paths. #2331

Merged
merged 6 commits into from
Jun 14, 2018
Merged

Conversation

erinxocon
Copy link
Contributor

@erinxocon erinxocon commented Jun 9, 2018

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.

@ghost
Copy link

ghost commented Jun 9, 2018

@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
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

ghost commented Jun 9, 2018

@erinxocon Do you mean the following?:

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

@erinxocon
Copy link
Contributor Author

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

@ghost
Copy link

ghost commented Jun 9, 2018

@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

ghost commented Jun 9, 2018

@erinxocon Corrected using -n auto. Errors still remain.

pytest_output_PR2331_using_n_auto.txt

@ghost
Copy link

ghost commented Jun 9, 2018

@erinxocon If it helps, here is my output from running pytest --fixtures.

pytest_fixtures_output.txt

@ghost
Copy link

ghost commented Jun 9, 2018

@erinxocon Installed pytest-fixtures and pytest-pypi. Rerunning tests, but now getting many more errors.
pytest_output_PR2331_run002.txt

@techalchemy
Copy link
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

ghost commented Jun 9, 2018

@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
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
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
Contributor Author

erinxocon commented Jun 10, 2018

@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

ghost commented Jun 10, 2018

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
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..

@erinxocon erinxocon closed this Jun 10, 2018
@erinxocon erinxocon reopened this Jun 10, 2018
@techalchemy
Copy link
Member

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

@erinxocon
Copy link
Contributor Author

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

if loc.is_absolute():
return normalize_drive(str(loc))
else:
return normalize_drive(str(loc.resolve()))
Copy link
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
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
Member

🍰

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.

2 participants