[PATCH v2] travis-ci: build documentation

classic Classic list List threaded Threaded
53 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Junio C Hamano
Third time's a charm, perhaps?
 
-- >8 --
Subject: [PATCH] ci: validate "gitlink:" in documentation

It is easy to add incorrect "linkgit:<page>[<section>]" references
to our documentation suite.  Catch these common classes of errors:

 * Referring to Documentation/<page>.txt that does not exist.

 * Referring to a <page> outside the Git suite.  In general, <page>
   must begin with "git".

 * Listing the manual <section> incorrectly.  The first line of the
   Documentation/<page>.txt must end with "(<section>)".

with a new script "ci/lint-gitlink", and drive it from "make check-docs".

Signed-off-by: Junio C Hamano <[hidden email]>
---
 Documentation/Makefile |  5 +++++
 Makefile               |  1 +
 ci/lint-gitlink        | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)
 create mode 100755 ci/lint-gitlink

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 3e39e28..e9cd43d 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -204,6 +204,7 @@ ifndef V
  QUIET_DBLATEX = @echo '   ' DBLATEX $@;
  QUIET_XSLTPROC = @echo '   ' XSLTPROC $@;
  QUIET_GEN = @echo '   ' GEN $@;
+ QUIET_LINT = @echo '   ' LINT $@;
  QUIET_STDERR = 2> /dev/null
  QUIET_SUBDIR0 = +@subdir=
  QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
@@ -427,4 +428,8 @@ quick-install-html: require-htmlrepo
 print-man1:
  @for i in $(MAN1_TXT); do echo $$i; done
 
+lint-docs::
+ $(QUIET_LINT)$(foreach txt,$(patsubst %.html,%.txt,$(DOC_HTML)), \
+ ../ci/lint-gitlink $(txt))
+
 .PHONY: FORCE
diff --git a/Makefile b/Makefile
index 2742a69..61bd0ab 100644
--- a/Makefile
+++ b/Makefile
@@ -2496,6 +2496,7 @@ ALL_COMMANDS += git-gui git-citool
 
 .PHONY: check-docs
 check-docs::
+ $(MAKE) -C Documentation lint-docs
  @(for v in $(ALL_COMMANDS); \
  do \
  case "$$v" in \
diff --git a/ci/lint-gitlink b/ci/lint-gitlink
new file mode 100755
index 0000000..6b6bf91
--- /dev/null
+++ b/ci/lint-gitlink
@@ -0,0 +1,56 @@
+#!/usr/bin/perl
+
+my $found_errors = 0;
+
+sub report {
+ my ($where, $what, $error) = @_;
+ print "$where: $error: $what\n";
+ $found_errors = 1;
+}
+
+sub grab_section {
+ my ($page) = @_;
+ open my $fh, "<", "$page.txt";
+ my $firstline = <$fh>;
+ chomp $firstline;
+ close $fh;
+ my ($section) = ($firstline =~ /.*\((\d)\)$/);
+ return $section;
+}
+
+sub lint {
+ my ($file) = @_;
+ open my $fh, "<", $file
+ or return;
+ while (<$fh>) {
+ my $where = "$file:$.";
+ while (s/linkgit:((.*?)\[(\d)\])//) {
+ my ($target, $page, $section) = ($1, $2, $3);
+
+ # De-AsciiDoc
+ $page =~ s/{litdd}/--/g;
+
+ if ($page !~ /^git/) {
+ report($where, $target, "nongit link");
+ next;
+ }
+ if (! -f "$page.txt") {
+ report($where, $target, "no such source");
+ next;
+ }
+ $real_section = grab_section($page);
+ if ($real_section != $section) {
+ report($where, $target,
+ "wrong section (should be $real_section)");
+ next;
+ }
+ }
+ }
+ close $fh;
+}
+
+for (@ARGV) {
+ lint($_);
+}
+
+exit $found_errors;
--
2.8.2-498-g6350fe8

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Jeff King
On Wed, May 04, 2016 at 02:34:23PM -0700, Junio C Hamano wrote:

> Third time's a charm, perhaps?
>  
> -- >8 --
> Subject: [PATCH] ci: validate "gitlink:" in documentation
>
> It is easy to add incorrect "linkgit:<page>[<section>]" references
> to our documentation suite.  Catch these common classes of errors:
>
>  * Referring to Documentation/<page>.txt that does not exist.
>
>  * Referring to a <page> outside the Git suite.  In general, <page>
>    must begin with "git".
>
>  * Listing the manual <section> incorrectly.  The first line of the
>    Documentation/<page>.txt must end with "(<section>)".
>
> with a new script "ci/lint-gitlink", and drive it from "make check-docs".
>
> Signed-off-by: Junio C Hamano <[hidden email]>
> ---

This looks good to me. Two minor nits that may not be worth addressing:

> +lint-docs::
> + $(QUIET_LINT)$(foreach txt,$(patsubst %.html,%.txt,$(DOC_HTML)), \
> + ../ci/lint-gitlink $(txt))
> +

This dependency feels funny. Wouldn't CI want to invoke this as:

  make -C Documentation lint-docs

IOW, Documentation owns the script (just like t/ owns its own lint
scripts like check-non-portable-shell.pl), and CI always just triggers
the make-driven checks, just as a normal developer would?

> +sub lint {
> + my ($file) = @_;
> + open my $fh, "<", $file
> + or return;
> + while (<$fh>) {
> + my $where = "$file:$.";
> [... actual linting of line ...]
> +}
> +
> +for (@ARGV) {
> + lint($_);
> +}

The usual perl way here would be:

  while(<>) {
        my $where = "$ARGV:$.";
        ... actual linting of line ...
  }

where "<>" automagically reads from files on the command-line or stdin
if none were specified (and sets $ARGV as appropriate). But maybe you
prefer not to handle the stdin case (it is a benefit sometimes, but an
annoyance if you accidentally end up with an empty file list).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Junio C Hamano
Jeff King <[hidden email]> writes:

> This dependency feels funny. Wouldn't CI want to invoke this as:
>
>   make -C Documentation lint-docs

I expected CI to do this instead

        make check-docs

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Ævar Arnfjörð Bjarmason
In reply to this post by Jeff King
On Wed, May 4, 2016 at 10:06 PM, Jeff King <[hidden email]> wrote:

> Would:
>
>   open(my $files, '-|', qw(git ls-files));
>   while (<$files>) {
>     chomp;
>     ...
>   }
>
> make sense? Or a simpler but non-streaming spelling:
>
>   my @files = map { chomp; $_ } `git ls-files`;

As a minor sidenote you can equivalently write that as:

    chomp(my @files = `git ls-files`);

I.e. chomp itself when given a list will chomp each item of the list.
So no need for a map.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Jeff King
In reply to this post by Junio C Hamano
On Wed, May 04, 2016 at 02:52:27PM -0700, Junio C Hamano wrote:

> Jeff King <[hidden email]> writes:
>
> > This dependency feels funny. Wouldn't CI want to invoke this as:
> >
> >   make -C Documentation lint-docs
>
> I expected CI to do this instead
>
> make check-docs

Ah, sure, that makes even more sense. But I think the point remains,
which is that your perl script is an implementation detail of the
Makefile process. I thought the "ci" directory was supposed to be for
ci-specific scripts that would be driven directly by Travis, etc.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Jeff King
In reply to this post by Ævar Arnfjörð Bjarmason
On Wed, May 04, 2016 at 11:54:52PM +0200, Ævar Arnfjörð Bjarmason wrote:

> >   my @files = map { chomp; $_ } `git ls-files`;
>
> As a minor sidenote you can equivalently write that as:
>
>     chomp(my @files = `git ls-files`);
>
> I.e. chomp itself when given a list will chomp each item of the list.
> So no need for a map.

Right, thanks, that is more readable. I use the map form reflexively
because I am often doing stuff like:

  my @list = do {
    open(my $fh, '<', $fn);
    map { chomp; $_ }
  };

(or replacing "do" with an actual function which is returning a
grotesque pipeline of grep/map, etc).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Junio C Hamano
In reply to this post by Jeff King
Jeff King <[hidden email]> writes:

> But I think the point remains,
> which is that your perl script is an implementation detail of the
> Makefile process. I thought the "ci" directory was supposed to be for
> ci-specific scripts that would be driven directly by Travis, etc.

True.  Documentation/ has tools like built-docdep.perl,
howto-index.sh, etc., so it can live next to it.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> Jeff King <[hidden email]> writes:
>
>> But I think the point remains,
>> which is that your perl script is an implementation detail of the
>> Makefile process. I thought the "ci" directory was supposed to be for
>> ci-specific scripts that would be driven directly by Travis, etc.
>
> True.  Documentation/ has tools like built-docdep.perl,
> howto-index.sh, etc., so it can live next to it.
>
> Thanks.

Gaah, the Makefile part of the final patch is wrong; we do not check
the included sources at all if we only passed the top-level targets'
sources.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> Gaah, the Makefile part of the final patch is wrong; we do not check
> the included sources at all if we only passed the top-level targets'
> sources.

I ended up queuing an enhanced version of File::Find based one on
'pu', but I won't be posting it here today.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Jeff King
On Wed, May 04, 2016 at 04:28:31PM -0700, Junio C Hamano wrote:

> Junio C Hamano <[hidden email]> writes:
>
> > Gaah, the Makefile part of the final patch is wrong; we do not check
> > the included sources at all if we only passed the top-level targets'
> > sources.
>
> I ended up queuing an enhanced version of File::Find based one on
> 'pu', but I won't be posting it here today.

Hmm. It seems like we should still be able to drive it from the Makefile
using the dependencies generated by build-docdep.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 1/2] Documentation: fix linkgit references

Jeff King
On Wed, May 04, 2016 at 07:41:53PM -0400, Jeff King wrote:

> On Wed, May 04, 2016 at 04:28:31PM -0700, Junio C Hamano wrote:
>
> > Junio C Hamano <[hidden email]> writes:
> >
> > > Gaah, the Makefile part of the final patch is wrong; we do not check
> > > the included sources at all if we only passed the top-level targets'
> > > sources.
> >
> > I ended up queuing an enhanced version of File::Find based one on
> > 'pu', but I won't be posting it here today.
>
> Hmm. It seems like we should still be able to drive it from the Makefile
> using the dependencies generated by build-docdep.

Having just read that script, it is blindly looking at "*.txt", so...as
long as that is what your script is doing (and it looks like the version
in pu is), I don't think there's any reason to get more complicated. The
only difference is that build-docdep does not even bother looking in
subdirectories (maybe it should, since we now build stuff in technical,
I think).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 2/2] travis-ci: build documentation

Junio C Hamano
In reply to this post by larsxschneider
[hidden email] writes:

> From: Lars Schneider <[hidden email]>
>
> Build documentation as separate Travis CI job to check for
> documentation errors.
>
> Signed-off-by: Lars Schneider <[hidden email]>
> ---
>  .travis.yml              | 15 +++++++++++++++
>  ci/test-documentation.sh | 14 ++++++++++++++
>  2 files changed, 29 insertions(+)
>  create mode 100755 ci/test-documentation.sh
>
> diff --git a/.travis.yml b/.travis.yml
> index 78e433b..55299bd 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -32,6 +32,21 @@ env:
>      # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
>      - GIT_SKIP_TESTS="t9810 t9816"

Completely offtopic, but this looks like this is made to apply to
all archs, not limited to OSX?  It of course would be ideal to see
why they fail only on OSX and fix them, but shouldn't the blacklist
at least limited to the platform with the problem?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v4 2/2] travis-ci: build documentation

larsxschneider

> On 10 May 2016, at 19:12, Junio C Hamano <[hidden email]> wrote:
>
> [hidden email] writes:
>
>> From: Lars Schneider <[hidden email]>
>>
>> Build documentation as separate Travis CI job to check for
>> documentation errors.
>>
>> Signed-off-by: Lars Schneider <[hidden email]>
>> ---
>> .travis.yml              | 15 +++++++++++++++
>> ci/test-documentation.sh | 14 ++++++++++++++
>> 2 files changed, 29 insertions(+)
>> create mode 100755 ci/test-documentation.sh
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 78e433b..55299bd 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -32,6 +32,21 @@ env:
>>     # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
>>     - GIT_SKIP_TESTS="t9810 t9816"
>
> Completely offtopic, but this looks like this is made to apply to
> all archs, not limited to OSX?  It of course would be ideal to see
> why they fail only on OSX and fix them, but shouldn't the blacklist
> at least limited to the platform with the problem?

As far as I remember the test was flaky on Linux and OSX. The problem
was not Git. The problem was the Perforce Server "p4d" p4 daemon which
would refuse to die sometimes.

I run a bigger number of Travis tests and it looks like the problem is
gone. Out of 60 runs only just one failed and t9810 / t9816 passed:
https://travis-ci.org/larsxschneider/git/jobs/129383851
(BTW: does anyone know how I can make prove show me the running tests?
I would like to know which test died there..)

It looks like as Perforce solved the t9810/t9816 problem with the update
from 15.2 to 16.1 that we made in 31f3c86 "travis-ci: update Git-LFS
and P4 to the latest version". I will post a patch to enable these tests,
again.

Thanks,
Lars


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123