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

Alter order of processing dependencies #1364

Closed
NYKevin opened this issue Jun 22, 2015 · 10 comments
Closed

Alter order of processing dependencies #1364

NYKevin opened this issue Jun 22, 2015 · 10 comments
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Milestone

Comments

@NYKevin
Copy link

NYKevin commented Jun 22, 2015

See this build, and in particular, note this information:

Collecting alabaster<0.8,>=0.7 (from sphinx==1.3.1)
  Using cached alabaster-0.7.5.tar.gz

My documentation does not build with Alabaster 0.7.5 because of #1350. But my requirements explicitly specify 0.7.4:

Collecting alabaster==0.7.4 (from -r doc_reqs.txt (line 1))

Read The Docs, ideally, should not install things the user did not ask for. I asked for 0.7.4, so I should get 0.7.4, not some other version. These docs build fine locally, because I have the right version of Alabaster. I imagine this problem affects everyone on Python 3.

According to the output, this is likely because Read The Docs is installing Sphinx into the virtualenv before it installs my requirements, so it ends up grabbing something I didn't ask for. Option one would be to install project requirements first, and the basic Sphinx etc. requirements afterwards. This is trivially accomplished; just move this function call down to here, and remove the -U flag. Since the virtualenv is created anew every time, that flag isn't doing anything anyway (Pip will already replace an existing package with a different version if asked). The downside is that this will override the versions of Sphinx etc. specified in the project's requirements, which is bad.

If we removed the version specifiers from the list here (so it's just a list of package names), that wouldn't happen, but then Read The Docs wouldn't have a consistent environment. On the other hand, the -U flag suggests you actually want that, so I'm not entirely sure what the intent here is in the first place.

As a compromise, we could change those version specifiers into a different kind of specifier, such as sphinx~=1.3.1 (basically meaning "You can specify your own version of Sphinx, but it can't be too old or too new, and we'll use the latest compatible one if you don't ask for anything," assuming we also move the call down as described above). Note that Pip never installs pre-releases unless explicitly asked with --pre, so this won't grab some randomly broken trunk snapshot by mistake.

Option two would be to check whether Sphinx (etc.) is already installed in the project requirements, and skip it if so. We can do this by calling pip show sphinx and checking for output, or grepping the output of pip freeze for packages we need. This has the downside of being significantly more involved (more code, checking the output of commands, etc.).

Going the really crazy route (option three), we could parse the user's requirements file and try to merge it with Read The Docs' requirements. I think this is a Bad Idea because it's particularly convoluted compared to the other solutions with no obvious upside. Pip already knows how to read these files. Trying to outsmart it is, I think, a poor choice.

The "quick fix" (option four?) is to specify alabaster==0.7.4 in the list, but that's #1350, not this bug, and a variation of this has already been rejected.

I originally reported this as #1359, but I badly misdiagnosed the issue and did a poor job of explaining the problem, so I'm re-reporting it with a brand-new description. I apologize if opening multiple issues is excessive in this case.

NYKevin added a commit to NYKevin/readthedocs.org that referenced this issue Jun 22, 2015
@NYKevin
Copy link
Author

NYKevin commented Jun 22, 2015

I have a pull request for option one 90% of the way done, but I can't get RTD's tests to run on my system, so I don't feel comfortable pulling the trigger on it. Please advise.

@gregmuellegger
Copy link
Contributor

I'm not sure what this change will have as implication, so someone with more experience over the builds must decide if that is a good idea. But I might be able to help with your tests.

How do you run your tests and what fails while doing so?
You should have your local virtualenv enabled and then run ./runtests.sh from the root of the checkout.

@NYKevin
Copy link
Author

NYKevin commented Jun 22, 2015

@gregmuellegger I can't even get the virtualenv set up. See this paste. I get this error in particular:

Collecting factory-boy==2.4.1 (from -r requirements/pip.txt (line 48))
  Using cached factory_boy-2.4.1.tar.gz
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 20, in <module>
      File "/tmp/pip-build-eZd5CA/factory-boy/setup.py", line 71, in <module>
        test_loader=test_loader,
      File "/usr/lib/python2.7/distutils/core.py", line 111, in setup
        _setup_distribution = dist = klass(attrs)
      File "build/bdist.linux-i686/egg/setuptools/dist.py", line 260, in __init__
      File "build/bdist.linux-i686/egg/setuptools/dist.py", line 284, in fetch_build_eggs
      File "build/bdist.linux-i686/egg/pkg_resources.py", line 569, in resolve
    pkg_resources.VersionConflict: (setuptools 0.6c11 (/home/kevin/git/readthedocs.org/venv/lib/python2.7/site-packages/setuptools-0.6c11-py2.7.egg), Requirement.parse('setuptools>=0.8'))

I forgot to do this during the paste, but the file /tmp/pip-build-eZd5CA/factory-boy does not exist.

@agjohnson agjohnson added the Needed: design decision A core team decision is required label Jun 22, 2015
@agjohnson
Copy link
Contributor

Option 1 is the only one I think we'd consider. We need to specific the version of Sphinx that we officially support, which is 1.3.1, and specific versions of several extensions and our theme. This can likely happen after per-project dependencies are installed, though there is a duplication of effort that will happen when Sphinx is installed by the project, and then upgraded to 1.3.1 if it isn't already current.

With 1.3 shipping themes as dependencies, it's probably worth re-evaluating how we are installing dependencies.

@NYKevin
Copy link
Author

NYKevin commented Jun 22, 2015

I should note that the ~= operator is perhaps questionable, depending on your design goals. Basically, sphinx~=1.3.1 expands to sphinx==1.3.* and sphinx>=1.3.1, so when 1.3.2 gets released, it would upgrade to that automatically. It will not, however, upgrade to 1.4.x+ without manual intervention, and will downgrade to 1.3.x if the user tries to explicitly specify a 1.4.x+ version in project requirements.

Another option I didn't specifically mention would be to add versions for every package which is part of the basic Sphinx+RTD requirements, including Alabaster (i.e. use the full output of pip freeze in a basic RTD environment), and continue to install the Sphinx+RTD requirements first. On one hand, this ensures we always get exactly the same basic environment to start with, so RTD's requirements (in theory) never break things by themselves. On the other hand, it means the list is longer and requires more effort to maintain. It also means potentially installing things the user didn't ask for or want, and then uninstalling them when we run through the project requirements. That means every time we bump a version number, we have to run tests to make sure the packages don't break things at install time like Alabaster 0.7.5 does.

@agjohnson agjohnson changed the title Alabaster 0.7.5 is incorrectly installed into Python 3 virtualenvs even when a different version is explicitly requested Alter order of processing dependencies Jun 22, 2015
@rbtcollins
Copy link

@NYKevin that error indicates that there is a versioned setup-requires on a newer setuptools than you have, and setuptools is bailing out (it can't handle that situation itself). upgrading your virtualenv package should get you a newer bundled setuptools, and that should get things going.

@gregmuellegger
Copy link
Contributor

@rbtcollins do you see this as related to #1428 ?

@rbtcollins
Copy link

@gregmuellegger no - I was looking for related issues, and saw that @NYKevin had gotten stuck here with something i could explain. The setuptools issue encountered is outside the venv that rtd creates during builds - its the one that NYKevin was using to test rtd, so yeah, unrelated.

@gregmuellegger gregmuellegger added the Improvement Minor improvement to code label Sep 14, 2015
@gregmuellegger gregmuellegger added this to the rtd-build milestone Sep 14, 2015
@bhearsum
Copy link

This appears to be actively breaking documentation one of my projects (https://readthedocs.org/projects/mozilla-balrog/builds/) because of the old version of setuptools that is installed prior my project's own dependencies. Other than downgrading my own dependencies, is there a way to work around this?

@agjohnson
Copy link
Contributor

We've altered how we're installing our dependencies, and have also upgraded setuptools to a more recent release. Considering this issue closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
None yet
Development

No branches or pull requests

5 participants