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

Use $HOME as CWD for virtualenv creation #4852

Merged
merged 1 commit into from Nov 5, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Nov 2, 2018

Turns out we do support shell expansions in the CWD :)

Closes #4808

@stsewd stsewd requested a review from Nov 2, 2018
@codecov
Copy link

@codecov codecov bot commented Nov 2, 2018

Codecov Report

Merging #4852 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4852   +/-   ##
=======================================
  Coverage   76.41%   76.41%           
=======================================
  Files         158      158           
  Lines        9990     9990           
  Branches     1262     1262           
=======================================
  Hits         7634     7634           
  Misses       2016     2016           
  Partials      340      340
Impacted Files Coverage Δ
readthedocs/doc_builder/python_environments.py 82.97% <ø> (ø) ⬆️

# Don't use virtualenv bin that doesn't exist yet
bin_path=None,
# Don't use the project's root, some config files can interfere
cwd='$HOME',
Copy link
Member

@ericholscher ericholscher Nov 5, 2018

I wonder if we should be running this from /tmp or something else that isn't in the checkout. I guess we aren't always sure any other path will exist (unless we make one w/ the tempfile module), so this seems fine. But I do wonder if we'll hit other issues at some point (eg. if we add our own pyproject.toml` file for RTD?

Copy link
Member Author

@stsewd stsewd Nov 5, 2018

Do the docker containers have access to the rtd code? I don't think so. But, yeah, using /tmp sounds better. People running this outside docker could have some problems I think, but I'm less worried about that. Should I go for plain /tmp or create one with the tempfile module?

Copy link
Member

@ericholscher ericholscher Nov 5, 2018

Ah good point, I forgot about Docker not having access. I think we can just go with $HOME for now, because it feels overkill (and could lead to errors) to create a temp directory.

@ericholscher ericholscher merged commit ee282a5 into readthedocs:master Nov 5, 2018
3 checks passed
@stsewd stsewd deleted the use-home-as-cwd-for-venv branch Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants