Implement extended install option #4740

stsewd · 2018-10-08T21:51:07Z

This implements the schema change of #4732

Ref #4354

TODO

stsewd · 2018-10-10T21:33:30Z

I think this is ready for review, I have tested it with some projects locally.

The got a little big, but mostly because of the tests. What I did:

Change the config module to validate the new spec
I'm converting the list of python.install to a dictionary with its indices as keys, that way is easy to handle with the current strict validation (pop_config).
An installation for a requirements file is always created for v1 (with requirements = None), to keep compatibility with finding a requirements file.
Change the rtd code to handle several installations of a requirements form (from a path or file)
PythonEnvironment now has only one entry point to install things (previously install_user_requirements and install_package).
The order of the list is respected when installing packages.
Conda is still not installing from a requirements file (any reason for this? Should we allow conda folks installing from a requirements file in addition to the environment.yml file? Is that necessary?)
Installations for pip and setup.py, now accept a path, which is relative to the root of the project. For pip this is a special case, as pip needs to differentiate from a regular package and a path, for that we need to prefix ./ to path (except to .).
And of course, there are new tests

stsewd · 2018-10-10T21:37:53Z

readthedocs/config/models.py

@@ -11,11 +11,24 @@
    'Python',
    [
        'version',
+        'install',


This is a list, not completely satisfy with this name, any suggestion for rename or if it's fine to let that name are welcome p:

stsewd · 2018-10-10T21:38:35Z

readthedocs/config/config.py

+            if raw_install:
+                # Transform to a dict, so it's easy to validate extra keys.
+                self.raw_config.setdefault('python', {})['install'] = (
+                    _list_to_dict(raw_install)


^ what my comment says

I think it's not a good idea to change the raw_config object. By doing this, it won't be "raw" anymore and it will bring potential confusions.

I understand that the raw_config is the user's YAML parsed into a Python object (without being touched by us).

Also, checking the validate_python_install I think this can be accessed by an index instead of a key without need to modify the original config object.

We can't access by the index because of pop_config, it needs to pop recursively, and we need to keep the track of the index (if index 3 is deleted, the index 4 passes to be 3, we don't want that). I guess I could create a copy.

OK. I see what you mean.

I don't really like the solution because it modifies the original structure and it's confusing to me but on the other hand, I don't have a better idea in my mind at the moment :(

stsewd · 2018-10-11T17:00:10Z

readthedocs/doc_builder/python_environments.py

@@ -63,13 +68,27 @@ def delete_existing_venv_dir(self):
            ))
            shutil.rmtree(venv_dir)

-    def install_package(self):
-        if (self.config.python.install_with_pip or
-                getattr(settings, 'USE_PIP_INSTALL', False)):


I almost forgot I removed this (it wasn't correctly setup there anyway). We have the config file v1 and v2 to install using pip, not sure if this is used, we can remove this (there are some docs that need to be updated too)

It's not clear why this can be removed, could you expand on this?

We have https://docs.readthedocs.io/en/latest/settings.html#use-pip-install, but the logic wasn't correct, I mean, I think this was used when we didn't have a config object, and this will change the behavior of the config from the web Install project using setup to use pip instead. So, this will invert that setting, but now we have the two settings, install with pip or setup, so, we can't guess or rely on install_with_setup or whatever to invert the meaning.

I think the logic before the refactor to support v2 was correct and was inverting the behavior if the user has the config from the web marked as install with setup that should work.

This looks like something not so useful now, and if someone was relying on it it was only on a custom installation. If you think this is still useful and the logic should be fixed, I can check for this flag when setting the defaults (here we can distingue the configs from the web)

I don't think we need the setting, we're not using this in production.

Regarding the database setting, I think we should be using this. As far as I can tell we aren't using that in this PR?

We are passig the value as default for v1 here https://github.com/rtfd/readthedocs.org/blob/4031b9078efba9948301e89c948a236671086d26/readthedocs/doc_builder/config.py#L49-L49

Ans v1 is setting that value here

https://github.com/rtfd/readthedocs.org/blob/4031b9078efba9948301e89c948a236671086d26/readthedocs/config/config.py#L423-L432

codecov · 2018-10-24T19:19:00Z

Codecov Report

Merging #4740 into master will decrease coverage by 0.07%.
The diff coverage is 95.61%.

@@            Coverage Diff             @@
##           master    #4740      +/-   ##
==========================================
- Coverage   77.01%   76.93%   -0.08%     
==========================================
  Files         158      159       +1     
  Lines       10082    10004      -78     
  Branches     1267     1257      -10     
==========================================
- Hits         7765     7697      -68     
+ Misses       1988     1973      -15     
- Partials      329      334       +5

Impacted Files	Coverage Δ
readthedocs/projects/tasks.py	`71.34% <0%> (+0.2%)`	⬆️
readthedocs/config/models.py	`100% <100%> (ø)`	⬆️
readthedocs/config/utils.py	`81.81% <81.81%> (ø)`
readthedocs/doc_builder/python_environments.py	`84.41% <94.73%> (+0.96%)`	⬆️
readthedocs/config/config.py	`98.52% <98.33%> (-0.11%)`	⬇️
readthedocs/vcs_support/backends/svn.py	`29.5% <0%> (-9.89%)`	⬇️
readthedocs/core/utils/__init__.py	`74.73% <0%> (-5.68%)`	⬇️
readthedocs/vcs_support/backends/hg.py	`64.51% <0%> (-3.18%)`	⬇️
readthedocs/notifications/storages.py	`82.53% <0%> (-3.18%)`	⬇️
readthedocs/projects/querysets.py	`80.58% <0%> (-1.07%)`	⬇️
... and 30 more

readthedocs/config/config.py

humitos

This code looks good to me and I think we can merge it.

Although, I'd like to have another member of the @rtfd/core to review it since its a good amount of code and having more eyes on it would be good.

Also, I left some opinions on things that I don't like too much and suggestions to improve the code but I don't think they are 100% strict to be done right now.

stsewd · 2019-01-08T01:44:14Z

There is only one thing missing discussing here

Conda is still not installing from a requirements file (any reason for this? Should we allow conda folks installing from a requirements file in addition to the environment.yml file? Is that necessary?)

Issue #2776

Actually we could just address this later, but just in case.

ericholscher

This looks good to me. I don't fully follow the validation changes, but the tests seem to prove it works :)

ericholscher · 2019-01-15T15:47:14Z

readthedocs/config/config.py

+        with self.catch_validation_error(key):
+            validate_dict(raw_install)
+
+        if 'requirements' in raw_install:


We could likely use constants for requirements and path, but not worth blocking this PR on.

stsewd · 2019-01-15T17:38:38Z

Can I merge this after today's deploy?

ericholscher · 2019-01-22T13:50:28Z

@stsewd I'm +1 on merging, after fixing conflicts.

stsewd changed the title ~~Implement extend install option~~ Implement extended install option Oct 8, 2018

stsewd added the PR: work in progress Pull request is not ready for full review label Oct 8, 2018

stsewd added 18 commits October 10, 2018 16:20

Extend validation for config.install

09c0c87

Use new models

c817d4b

Fix tests for v1

dae2c8b

More checks

fb25527

Fix tests for v2

f98103f

More tests

258dff3

Always install from requirements file on v1

8974822

Always use absolute path

74f8470

Refactor rtd code to use multiple installs

bb81a7b

Fix tests

6897f07

Fix tests from config_integration

2987a33

Fix test for doc_building

4b7ca7f

Fix linter

3fb5964

New test for multiple installs

789dff3

Install from local path

058fadd

Refactoring and docstrings

46c2ffb

Move comment

39d6e3b

Mores tests

f99afe4

stsewd force-pushed the implement-extend-install-option branch from 64d39db to f99afe4 Compare October 10, 2018 21:34

stsewd commented Oct 10, 2018

View reviewed changes

stsewd removed the PR: work in progress Pull request is not ready for full review label Oct 10, 2018

stsewd commented Oct 11, 2018

View reviewed changes

This was referenced Oct 18, 2018

Implement pipfile support #4783

Closed

readthedocs.yaml not respecting requirements_file directive #4807

Closed

Merge branch 'master' into implement-extend-install-option

d45aa02

stsewd added 7 commits December 5, 2018 12:11

Merge branch 'master' into implement-extend-install-option

88581af

Move function to utils

a6b7c97

Add explanation about using __slots__

d194198

Docstrings

b555259

Better tests

9c677a4

Don't return

4159fed

Don't touch original config

808e83b

stsewd requested a review from a team December 5, 2018 19:15

humitos reviewed Dec 6, 2018

View reviewed changes

readthedocs/config/config.py Show resolved Hide resolved

humitos approved these changes Dec 6, 2018

View reviewed changes

stsewd mentioned this pull request Dec 6, 2018

Make raw_config private #4966

Closed

Better docstrings and comments

0df97dc

This was referenced Dec 12, 2018

Unclean package installation when virtualenv option is enabled #4968

Closed

Failing to build from mkdocs repository though layout respected #4974

Closed

stsewd requested a review from agjohnson January 7, 2019 17:19

This was referenced Jan 10, 2019

Support Pipfile #3181

Closed

Config file v2 docs #4451

Merged

humitos requested a review from a team January 14, 2019 22:13

ericholscher approved these changes Jan 15, 2019

View reviewed changes

stsewd added 3 commits January 15, 2019 12:41

Merge branch 'master' into implement-extend-install-option

823a9dc

Merge branch 'master' into implement-extend-install-option

b2939b4

Fix merge

0345845

Merge branch 'master' into implement-extend-install-option

296e365

stsewd merged commit fd28e09 into readthedocs:master Jan 22, 2019

stsewd deleted the implement-extend-install-option branch January 22, 2019 16:51

stsewd mentioned this pull request Jan 22, 2019

Consider adding support to setup.py path #4354

Closed

stsewd mentioned this pull request Feb 21, 2019

Remove unused setting #5336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement extended install option #4740

Implement extended install option #4740

stsewd commented Oct 8, 2018 •

edited

Loading

stsewd commented Oct 10, 2018

stsewd Oct 10, 2018

stsewd Oct 10, 2018

humitos Dec 3, 2018

humitos Dec 3, 2018

stsewd Dec 5, 2018

humitos Dec 6, 2018

stsewd Oct 11, 2018

agjohnson Nov 15, 2018

stsewd Nov 15, 2018

agjohnson Nov 19, 2018

stsewd Nov 19, 2018

codecov bot commented Oct 24, 2018 •

edited

Loading

humitos left a comment

stsewd commented Jan 8, 2019

ericholscher left a comment

ericholscher Jan 15, 2019

stsewd commented Jan 15, 2019

ericholscher commented Jan 22, 2019

Implement extended install option #4740

Implement extended install option #4740

Conversation

stsewd commented Oct 8, 2018 • edited Loading

TODO

stsewd commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2018 • edited Loading

Codecov Report

humitos left a comment

Choose a reason for hiding this comment

stsewd commented Jan 8, 2019

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Jan 15, 2019

ericholscher commented Jan 22, 2019

stsewd commented Oct 8, 2018 •

edited

Loading

codecov bot commented Oct 24, 2018 •

edited

Loading