Skip to content

[RDY] Fix requirements file lookup #3800

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Apr 17, 2018
Merged

[RDY] Fix requirements file lookup #3800

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9731997
Add tests for install core requirements
stsewd Mar 14, 2018
9aecefc
Add tests for user requirements
stsewd Mar 14, 2018
210922b
Fix install_user_requirements
stsewd Mar 14, 2018
028077b
Add tests for conda
stsewd Mar 14, 2018
928ab3e
Reuse the same requirements
stsewd Mar 23, 2018
b765727
Add docstring and comments
stsewd Mar 23, 2018
6446f48
Reuse conda requirements
stsewd Mar 23, 2018
029c078
Reuse pip install args
stsewd Mar 23, 2018
28c8a0b
Merge branch 'master' into fix-requirements-file-lookup
stsewd Mar 23, 2018
507b714
Reset mock last test
stsewd Mar 23, 2018
636d487
Merge branch 'master' into fix-requirements-file-lookup
stsewd Mar 27, 2018
d482c89
Merge branch 'master' into fix-requirements-file-lookup
stsewd Mar 27, 2018
98d47ec
Merge branch 'master' into fix-requirements-file-lookup
stsewd Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions readthedocs/doc_builder/python_environments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from __future__ import (
absolute_import, division, print_function, unicode_literals)

import itertools
import json
import logging
import os
Expand Down Expand Up @@ -252,12 +253,13 @@ def install_user_requirements(self):
builder_class = get_builder_class(self.project.documentation_type)
docs_dir = (builder_class(build_env=self.build_env, python_env=self)
.docs_dir())
for path in [docs_dir, '']:
for req_file in ['pip_requirements.txt', 'requirements.txt']:
test_path = os.path.join(self.checkout_path, path, req_file)
if os.path.exists(test_path):
requirements_file_path = test_path
break
paths = [docs_dir, '']
req_files = ['pip_requirements.txt', 'requirements.txt']
for path, req_file in itertools.product(paths, req_files):
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any logic change here, right?

We just had 2 nested for and now we use itertools.products that produces the same result in the end. Am I right?

Copy link
Member

Choose a reason for hiding this comment

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

>>> import itertools
>>> d = ['a', 'b']
>>> r = [1, 2]
>>> iter
iter(      itertools  
>>> list(itertools.product(d, r))
[('a', 1), ('a', 2), ('b', 1), ('b', 2)]
>>> for _d in d:
...     for _r in r:
...         print(_d, _r)
... 
a 1
a 2
b 1
b 2
>>>

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the same result, but the break now exists the loop #2812 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Oh boy! Great catch! :)

test_path = os.path.join(self.checkout_path, path, req_file)
if os.path.exists(test_path):
requirements_file_path = test_path
break

if requirements_file_path:
args = [
Expand Down
239 changes: 238 additions & 1 deletion readthedocs/rtd_tests/tests/test_doc_building.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,18 @@
from docker.errors import APIError as DockerAPIError
from docker.errors import DockerException
from mock import Mock, PropertyMock, mock_open, patch
from django_dynamic_fixture import get

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, DockerBuildEnvironment, LocalBuildEnvironment)
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
from readthedocs.doc_builder.python_environments import Virtualenv
from readthedocs.doc_builder.python_environments import Conda, Virtualenv
from readthedocs.projects.models import Project
from readthedocs.rtd_tests.mocks.environment import EnvironmentMockGroup
from readthedocs.rtd_tests.mocks.paths import fake_paths_lookup
from readthedocs.rtd_tests.tests.test_config_wrapper import create_load

DUMMY_BUILD_ID = 123
Expand Down Expand Up @@ -839,6 +841,241 @@ def test_command_oom_kill(self):
u'Command killed due to excessive memory consumption\n')


class TestPythonEnvironment(TestCase):

def setUp(self):
self.project_sphinx = get(Project, documentation_type='sphinx')
self.version_sphinx = get(Version, project=self.project_sphinx)

self.project_mkdocs = get(Project, documentation_type='mkdocs')
self.version_mkdocs = get(Version, project=self.project_mkdocs)

self.build_env_mock = Mock()

self.base_requirements = [
'Pygments==2.2.0',
'setuptools==37.0.0',
'docutils==0.13.1',
'mock==1.0.1',
'pillow==2.6.1',
'alabaster>=0.7,<0.8,!=0.7.5',
]
self.base_conda_requirements = [
'mock',
'pillow',
]

self.pip_install_args = [
'python',
mock.ANY, # pip path
'install',
'--use-wheel',
'--upgrade',
'--cache-dir',
mock.ANY, # cache path
]

def test_install_core_requirements_sphinx(self):
python_env = Virtualenv(
version=self.version_sphinx,
build_env=self.build_env_mock,
)
python_env.install_core_requirements()
requirements_sphinx = [
'commonmark==0.5.4',
'recommonmark==0.4.0',
'sphinx==1.6.5',
'sphinx-rtd-theme<0.3',
'readthedocs-sphinx-ext<0.6',
]
requirements = self.base_requirements + requirements_sphinx
args = self.pip_install_args + requirements
self.build_env_mock.run.assert_called_once_with(
*args, bin_path=mock.ANY
)

def test_install_core_requirements_mkdocs(self):
python_env = Virtualenv(
version=self.version_mkdocs,
build_env=self.build_env_mock
)
python_env.install_core_requirements()
requirements_mkdocs = [
'commonmark==0.5.4',
'recommonmark==0.4.0',
'mkdocs==0.15.0',
]
requirements = self.base_requirements + requirements_mkdocs
args = self.pip_install_args + requirements
self.build_env_mock.run.assert_called_once_with(
*args, bin_path=mock.ANY
)

def test_install_user_requirements(self):
"""
If a projects does not specify a requirements file,
RTD will choose one automatically.

First by searching under the docs/ directory and then under the root.
The files can be named as:

- ``pip_requirements.txt``
- ``requirements.txt``
"""
self.build_env_mock.project = self.project_sphinx
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a docstring here explaining the test case in general?

self.build_env_mock.version = self.version_sphinx
python_env = Virtualenv(
version=self.version_sphinx,
build_env=self.build_env_mock
)

checkout_path = python_env.checkout_path
docs_requirements = os.path.join(
checkout_path, 'docs', 'requirements.txt'
)
root_requirements = os.path.join(
checkout_path, 'requirements.txt'
)
paths = {
os.path.join(checkout_path, 'docs'): True,
}
args = [
'python',
mock.ANY, # pip path
'install',
'--exists-action=w',
'--cache-dir',
mock.ANY, # cache path
'requirements_file'
]

# One requirements file on the docs/ dir
# should be installed
paths[docs_requirements] = True
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a simple comment in each of the cases saying what's the scenario and what we expect?

paths[root_requirements] = False
with fake_paths_lookup(paths):
python_env.install_user_requirements()
args[-1] = '-r{}'.format(docs_requirements)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this modification of the list, but I don't have a cleaner way of doing it :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't like it either, but it was the only way I thought to reuse the previous list p:

self.build_env_mock.run.assert_called_with(
*args, cwd=mock.ANY, bin_path=mock.ANY
)

# One requirements file on the root dir
# should be installed
paths[docs_requirements] = False
paths[root_requirements] = True
with fake_paths_lookup(paths):
python_env.install_user_requirements()
args[-1] = '-r{}'.format(root_requirements)
self.build_env_mock.run.assert_called_with(
*args, cwd=mock.ANY, bin_path=mock.ANY
)

# Two requirements files on the root and docs/ dirs
# the one on docs/ should be installed
paths[docs_requirements] = True
paths[root_requirements] = True
with fake_paths_lookup(paths):
python_env.install_user_requirements()
args[-1] = '-r{}'.format(docs_requirements)
self.build_env_mock.run.assert_called_with(
*args, cwd=mock.ANY, bin_path=mock.ANY
)

# No requirements file
# no requirements should be installed
self.build_env_mock.run.reset_mock()
Copy link
Member Author

Choose a reason for hiding this comment

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

With the last update of the mock lib, the assert_not_called() was counting the previous calls too, so I have to reset it here.

paths[docs_requirements] = False
paths[root_requirements] = False
with fake_paths_lookup(paths):
python_env.install_user_requirements()
self.build_env_mock.run.assert_not_called()

def test_install_core_requirements_sphinx_conda(self):
python_env = Conda(
version=self.version_sphinx,
build_env=self.build_env_mock,
)
python_env.install_core_requirements()
conda_sphinx = [
'sphinx',
'sphinx_rtd_theme',
]
conda_requirements = self.base_conda_requirements + conda_sphinx
pip_requirements = [
'recommonmark',
'readthedocs-sphinx-ext',
]

args_pip = [
'python',
mock.ANY, # pip path
'install',
'-U',
Copy link
Member

Choose a reason for hiding this comment

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

You can also use a shared args_pip for all these tests. You will need to change the -U by --upgrade but I think that's OK and it's better to use long attributes instead of short. So, 👍

Copy link
Member Author

@stsewd stsewd Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There is two types of the arg_pip, on conda the --use-wheel isn't used. Is that on purpose? or perhaps we can unify both :)?

'--cache-dir',
mock.ANY, # cache path
]
args_pip.extend(pip_requirements)

args_conda = [
'conda',
'install',
'--yes',
'--name',
self.version_sphinx.slug,
]
args_conda.extend(conda_requirements)

self.build_env_mock.run.assert_has_calls([
mock.call(*args_conda),
mock.call(*args_pip, bin_path=mock.ANY)
])

def test_install_core_requirements_mkdocs_conda(self):
python_env = Conda(
version=self.version_mkdocs,
build_env=self.build_env_mock,
)
python_env.install_core_requirements()
conda_requirements = self.base_conda_requirements
pip_requirements = [
'recommonmark',
'mkdocs',
]

args_pip = [
'python',
mock.ANY, # pip path
'install',
'-U',
'--cache-dir',
mock.ANY, # cache path
]
args_pip.extend(pip_requirements)

args_conda = [
'conda',
'install',
'--yes',
'--name',
self.version_mkdocs.slug,
]
args_conda.extend(conda_requirements)

self.build_env_mock.run.assert_has_calls([
mock.call(*args_conda),
mock.call(*args_pip, bin_path=mock.ANY)
])

def test_install_user_requirements_conda(self):
python_env = Conda(
version=self.version_sphinx,
build_env=self.build_env_mock,
)
python_env.install_user_requirements()
self.build_env_mock.run.assert_not_called()


class AutoWipeEnvironmentBase(object):
fixtures = ['test_data']
build_env_class = None
Expand Down