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 a team November 2, 2018 03:47
@codecov
Copy link

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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
@stsewd stsewd deleted the use-home-as-cwd-for-venv branch November 5, 2018 18:47
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