freedomofpress / sunder

Sunder is a user-friendly graphical interface for Shamir's Secret Sharing.
https://sunder.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
160 stars 13 forks source link

added doc-lint to make test #135

Closed abhn closed 6 years ago

abhn commented 6 years ago

Status

Ready for review

Description of Changes

Fixes #119 Doc-linting along with running tests in CI

Testing

Running make test now runs docs-lint as part of the testing process and fails if there's any error in the compilation of the docs. This will enable automatically docs linting by CI when PR is raised

liliakai commented 6 years ago

Good idea, but I hit some snags in testing:

> make docs-lint
mkdir -p docs/_static docs/_build
make -C docs/ clean
/usr/bin/python: No module named sphinx
make[1]: *** [clean] Error 1
make: *** [docs-clean] Error 2

So, if we do this we probably need to add a note somewhere about installing sphinx. But then, when I tried to install python dependencies from requirements.txt, it failed with an SSL warning:

 Could not fetch URL https://pypi.python.org/simple/sphinx/: There was a problem confirming the ssl certificate: [SSL: TLSV1_ALERT_PROTOCOL_VERSION] tlsv1 alert protocol version (_ssl.c:590) - skipping

🙀

(This was on osx, ymmv)

abhn commented 6 years ago

Aah.

We can add the Sphinx requirements part in the testing section of README.md

It went all okay when I tried pip install -r requirements.txt on my machine (Arch). I found a couple of links on searching (https://serverfault.com/questions/194538/sphinx-on-osx-building-on-osx and https://stackoverflow.com/questions/36137496/os-x-install-of-sphinx-the-sphinx-build-and-sphinx-quickstart-not-found), but not sure how relevant are they.

How do we go about making this right, since I don't own an OSX device? Any ideas?

liliakai commented 6 years ago

Found an explaination here:

https://pyfound.blogspot.com/2017/01/time-to-upgrade-your-python-tls-v12.html

tldr: as of June 2018 (this month), the "system Python" on osx can't pip install things (because TLS...). Instead, osx users must independently install a newer version of python and pip, for instance by using Homebrew:

brew install python@2
pip2 install -r requirements

We should certainly add a note about these osx-specific steps to the readme, but let's also consider the casual contributor who just wants to run tests...

Is it possible to have the docs-related make targets bail gracefully if the required python modules aren't found?

Or, perhaps a better solution is to modify circle.yml, as suggested in #119, adding a make docs-lint command there instead of modifying the behavior of make test.

conorsch commented 6 years ago

perhaps a better solution is to modify circle.yml, as suggested in #119, adding a make docs-lint command there instead of modifying the behavior of make test.

Agreed with @liliakai — that would provide a more straightforward experience for devs, while still providing feedback on docs linting errors on PRs. Would you mind revising this PR to accommodate, @abhn ?

abhn commented 6 years ago

perhaps a better solution is to modify circle.yml, as suggested in #119, adding a make docs-lint command there instead of modifying the behavior of make test.

That would be a more apt fix to this.

Would you mind revising this PR to accommodate, @abhn

Sure, I'll get that done.

abhn commented 6 years ago

Made the changes. Now the make test and make docs-lint will run in CI when PR is raised, but a tester who wishes to just test the code can do so with make test.

I hope this fixes it. Apologies for the bad fix.