Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add application-help-to-doxygen #533

Merged
merged 6 commits into from
Jan 20, 2017
Merged

Add application-help-to-doxygen #533

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
65a364d
Add application-help-to-doxygen
Jan 16, 2017
69aab09
CR #533
Jan 18, 2017
fb29ac2
CR #533 2
Jan 18, 2017
e4cb3b9
Fix graph generation dependencies
Jan 18, 2017
853434f
CR #533 3
Jan 18, 2017
14ca5fd
Exclude -help from default targets
Jan 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# git master

* [533](https://github.com/Eyescale/CMake/pull/533):
Add application-help-to-doxygen extraction
* [532](https://github.com/Eyescale/CMake/pull/532):
Only update SHA-1s in .gitsubprojects after ```update``` instead of rewriting
the whole file
Expand Down
33 changes: 31 additions & 2 deletions CommonApplication.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
# Copyright (c) 2014-2016 Stefan.Eilemann@epfl.ch
# Copyright (c) 2014-2017 Stefan.Eilemann@epfl.ch
# Raphael.Dumusc@epfl.ch

# Configures the build for a simple application:
# common_application(<Name> [GUI] [EXAMPLE])
# common_application(<Name> [GUI] [EXAMPLE] [DOXYGEN])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOXYGEN should be NOHELP

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#
# 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it not non-GUI applications?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, non-GUI applications have to do it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It does not fail silently
  2. Some GUI apps (livreGUI) start and stay open even with --help. muy bad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussions, will make opt-out for all applications. Internal devs using it for non-GUI applications shall be temporarily shot during PR.

#
# Input:
# * NAME_SOURCES for all compilation units
Expand Down Expand Up @@ -122,6 +123,34 @@ function(common_application Name)
endif()
endif()

if(NOT THIS_GUI OR THIS_DOXYGEN)
# run binary with --help to capture output for doxygen
set(_doc "${PROJECT_BINARY_DIR}/help/${Name}.md")
set(_cmake "${CMAKE_CURRENT_BINARY_DIR}/${Name}.cmake")
file(WRITE ${_cmake} "
execute_process(COMMAND \${APP} --help
OUTPUT_FILE ${_doc} ERROR_FILE ${_doc}.error)
file(READ ${_doc} _doc_content)
if(NOT _doc_content)
message(FATAL_ERROR \"${Name} is missing --help\")
endif()
file(WRITE ${_doc} \"${Name} {#${Name}}
============

```
\${_doc_content}
```
\")
")
add_custom_command(OUTPUT ${_doc}
COMMAND ${CMAKE_COMMAND} -DAPP=$<TARGET_FILE:${Name}> -P ${_cmake}
DEPENDS ${Name}
COMMENT "Creating help for ${Name}")
add_custom_target(${Name}-help ALL DEPENDS ${_doc})
add_dependencies(${PROJECT_NAME}-all ${Name}-help)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I agree, if we fear that it might break unnoticed, then we should add a doxygen build to all Jenkins plans.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

set_property(GLOBAL APPEND PROPERTY ${PROJECT_NAME}_HELP ${Name})
endif()

if(NOT ${NAME}_OMIT_CHECK_TARGETS)
common_check_targets(${Name})
endif()
Expand Down
1 change: 1 addition & 0 deletions Doxyfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,7 @@ WARN_LOGFILE =

INPUT = @CMAKE_INSTALL_PREFIX@/include/@PROJECT_INCLUDE_NAME@ \
@PROJECT_SOURCE_DIR@/doc \
@PROJECT_BINARY_DIR@/help \
@DOXYGEN_EXTRA_INPUT@

# This tag can be used to specify the character encoding of the source files
Expand Down
13 changes: 13 additions & 0 deletions DoxygenRule.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it would pick up stale files (see above for manual checks during release...)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we document this if the user is not supposed to use them?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if(__help)
set(__index "${PROJECT_BINARY_DIR}/help/applications.md")
file(WRITE ${__index} "Application Help {#apps}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applications in plural ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik that is frenglish.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, my bad then :-)

============

")
foreach(_help ${__help})
file(APPEND ${__index} "* @subpage ${_help}\n")
endforeach()
endif()

# list-to-string transform
string(REPLACE ";" " " DOXYGEN_EXTRA_INPUT "${DOXYGEN_EXTRA_INPUT}")

Expand Down