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

Implement extended install option #4740

Merged
merged 35 commits into from Jan 22, 2019

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 8, 2018

This implements the schema change of #4732

Ref #4354

TODO

  • Implement new schema
  • Fix tests for new schema
  • New tests for new schema
  • Change rtd code
  • Fix rtd tests
  • Add new tests
  • Docs & minor refactors
@stsewd stsewd changed the title Implement extend install option Implement extended install option Oct 8, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 10, 2018

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 stsewd force-pushed the implement-extend-install-option branch from 64d39db to f99afe4 Oct 10, 2018
@@ -11,11 +11,24 @@
'Python',
[
'version',
'install',
Copy link
Member Author

@stsewd stsewd Oct 10, 2018

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:

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)
Copy link
Member Author

@stsewd stsewd Oct 10, 2018

^ what my comment says

Copy link
Member

@humitos humitos Dec 3, 2018

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).

Copy link
Member

@humitos humitos Dec 3, 2018

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.

Copy link
Member Author

@stsewd stsewd Dec 5, 2018

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.

Copy link
Member

@humitos humitos Dec 6, 2018

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 :(

@@ -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)):
Copy link
Member Author

@stsewd stsewd Oct 11, 2018

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)

Copy link
Contributor

@agjohnson agjohnson Nov 15, 2018

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

Copy link
Member Author

@stsewd stsewd Nov 15, 2018

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)

Copy link
Contributor

@agjohnson agjohnson Nov 19, 2018

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?

@codecov
Copy link

@codecov codecov bot commented Oct 24, 2018

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

@stsewd stsewd requested a review from Dec 5, 2018
humitos
humitos approved these changes Dec 6, 2018
Copy link
Member

@humitos humitos left a comment

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 stsewd mentioned this pull request Dec 6, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Jan 8, 2019

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.

@stsewd stsewd mentioned this pull request Jan 10, 2019
@stsewd stsewd mentioned this pull request Jan 14, 2019
@humitos humitos requested a review from Jan 14, 2019
Copy link
Member

@ericholscher ericholscher left a comment

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

with self.catch_validation_error(key):
validate_dict(raw_install)

if 'requirements' in raw_install:
Copy link
Member

@ericholscher ericholscher Jan 15, 2019

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

@stsewd
Copy link
Member Author

@stsewd stsewd commented Jan 15, 2019

Can I merge this after today's deploy?

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 22, 2019

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

@stsewd stsewd merged commit fd28e09 into readthedocs:master Jan 22, 2019
1 check passed
@stsewd stsewd deleted the implement-extend-install-option branch Jan 22, 2019
@stsewd stsewd mentioned this pull request Feb 21, 2019
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

4 participants