Add application-help-to-doxygen #533
Conversation
See screenshot for example output. Runs all non-GUI applications with --help and pipes the output into a new doxygen section 'Application help'
|
|
| # | ||
| # Arguments: | ||
| # * GUI: if set, build cross-platform GUI application | ||
| # * EXAMPLE: install all sources in share/Project/examples/Name | ||
| # * DOXYGEN: opt into doxygen help extraction for GUI applications |
There was a problem hiding this comment.
No, non-GUI applications have to do it.
There was a problem hiding this comment.
OK, I didn't pay enough attention that it was always on. But now I realize, if it fails silently, why is it not on for all apps including GUI apps? If --help is not implemented, the risk of undefined behaviour is the same for a command line application than for a gui app, right? And I can see it being useful for Brayns for instance. Maybe it should be an opt-out instead?
There was a problem hiding this comment.
- It does not fail silently
- Some GUI apps (livreGUI) start and stay open even with --help. muy bad.
There was a problem hiding this comment.
After discussions, will make opt-out for all applications. Internal devs using it for non-GUI applications shall be temporarily shot during PR.
| get_property(__help GLOBAL PROPERTY ${PROJECT_NAME}_HELP) | ||
| if(__help) | ||
| set(__index "${PROJECT_BINARY_DIR}/help/applications.md") | ||
| file(WRITE ${__index} "Application Help {#apps} |
| DEPENDS ${Name} | ||
| COMMENT "Creating help for ${Name}") | ||
| add_custom_target(${Name}-help ALL DEPENDS ${_doc}) | ||
| add_dependencies(${PROJECT_NAME}-all ${Name}-help) |
There was a problem hiding this comment.
If I understand correctly, this runs all the applications with --help when building Brion-all? If so, that does not look correct, it should be ${PROJECT_NAME}-doxygen, no?
There was a problem hiding this comment.
The -all target is run before doxygen. We could also attach it to -doxygen, but this feels more consistent, and will make sure it does get fixed since not everybody does verify this stuff during release.
There was a problem hiding this comment.
Then it should be attached to the doxygen target, since that's where it is used. It does not make sense to run it every time we build. The argument that it has more chances of being fixed is not a valid reason, people won't notice if this breaks in any case unless they look at the documentation output...
There was a problem hiding this comment.
I disagree. It does fail, and it will show up earlier (ie in a PR). Experience shows that people won't notice. It also does not affect build performance.
@tribal-tec, what's your opinion?
There was a problem hiding this comment.
Fail early is good in principal, but I agree that building 'help' everytime you build 'all' is wrong, also annoying as I personally do 'all' quite often. Moreover, doxygen dependes on install, not on 'all'!
To fight problems being not noticed we could run doxygen as part of CI and report the errors from error. Now we actually have sometimes doxygen errors/warnings going unnoticed. We catch them only from time to time by checking the documentation CI plans...
There was a problem hiding this comment.
yes, I agree, if we fear that it might break unnoticed, then we should add a doxygen build to all Jenkins plans.
There was a problem hiding this comment.
Ok, will attach to project-doxygen.
For the record, I still think this is a bad idea since it makes the dependency chain even more entangled. As @tribal-tec shows, it's already not well understood:
- all is an internal, inaccessible CMake target
- project-all is our target to which all project dependencies are attached, now also used by people during dev work
- project-install depends on project-all (for obvious reasons)
- project-doxygen depends on project-install since it works on the installed headers only
- new: project-doxygen depends also on all project-help-targets
There was a problem hiding this comment.
And here I clicked send to quick: I can't do the above: the project-doxygen target does not exist here. It's the whole raison de etre for the project-all target to collect all dependencies for doxygen.
There was a problem hiding this comment.
In my understanding:
- the "all" target is the default target when one does just "make", and it should just build everything (not install or do anything fancy).
- the project-all target is the equivalent of the "all" target for each subproject, in the same way that project-install is the equivalent of the global "install" target.
- the project-doxygen is our own concept and can do whatever it likes to produce documentation.
If the project-doxygen is not available at this point, then the solution is to reverse the dependency order, exactly like it is done with the ${PROJECT_NAME}-coverage target. Which is also run by project-doxygen and not by project-all...
There was a problem hiding this comment.
the "all" target is the default target when one does just "make", and it should just build everything (not install or do anything fancy).
Yes.
the project-all target is the equivalent of the "all" target for each subproject, in the same way that project-install is the equivalent of the global "install" target.
Now it is. Originally it was created for project-doxygen.
the project-doxygen is our own concept and can do whatever it likes to produce documentation.
Yes.
If the project-doxygen is not available at this point, then the solution is to reverse the dependency order, exactly like it is done with the ${PROJECT_NAME}-coverage target. Which is also run by project-doxygen and not by project-all...
So now I have to create a project-help target to be able to attach it to project-doxygen?
Will do, but as a side note this is pretty demotivating for future work. For some reason it always feels like a ton of work for very little improvement. Somehow I would have less problems if this felt like an effort shared by all. Let's discuss f2f.
| @@ -88,6 +88,19 @@ if(NOT COMMON_ORGANIZATION_NAME) | |||
| set(COMMON_ORGANIZATION_NAME Unknown) | |||
| endif() | |||
|
|
|||
| # collect application help page | |||
| get_property(__help GLOBAL PROPERTY ${PROJECT_NAME}_HELP) | |||
There was a problem hiding this comment.
How about file(GLOB __help "${PROJECT_BINARY_DIR}/help/*.md") instead of a global property? Potentially a bit more flexible and decoupled (for someone who wants to add his own help files in there).
There was a problem hiding this comment.
No, it would pick up stale files (see above for manual checks during release...)
There was a problem hiding this comment.
The release is made by a Jenkin plan which does a clean build, and anyone doing a manual release will (should) do the same... But I see your point, as you prefer. ${PROJECT_NAME}_HELP should be documented as an IN variable here for consistency, and as an OUT variable in CommonApplication.cmake normally.
There was a problem hiding this comment.
Do we document this if the user is not supposed to use them?
There was a problem hiding this comment.
That's a fair question. My take on this is to try to document every in/out variable of each CMake file, because it makes the interdependence between different files explicit. Having hidden global variables is not good for maintenance in the long run. Regarding this specific case, I am not sure if there is a reason users should be discouraged to add their own help file for a python application for instance (tide, rtneuron-app.py, ...)
|
Do not merge due to opt-out needed for many targets now. |
| # Raphael.Dumusc@epfl.ch | ||
|
|
||
| # Configures the build for a simple application: | ||
| # common_application(<Name> [GUI] [EXAMPLE]) | ||
| # common_application(<Name> [GUI] [EXAMPLE] [DOXYGEN]) |
| @@ -25,6 +25,10 @@ | |||
| # * ${PROJECT_NAME}_VERSION_(MINOR|MAJOR) The project's major/minor version, | |||
| # generally set by the project() command in the top-level CMakeLists.txt. | |||
| # | |||
| # Input Global Properties | |||
| # * ${PROJECT_NAME}-HELP a list of doxygen page anchors to add to the | |||
See screenshot for example output. Runs all non-GUI applications with
--help and pipes the output into a new doxygen section 'Application
help'