tools/doccheck: Detect when `make doc` fails to run. by jcarrano · Pull Request #9819 · RIOT-OS/RIOT

jcarrano · 2018-08-22T13:54:30Z

The previous doccheck would give a false negative when doxygen does not even run (because of misconfiguration). Guess what? Doxygen was never running on Travis! That means our docchecks in the static tests were a lie.

Issues/PRs references

For an example of why this is a problem, see #9818.

~~Depends on #10237 (if we enable the check before fixing travis then everything will fail,)~~
Depends on ~~#10251~~ (the old version of Doxygen currently in travis reports a lot of errors).

jia200x · 2018-08-22T14:01:14Z

that was fast :)

I'm currently trying the script

jia200x · 2018-08-22T14:05:43Z

dist/tools/doccheck/check.sh

 RIOTBASE="$(cd $(dirname $0)/../../..; pwd)"

+die() {
+   echo "warning: Failed to build run doxygen"


these error messages can come from the Doxygen makefile. E.g there might be a "doxygen not found" or a "git-lfs not installed", they are 2 different errors

The idea of this PR was to catch any non-zero exit code from make doc. Maybe I can change the message to "make doc exited with non-zero code" to make it explicit.
Afterwards we could add filters to point at the cause of the problem.

sounds good

jia200x · 2018-08-22T14:06:28Z

besides the comment I just made, the script is running as expected. Great!

jia200x · 2018-08-22T14:07:03Z

(Travis fails, the error seems to be triggered there)

jcarrano · 2018-08-22T14:15:23Z

(Travis fails, the error seems to be triggered there)

Weird. It works on my machine. Could it be that make doc was failing (in travis) all this time and we never detected it?

jia200x · 2018-08-22T14:16:53Z

Weird. It works on my machine. Could it be that make doc was failing (in travis) all this time and we never detected it?

If I remember correctly I have seen Doxygen errors, not sure where (Travis CI or Murdock) . I will try the Docker image to see if it works.

aabadie · 2018-08-22T14:22:55Z

Could it be that make doc was failing (in travis) all this time

I already noticed related problems in previous PRs where Murdock was reporting doxygen errors (undocumented paramaters, etc) but not Travis. I thought that doxygen version was the culprit and didn't look at that too much.

jcarrano · 2018-10-18T09:43:49Z

@jia200x Is this PR still relevant or has it been solved in another one?

jia200x · 2018-10-18T12:27:45Z

AFAIK the issue is still present

jcarrano · 2018-10-24T12:36:35Z

Update: the problem was Travis and it's outdated lesscpy command. I did another PR to fix it.

aabadie · 2018-10-24T12:39:52Z

How about moving travis configuration to xenial (instead of trusty) ?

cladmi · 2018-10-24T12:44:03Z

@aabadie why not updating it directly to the new bionic docker image after the release when we use it in murdock ?

aabadie · 2018-10-24T12:46:30Z

why not updating it directly to the new bionic docker image after the release when we use it in murdock ?

Easy, have a look at #8729. But it requires to move to CircleCI => which provides a nice container environment.

jcarrano · 2018-10-24T12:50:16Z

Can we first update and then move to CircleCI? I'm sure it won't be without inconveniences. Then we would have the problems that come from updating ubuntu and changing CI all at the same time.

cladmi · 2018-10-24T12:51:30Z

Can we not use our image in travis ?

But sure using the latest version on travis would be better anyway. What is missing to go for it ?

aabadie · 2018-10-24T13:05:47Z

Can we not use our image in travis ?

Apparently yes, see https://docs.travis-ci.com/user/docker/
But I find it less handy than the circle ci config. Note that on CircleCI there's the possibility to store builds artifacts and this is what is used to have an overview of the generated doc in #8729

cladmi · 2018-10-24T15:08:54Z

@aabadie It is still two different tasks, use travis with an updated image even if ugly and migrating should be justified by real benefits and could be a separate thing. Fix first, migrating is another step.

Now there is a real issue with travis with the doxygen version as shown in #10237

jcarrano · 2018-10-25T10:07:25Z

@cladmi

use travis with an updated image even

~~d'oh! It seems that is not possible. @aabadie is right, we need to move out of travis, see this:~~

~~travis-ci/travis-ci#9928~~

~~Travis is stuck with Trusty. Let them rot!!!!~~

EDIT: I provided a PR to use our own image in travis.

cladmi · 2018-11-22T13:38:53Z

@jcarrano please rebase now that #10251 is merged.
You can also directly squash.

The previous doccheck would give a false negative when doxygen does not even run (for example, because of misconfiguration). Also, when doxygen fails to run, print the full output.

cladmi

I tested locally by adding a false in the make doc target.

Running make static-test correctly fails:

Running './dist/tools/doccheck/check.sh' x
Command output:

        'make doc' exited with non-zero code (2)
        make[1]: Entering directory '/home/harter/work/git/RIOT'
        "make" -BC doc/doxygen
        make[2]: Entering directory '/home/harter/work/git/RIOT/doc/doxygen'
        ( cat riot.doxyfile ; echo "GENERATE_HTML = yes" ) | doxygen -
        *make[2]: Leaving directory '/home/harter/work/git/RIOT/doc/doxygen'
        false
        Makefile:10: recipe for target 'doc' failed
        make[1]: *** [doc] Error 1
        make[1]: Leaving directory '/home/harter/work/git/RIOT'

When run with the same test commit in master, the error is not detected.

An invalid command in a doxygen message is also still detected

ERROR: Doxygen generates the following warnings:
core/include/assert.h:18: warning: Found unknown command `\auienras'

When you push force please put a message in the PR or we do not receive any notification about it.

cladmi · 2018-11-28T17:35:25Z

@jia200x It's good for me, I let you have the final word.

jcarrano · 2018-11-29T10:26:42Z

I tested locally by adding a false in the make doc target.

D'oh, I didn't realize I could do that. I was trying to force it to fail by inserting garbage into the doxyfile.

The issues were addressed (see comments)

jcarrano requested a review from jia200x August 22, 2018 13:54

jia200x previously requested changes Aug 22, 2018

View reviewed changes

jcarrano added Area: tools Area: Supplementary tools Area: doc Area: Documentation Area: CI Area: Continuous Integration of RIOT components labels Aug 24, 2018

jia200x added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 28, 2018

jcarrano force-pushed the doccheck-report-total-failure branch from 5b3f873 to 96e9927 Compare September 18, 2018 12:32

jcarrano force-pushed the doccheck-report-total-failure branch 2 times, most recently from e564137 to a37c56e Compare October 24, 2018 12:30

jcarrano mentioned this pull request Oct 24, 2018

travis.yml: Use lesscpy from pip. #10237

Closed

jcarrano added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 24, 2018

jcarrano mentioned this pull request Oct 25, 2018

travis.yml: use the same image as Murdock. #10251

Merged

jcarrano force-pushed the doccheck-report-total-failure branch from a37c56e to bf06240 Compare October 25, 2018 10:55

jcarrano force-pushed the doccheck-report-total-failure branch from db82f40 to d920c6e Compare October 25, 2018 11:56

cladmi removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 22, 2018

jcarrano force-pushed the doccheck-report-total-failure branch from d920c6e to ae4cecd Compare November 22, 2018 15:50

tools/doccheck: Detect when make doc fails to run.

ae4cecd

The previous doccheck would give a false negative when doxygen does not even run (for example, because of misconfiguration). Also, when doxygen fails to run, print the full output.

jcarrano added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 26, 2018

cladmi approved these changes Nov 28, 2018

View reviewed changes

jcarrano merged commit f4e810c into RIOT-OS:master Nov 29, 2018

aabadie added this to the Release 2019.01 milestone Dec 2, 2018

jcarrano mentioned this pull request Mar 5, 2019

git-lfs: add support for board documentation files #9818

Closed

Conversation

jcarrano commented Aug 22, 2018 • edited by cladmi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues/PRs references

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jia200x Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcarrano Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jcarrano commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

aabadie commented Aug 22, 2018

Uh oh!

jcarrano commented Oct 18, 2018

Uh oh!

jia200x commented Oct 18, 2018

Uh oh!

jcarrano commented Oct 24, 2018

Uh oh!

aabadie commented Oct 24, 2018

Uh oh!

cladmi commented Oct 24, 2018

Uh oh!

aabadie commented Oct 24, 2018

Uh oh!

jcarrano commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Oct 24, 2018

Uh oh!

aabadie commented Oct 24, 2018

Uh oh!

cladmi commented Oct 24, 2018

Uh oh!

jcarrano commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Nov 22, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Nov 28, 2018

Uh oh!

jcarrano commented Nov 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcarrano commented Aug 22, 2018 •

edited by cladmi

Loading

jia200x Aug 22, 2018 •

edited

Loading

jcarrano commented Aug 22, 2018 •

edited

Loading

jcarrano commented Oct 24, 2018 •

edited

Loading

jcarrano commented Oct 25, 2018 •

edited

Loading