Skip to content

Docs now build successfully out of the box on macOS CPython. #516

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

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

tmontes
Copy link
Contributor

@tmontes tmontes commented Jan 11, 2019

Exploring possibilities of collaboration, a following up on this conversation at #470, I found out that I could not build the documentation on my laptop, running macOS with vanilla installations of CPython 3.6 and 3.7 from www.python.org.

The failure is in the docs/jsonschema_role.py Sphix extension, in the fetch_or_load function, which uses urllib.request.urlopen with an HTTPS URL. Unfortunately, such CPython installs fail to validate server-side TLS certificates.

The motive stems from the fact that, as stated in the installer's README, a private OpenSSL copy is included in the distribution which does not use the system certificate store for validation; instead, the suggested approach is doing a system-wide installation of the certifi third-party package and then creating a symlink from a file in that package to a file in the standard library (argh!).

Since I explicitly avoid installing any system-wide Python packages -- and, thus, completing the above mentioned install -- I often need to find ways to make things work. I believe many others experience similar failures due to a) actively understanding the issue and not installing system-wide things (like me), or b) not being aware of the issue and not reading the installer README where a possible solution is documented and suggested (many/most people, I'd venture).

However urllib.request.urlopen can be explicitly given a cafile argument which, again explicitly in the context of jsonschema, can use certifi's CA list. This PR adds certifi as a requirement to build docs and adjusts the urlopen call such that is uses certifi's CA list.

With this in place, the docs can be build on any platform, regardless of the (unfortunately somewhat sad) state of the underlying Python's TLS certificate validation mechanism / dependencies.

Feedback is welcome, of course.
Thanks.

@Julian
Copy link
Member

Julian commented Jan 11, 2019

Cool! Thanks, so two comments --

Firstly, purely on the matter of "supporting" CPython as a development platform, I don't use it -- I'm totally happy to "support" it (i.e. guarantee that this and related stuff "works" when developing jsonschema), but would love for that to be tested, to ensure it doesn't break, especially given that it's not an environment I'm likely to test under locally. Long story short, I think for this, it'd be nice to add building the docs under CPython to the tox.ini if that's a thing that we want to make sure always works.

I suspect that making the tox.ini build the docs on multiple interpreters will clutter up this PR a bit, so to me probably perfectly reasonable to do it "right after" this one, but yeah lemme know if that makes sense to you. I don't for example necessarily think we need to support building the docs (or development in general) on multiple versions of Python (beyond PyPy2.latest, PyPy3.latest, CPython2.latest, Cpython 3.latest).

Overall... Obviously I get queazy around security things :)

Everything you said makes total sense, but I wonder whether it's even safer to just decide to use requests there instead? We're anyways adding a doc-dep.

@tmontes
Copy link
Contributor Author

tmontes commented Jan 11, 2019

Firstly, purely on the matter of "supporting" CPython as a development platform, I don't use it -- I'm totally happy to "support" it (i.e. guarantee that this and related stuff "works" when developing jsonschema), but would love for that to be tested, to ensure it doesn't break, especially given that it's not an environment I'm likely to test under locally.

Out of curiosity, if you don't develop on CPython, which interpreter(s) do you use, pypy/pypy3? Or do you mean you don't develop on CPython on macOS?

Long story short, I think for this, it'd be nice to add building the docs under CPython to the tox.ini if that's a thing that we want to make sure always works.

That makes sense, yes. For now, like I commented yesterday, the environment I prepared is not using tox so I just built the docs manually with cd docs && pip install -r requirements.txt && make html.

I suspect that making the tox.ini build the docs on multiple interpreters will clutter up this PR a bit, so to me probably perfectly reasonable to do it "right after" this one, but yeah lemme know if that makes sense to you. I don't for example necessarily think we need to support building the docs (or development in general) on multiple versions of Python (beyond PyPy2.latest, PyPy3.latest, CPython2.latest, Cpython 3.latest).

Those platforms make sense, yes. Even a 3-only subset would make sense, I'd say, regarding "developing documentation", but maybe that's a stretch...

Overall... Obviously I get queazy around security things :)

Everything you said makes total sense, but I wonder whether it's even safer to just decide to use requests there instead? We're anyways adding a doc-dep.

I gave that a quick thought and said to myself "urlopen is good enough, no need for more dependencies, even if doc-building only". Less is more, I said. :)

@tmontes
Copy link
Contributor Author

tmontes commented Jan 11, 2019

All AppVeyor and Travis CI jobs failed... :/

  • Most due to the fact that test jsonschema.tests.test_jsonschema_test_suite.TestDraft7.test_time_validation_of_time_strings_a_valid_time_string is failing...
  • The pypy one on Travis here failed for a different motive.

Does this make any sense to you?

@Julian
Copy link
Member

Julian commented Jan 11, 2019 via email

@Julian
Copy link
Member

Julian commented Jan 11, 2019

Uh sorry and as for

The pypy one on Travis here failed for a different motive.

That is pyga/ebb-lint#13 which I now have admin rights to so have merged the PR, but in the process of getting access to PyPI to do a release to fix it. I hope that'll happen in the next few days, but if not we can pin intervaltree<3 to fix it temporarily.

@codecov
Copy link

codecov bot commented Jan 14, 2019

Codecov Report

Merging #516 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #516   +/-   ##
=======================================
  Coverage   95.68%   95.68%           
=======================================
  Files          19       19           
  Lines        2364     2364           
  Branches      308      308           
=======================================
  Hits         2262     2262           
  Misses         89       89           
  Partials       13       13

@Julian
Copy link
Member

Julian commented Jan 21, 2019

Out of curiosity, if you don't develop on CPython, which interpreter(s) do you use, pypy/pypy3? Or do you mean you don't develop on CPython on macOS?

pypy yeah. I haven't particularly used CPython for anything other than "check what CPython does on this code" in quite a number of years now, so while of course it's supported for end-users (who haven't learned yet to use PyPy :) I don't generally deal with it on a day to day basis.

And this is green now with the two fixes, so think that's a good to merge then, though yeah lemme know if you can add the tox.ini things.

@Julian Julian merged commit ae6fe2a into python-jsonschema:master Jan 21, 2019
@tmontes tmontes deleted the macos-build-docs branch March 1, 2019 11:33
Julian added a commit that referenced this pull request Oct 7, 2021
54440eab4 Merge pull request #516 from ChALkeR/chalker/ipv6
e7b22e1c6 Fix the sanity check by pinning.
8891d8107 Merge pull request #519 from json-schema-org/ether/custom-dialect
5f5fccda3 test the format-assertion vocabulary with a custom metaschema
3fcee3868 Merge pull request #512 from json-schema-org/ether/formats-and-non-strings
b349b8797 test that format-assertions are valid with non-string types
8e5b2f10d fix needless inconsistencies in format tests between drafts
02d7cb59a Correct "ref with sibling id" tests
1649470ba More ipv6 tests to increase coverage
7334b4c7e Merge pull request #505 from ChALkeR/chalker/fix-unicode
0fb2d2787 Consolidate optional/unicode into optional/ecmascript-regex
4f8c6d7bf unevaluatedProperties: deep dynamic + refs
9103f3b6f $ref wit id does not test what it is indented to do
f300dd15f Add test "same $anchor with different base uri"
d128f9d7f Add test to check that $id resolved against nearest parent, not just immediate parent
72e31dd20 Merge pull request #515 from json-schema-org/ether/fix-mandatory-format-tests
0173a0835 Revert "by default, "format" only annotates, not validates"
66e813a90 Merge pull request #506 from json-schema-org/ether/formats-non-ascii
9430972bc fix unicode tests in accordance to pattern/patternProperties spec

git-subtree-dir: json
git-subtree-split: 54440eab4d50b80a62cc9f9c561e306cdbb19591
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.

2 participants