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

Check versions used to create the venv and auto-wipe #3432

Merged
merged 12 commits into from
Dec 28, 2017
25 changes: 15 additions & 10 deletions readthedocs/doc_builder/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,7 @@ def extra_requirements(self):

@property
def python_interpreter(self):
ver = self.python_version
if ver in [2, 3]:
# Get the highest version of the major series version if user only
# gave us a version of '2', or '3'
ver = max(
list(
filter(
lambda x: x < ver + 1,
self._yaml_config.get_valid_python_versions(),
)))
ver = self.python_full_version
return 'python{0}'.format(ver)

@property
Expand All @@ -75,6 +66,20 @@ def python_version(self):
version = 3
return version

@property
def python_full_version(self):
ver = self.python_version
if ver in [2, 3]:
# Get the highest version of the major series version if user only
# gave us a version of '2', or '3'
ver = max(
list(
filter(
lambda x: x < ver + 1,
self._yaml_config.get_valid_python_versions(),
)))
return ver

@property
def use_system_site_packages(self):
if 'use_system_site_packages' in self._yaml_config.get('python', {}):
Expand Down
84 changes: 81 additions & 3 deletions readthedocs/doc_builder/python_environments.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
# -*- coding: utf-8 -*-
"""An abstraction over virtualenv and Conda environments."""

from __future__ import absolute_import
from builtins import object
from __future__ import (
absolute_import, division, print_function, unicode_literals)

import json
import logging
import os
import shutil
from builtins import object, open

from django.conf import settings

from readthedocs.doc_builder.config import ConfigWrapper
from readthedocs.doc_builder.constants import DOCKER_IMAGE
from readthedocs.doc_builder.loader import get_builder_class
from readthedocs.projects.constants import LOG_TEMPLATE
from readthedocs.projects.models import Feature
Expand Down Expand Up @@ -38,7 +43,6 @@ def _log(self, msg):
msg=msg))

def delete_existing_build_dir(self):

# Handle deleting old build dir
build_dir = os.path.join(
self.venv_path(),
Expand All @@ -47,6 +51,13 @@ def delete_existing_build_dir(self):
self._log('Removing existing build directory')
shutil.rmtree(build_dir)

def delete_existing_venv_dir(self):
venv_dir = self.venv_path()
# Handle deleting old venv dir
if os.path.exists(venv_dir):
self._log('Removing existing venv directory')
shutil.rmtree(venv_dir)

def install_package(self):
if self.config.install_project:
if self.config.pip_install or getattr(settings, 'USE_PIP_INSTALL', False):
Expand Down Expand Up @@ -87,6 +98,73 @@ def venv_bin(self, filename=None):
parts.append(filename)
return os.path.join(*parts)

def environment_json_path(self):
"""Return the path to the ``readthedocs-environment.json`` file."""
return os.path.join(
self.venv_path(),
'readthedocs-environment.json',
)

@property
def is_obsolete(self):
"""
Determine if the Python version of the venv obsolete.

It checks the the data stored at ``readthedocs-environment.json`` and
compares it against the Python version in the project version to be
built and the Docker image used to create the venv against the one in
the project version config.

:returns: ``True`` when it's obsolete and ``False`` otherwise

:rtype: bool
"""
# Always returns False if we don't have information about what Python
# version/Docker image was used to create the venv as backward
# compatibility.
if not os.path.exists(self.environment_json_path()):
return False

try:
with open(self.environment_json_path(), 'r') as fpath:
environment_conf = json.load(fpath)
env_python_version = environment_conf['python']['version']
env_build_image = environment_conf['build']['image']
except (IOError, TypeError, KeyError, ValueError):
log.error('Unable to read/parse readthedocs-environment.json file')
return False

# TODO: remove getattr when https://github.com/rtfd/readthedocs.org/pull/3339 got merged
build_image = getattr(self.config, 'build_image', self.version.project.container_image) or DOCKER_IMAGE # noqa

# If the user define the Python version just as a major version
# (e.g. ``2`` or ``3``) we won't know exactly which exact version was
# used to create the venv but we can still compare it against the new
# one coming from the project version config.
return any([
env_python_version != self.config.python_full_version,
env_build_image != build_image,
])

def save_environment_json(self):
"""Save on disk Python and build image versions used to create the venv."""
# TODO: remove getattr when https://github.com/rtfd/readthedocs.org/pull/3339 got merged
build_image = getattr(self.config, 'build_image', self.version.project.container_image) or DOCKER_IMAGE # noqa

data = {
'python': {
'version': self.config.python_full_version,
},
'build': {
'image': build_image,
},
}

with open(self.environment_json_path(), 'w') as fpath:
# Compatibility for Py2 and Py3. ``io.TextIOWrapper`` expects
# unicode but ``json.dumps`` returns str in Py2.
fpath.write(unicode(json.dumps(data)))


class Virtualenv(PythonEnvironment):

Expand Down
10 changes: 9 additions & 1 deletion readthedocs/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,16 @@ def setup_environment(self):
version=self.version,
max_lock_age=getattr(settings, 'REPO_LOCK_SECONDS', 30)):

self.python_env.delete_existing_build_dir()
# Check if the python version/build image in the current venv is the
# same to be used in this build and if it differs, wipe the venv to
# avoid conflicts.
if self.python_env.is_obsolete:
self.python_env.delete_existing_venv_dir()
else:
self.python_env.delete_existing_build_dir()

self.python_env.setup_base()
self.python_env.save_environment_json()
self.python_env.install_core_requirements()
self.python_env.install_user_requirements()
self.python_env.install_package()
Expand Down
137 changes: 136 additions & 1 deletion readthedocs/rtd_tests/tests/test_doc_building.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,22 @@
from builtins import str

import mock
import pytest
from django.test import TestCase
from docker.errors import APIError as DockerAPIError
from docker.errors import DockerException
from mock import Mock, PropertyMock, patch
from mock import Mock, PropertyMock, mock_open, patch

from readthedocs.builds.constants import BUILD_STATE_CLONING
from readthedocs.builds.models import Version
from readthedocs.doc_builder.config import ConfigWrapper
from readthedocs.doc_builder.environments import (
BuildCommand, DockerBuildCommand, DockerEnvironment, LocalEnvironment)
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
from readthedocs.doc_builder.python_environments import Virtualenv
from readthedocs.projects.models import Project
from readthedocs.rtd_tests.mocks.environment import EnvironmentMockGroup
from readthedocs.rtd_tests.tests.test_config_wrapper import create_load

DUMMY_BUILD_ID = 123
SAMPLE_UNICODE = u'HérÉ îß sömê ünïçó∂é'
Expand Down Expand Up @@ -831,3 +835,134 @@ def test_command_oom_kill(self):
self.assertEqual(
str(cmd.output),
u'Command killed due to excessive memory consumption\n')




class TestAutoWipeEnvironment(TestCase):
fixtures = ['test_data']

def setUp(self):
self.pip = Project.objects.get(slug='pip')
self.version = self.pip.versions.get(slug='0.8')

def test_is_obsolete_without_env_json_file(self):
yaml_config = create_load()()[0]
config = ConfigWrapper(version=self.version, yaml_config=yaml_config)

with patch('os.path.exists') as exists:
exists.return_value = False
python_env = Virtualenv(
version=self.version,
build_env=None,
config=config,
)

self.assertFalse(python_env.is_obsolete)

def test_is_obsolete_with_invalid_env_json_file(self):
yaml_config = create_load()()[0]
config = ConfigWrapper(version=self.version, yaml_config=yaml_config)

with patch('os.path.exists') as exists:
exists.return_value = True
python_env = Virtualenv(
version=self.version,
build_env=None,
config=config,
)

self.assertFalse(python_env.is_obsolete)

def test_is_obsolete_with_json_different_python_version(self):
config_data = {
'build': {
'image': '2.0',
},
'python': {
'version': 2.7,
},
}
yaml_config = create_load(config_data)()[0]
config = ConfigWrapper(version=self.version, yaml_config=yaml_config)

python_env = Virtualenv(
version=self.version,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 3.5}}'
with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa
Copy link
Member Author

Choose a reason for hiding this comment

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

These with patch() lines are terrible, but I didn't find a better way to write them :(

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to escape with backslash here according to pep8 https://www.python.org/dev/peps/pep-0008/#maximum-line-length

exists.return_value = True
self.assertTrue(python_env.is_obsolete)

@pytest.mark.xfail(reason='build.image is not being considered yet')
Copy link
Member Author

Choose a reason for hiding this comment

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

to make this test pass, we need the PR that get the image from yaml merged... then we can remove this line and it should pass :)

def test_is_obsolete_with_json_different_build_image(self):
config_data = {
'build': {
'image': 'latest',
},
'python': {
'version': 2.7,
},
}
yaml_config = create_load(config_data)()[0]
config = ConfigWrapper(version=self.version, yaml_config=yaml_config)

python_env = Virtualenv(
version=self.version,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 2.7}}'
with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa
exists.return_value = True
self.assertTrue(python_env.is_obsolete)

def test_is_obsolete_with_project_different_build_image(self):
config_data = {
'build': {
'image': '2.0',
},
'python': {
'version': 2.7,
},
}
yaml_config = create_load(config_data)()[0]
config = ConfigWrapper(version=self.version, yaml_config=yaml_config)

# Set container_image manually
self.pip.container_image = 'readthedocs/build:latest'
self.pip.save()

python_env = Virtualenv(
version=self.version,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 2.7}}'
with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa
exists.return_value = True
self.assertTrue(python_env.is_obsolete)

def test_is_obsolete_with_json_same_data_as_version(self):
config_data = {
'build': {
'image': '2.0',
},
'python': {
'version': 3.5,
},
}
yaml_config = create_load(config_data)()[0]
config = ConfigWrapper(version=self.version, yaml_config=yaml_config)

python_env = Virtualenv(
version=self.version,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 3.5}}'
with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa
exists.return_value = True
self.assertFalse(python_env.is_obsolete)