| Message ID | 20170102114742.13949-4-stephen@that.guru |
|---|---|
| State | RFC |
| Headers | show |
On Mon, Jan 02, 2017 at 11:47:41AM +0000, Stephen Finucane wrote: > If someone makes changes to documentation or Python scripts, they should > validate these changes using the relevant targets. However, said targets > use optional dependencies and are not guaranteed to be enabled. Enforce > running of these checks whenever changes are made to select files, > ensuring the user has installed the changes. > > Signed-off-by: Stephen Finucane <stephen@that.guru> This always fails for me, because I don't have a branch named "master". (I also don't have a remote named "origin".) Also, once we branch for 2.7, "master" will not always be the correct branch to check. I like the idea here, but I don't think that this implementation is quite right, and it seems like it's hard to get right in general. I don't have a good suggestion about how to make it better. For check-docs in particular, it seems reasonable to always just say that the docs can't be checked, if sphinx is not installed.
On Wed, 2017-01-04 at 08:20 -0800, Ben Pfaff wrote: > On Mon, Jan 02, 2017 at 11:47:41AM +0000, Stephen Finucane wrote: > > If someone makes changes to documentation or Python scripts, they > > should > > validate these changes using the relevant targets. However, said > > targets > > use optional dependencies and are not guaranteed to be enabled. > > Enforce > > running of these checks whenever changes are made to select files, > > ensuring the user has installed the changes. > > > > Signed-off-by: Stephen Finucane <stephen@that.guru> > > This always fails for me, because I don't have a branch named > "master". > (I also don't have a remote named "origin".) > > Also, once we branch for 2.7, "master" will not always be the correct > branch to check. > > I like the idea here, but I don't think that this implementation is > quite right, and it seems like it's hard to get right in general. I > don't have a good suggestion about how to make it better. > > For check-docs in particular, it seems reasonable to always just say > that the docs can't be checked, if sphinx is not installed. That's fair - this should probably have been an RFC as it is certainly a little...hacky. Feel free to drop from the series. As an alternative approach, I wonder could we install Sphinx in Travis so that it will run 'check-docs' for us? Stephen
On Wed, Jan 04, 2017 at 04:25:40PM +0000, Stephen Finucane wrote: > On Wed, 2017-01-04 at 08:20 -0800, Ben Pfaff wrote: > > On Mon, Jan 02, 2017 at 11:47:41AM +0000, Stephen Finucane wrote: > > > If someone makes changes to documentation or Python scripts, they > > > should > > > validate these changes using the relevant targets. However, said > > > targets > > > use optional dependencies and are not guaranteed to be enabled. > > > Enforce > > > running of these checks whenever changes are made to select files, > > > ensuring the user has installed the changes. > > > > > > Signed-off-by: Stephen Finucane <stephen@that.guru> > > > > This always fails for me, because I don't have a branch named > > "master". > > (I also don't have a remote named "origin".) > > > > Also, once we branch for 2.7, "master" will not always be the correct > > branch to check. > > > > I like the idea here, but I don't think that this implementation is > > quite right, and it seems like it's hard to get right in general. I > > don't have a good suggestion about how to make it better. > > > > For check-docs in particular, it seems reasonable to always just say > > that the docs can't be checked, if sphinx is not installed. > > That's fair - this should probably have been an RFC as it is certainly > a little...hacky. Feel free to drop from the series. > > As an alternative approach, I wonder could we install Sphinx in Travis > so that it will run 'check-docs' for us? Sounds great to me. I guess this is a matter of adding a few lines to .travis.yml?
On Wed, 2017-01-04 at 08:34 -0800, Ben Pfaff wrote: > On Wed, Jan 04, 2017 at 04:25:40PM +0000, Stephen Finucane wrote: > > On Wed, 2017-01-04 at 08:20 -0800, Ben Pfaff wrote: > > > On Mon, Jan 02, 2017 at 11:47:41AM +0000, Stephen Finucane wrote: > > > > If someone makes changes to documentation or Python scripts, > > > > they > > > > should > > > > validate these changes using the relevant targets. However, > > > > said > > > > targets > > > > use optional dependencies and are not guaranteed to be enabled. > > > > Enforce > > > > running of these checks whenever changes are made to select > > > > files, > > > > ensuring the user has installed the changes. > > > > > > > > Signed-off-by: Stephen Finucane <stephen@that.guru> > > > > > > This always fails for me, because I don't have a branch named > > > "master". > > > (I also don't have a remote named "origin".) > > > > > > Also, once we branch for 2.7, "master" will not always be the > > > correct > > > branch to check. > > > > > > I like the idea here, but I don't think that this implementation > > > is > > > quite right, and it seems like it's hard to get right in > > > general. I > > > don't have a good suggestion about how to make it better. > > > > > > For check-docs in particular, it seems reasonable to always just > > > say > > > that the docs can't be checked, if sphinx is not installed. > > > > That's fair - this should probably have been an RFC as it is > > certainly > > a little...hacky. Feel free to drop from the series. > > > > As an alternative approach, I wonder could we install Sphinx in > > Travis > > so that it will run 'check-docs' for us? > > Sounds great to me. I guess this is a matter of adding a few lines > to > .travis.yml? Aye. That would ensure we get a big red warning when someone breaks the doc build. Patch incoming. Stephen
diff --git a/Documentation/automake.mk b/Documentation/automake.mk index 51abd55..aac21d5 100644 --- a/Documentation/automake.mk +++ b/Documentation/automake.mk @@ -103,13 +103,17 @@ htmldocs: $(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(SPHINXBUILDDIR)/html ALL_LOCAL += htmldocs -check-docs: - $(SPHINXBUILD) -b linkcheck $(ALLSPHINXOPTS) $(SPHINXBUILDDIR)/linkcheck - clean-docs: rm -rf $(SPHINXBUILDDIR)/* CLEAN_LOCAL += clean-docs endif .PHONY: htmldocs -.PHONY: check-docs .PHONY: clean-docs + +check-docs: +if HAVE_SPHINX + $(SPHINXBUILD) -b linkcheck $(ALLSPHINXOPTS) $(SPHINXBUILDDIR)/linkcheck +else + $(srcdir)/build-aux/check-file-changes "Documentation/" "sphinx" +endif +.PHONY: check-docs diff --git a/Makefile.am b/Makefile.am index 74839e1..7969fd1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -79,6 +79,7 @@ EXTRA_DIST = \ build-aux/cccl \ build-aux/cksum-schema-check \ build-aux/calculate-schema-cksum \ + build-aux/check-file-changes \ build-aux/dist-docs \ build-aux/sodepends.pl \ build-aux/soexpand.pl \ @@ -319,7 +320,6 @@ manpage-check: $(man_MANS) $(dist_man_MANS) $(noinst_man_MANS) CLEANFILES += manpage-check endif -if HAVE_FLAKE8 ALL_LOCAL += flake8-check # http://flake8.readthedocs.org/en/latest/warnings.html # All warnings explicitly selected or ignored should be listed below. @@ -343,9 +343,12 @@ ALL_LOCAL += flake8-check # H233 Python 3.x incompatible use of print operator # H238 old style class declaration, use new style (inherit from `object`) flake8-check: $(FLAKE8_PYFILES) +if HAVE_FLAKE8 $(AM_V_GEN) if flake8 $^ --select=H231,H232,H233,H238 ${FLAKE8_FLAGS} && \ flake8 $^ --ignore=E121,E123,E125,E126,E127,E128,E129,E131,W503,F811,D,H ${FLAKE8_FLAGS}; then \ touch $@; else exit 1; fi +else + $(srcdir)/build-aux/check-file-changes "python/" "flake8" endif CLEANFILES += flake8-check diff --git a/build-aux/check-file-changes b/build-aux/check-file-changes new file mode 100755 index 0000000..5c0b809 --- /dev/null +++ b/build-aux/check-file-changes @@ -0,0 +1,19 @@ +#!/usr/bin/env bash + +directory="$1" +dependency="$2" + +# ensure git is actually installed +if ! type "git" > /dev/null; then + printf "git is not available - skipping check-file-changes" + exit 0 +fi + +file_changes=$(git diff --name-only master..HEAD "$directory") + +if [ -n "$file_changes" ]; then + printf "There are changes in the '$directory' directory, but the " + printf "'$dependency' dependency is missing. Install missing dependencies " + printf "before continuing.\n" + exit 1 +fi
If someone makes changes to documentation or Python scripts, they should validate these changes using the relevant targets. However, said targets use optional dependencies and are not guaranteed to be enabled. Enforce running of these checks whenever changes are made to select files, ensuring the user has installed the changes. Signed-off-by: Stephen Finucane <stephen@that.guru> --- Documentation/automake.mk | 12 ++++++++---- Makefile.am | 5 ++++- build-aux/check-file-changes | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 5 deletions(-) create mode 100755 build-aux/check-file-changes