Skip to content

Conversation

Mariatta
Copy link
Member

No description provided.

@mention-bot
Copy link

@Mariatta, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @benjaminp and @tiran to be potential reviewers.

Doc/Makefile Outdated
@@ -107,7 +107,7 @@ clean:

venv:
$(PYTHON) -m venv venv
./venv/bin/python3 -m pip install -U Sphinx
./venv/bin/python3 -m pip install sphinx~=1.5.6
Copy link
Member

Choose a reason for hiding this comment

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

I think, we depend upon the latest version intentionally.

So, after this lands, #1613 can be merged and we can revert back to using the latest version?

Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to specify the version in .travis.yml and leave the Makefile alone. My thinking is that locking Travis for testing makes sure it stays green and it's obvious what we are requiring for CI to pass, but leaving the Makefile alone makes it easier to update the docs to the latest version of Sphinx offline.

Python 2.7 also doesn't have the venv command in the Makefile but it does have the explicit pip call in .travis.yml.

Or maybe I'm just nuts and that logic is flawed. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Keeping version details out of the Makefile sounds like a better option to me. Makefiles can increase the complexity of its own :)

@brettcannon
Copy link
Member

brettcannon commented May 16, 2017

I have edited Mariatta's branch to show what I'm suggesting for editing .travis.yml so we can look at the options side-by-side (I hope that's okay, @Mariatta , and if it's not I apologize as I'm not trying to hijack this PR, just to resolve this quickly so we can start clicking "restart build" in Travis a bunch of times 😉 ).

@brettcannon
Copy link
Member

Travis passed for both approaches so they both solve the problem, so either solution will work.

@Mariatta
Copy link
Member Author

No that's ok 😄 Whatever's needed to get this fixed soon. I'm at the airport so if it looks good then please merge 😃

@brettcannon brettcannon changed the title bpo-30380: use sphinx~=1.5.6 bpo-30380: Pin the version of Sphinx used to build the documentation May 16, 2017
@brettcannon
Copy link
Member

brettcannon commented May 16, 2017

It looks like @serhiy-storchaka has fixed the 1.6.1-specific issues for at least master and is in the process of backporting the changes, so I'm re-pinning to 1.6.1 in master and then will pin to 1.5.6 in the older branches while he fixes his cherry-pick PRs.

@Mariatta
Copy link
Member Author

Thanks @brettcannon and @serhiy-storchaka for actually taking care of this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants