Disable auto updating of docs/doxygen/GeosxConfig.hpp #2602
Conversation
| if("${header_file}" MATCHES "HYPRE_DEVELOP_BRANCH *\"([^\"]*)\"") | ||
| set(hypre_dev_branch "${CMAKE_MATCH_1}") | ||
| endif() | ||
| set(hypre_VERSION "${hypre_dev_string} (${hypre_dev_branch})" CACHE STRING "" FORCE) |
There was a problem hiding this comment.
I find it useful to know the name of the branch associated with the commit hash. HYPRE_BRANCH_NAME provides that info. Should it be added here? PS: very nice organization work
There was a problem hiding this comment.
@victorapm I think I just took the macros that we used to print out in HypreInterface::initialize() and moved them to CMake-time for consistency with other dependecies. HYPRE_DEVELOP_STRING and HYPRE_DEVELOP_BRANCH are the ones we used to print. Should I replace HYPRE_DEVELOP_BRANCH with HYPRE_BRANCH_NAME? What is the difference between the two?
There was a problem hiding this comment.
Good! HYPRE_BRANCH_NAME gives the name of the branch that a particular commit belongs to, HYPRE_DEVELOP_BRANCH gives the name of the main development branch in hypre (master). So the first is more general and should be the preferable choice.
Lastly, I think you are aware but just mentioning here: these macros are only defined when building hypre from directory that has been cloned from git, so they don't exist for hypre tarballs downloaded from git or releases
klevzoff
force-pushed
the
cleanup/klevzoff/geos-config-for-doxygen
branch
2 times, most recently
from
7467a53 to
1e06d70
Compare
| configure_file( ${CMAKE_SOURCE_DIR}/coreComponents/common/GeosxConfig.hpp.in | ||
| ${CMAKE_SOURCE_DIR}/docs/doxygen/GeosxConfig.hpp ) |
There was a problem hiding this comment.
The status after this patch would be that we'll end up with the out of date information.
This may not be what we want either.
I wonder if the simplest answer to this hassle is to generate GeosxConfig.hpp from GeosxConfig.hpp.in (like we're doing), but in the generatedSrc and not in the sources (I think) folder.
So it's not committed but up-to-date for all the builds.
That looks to me like a sensible thing to do.
There was a problem hiding this comment.
The goal of generating this file is for it to be accessible in readthedocs environment where doxygen docs are built and where we don't have a full-fledged build environment to run cmake in. It has to be committed to the repo, that's the whole point. Otherwise we are already generating one under build/include/common/GeosxConfig.hpp, it would do the job perfectly if only we could do it on readthedocs.
Yes, it will go out of date, but it only affects what the user sees when they browse doxygen online, it's not a big deal. Once in a while I will push an updated version, but even if I don't, not much harm.
There was a problem hiding this comment.
This file could also be an artefact from the ci where the cmake environment is set. Either used by a documentation target either another pipeline. See draft idea #2603
There was a problem hiding this comment.
Yes, it will go out of date, but it only affects what the user sees when they browse doxygen online, it's not a big deal. Once in a while I will push an updated version, but even if I don't, not much harm.
If we need to commit/push once in a while, what's the benefit w.r.t. directly editing readthedocs (which can even be done in the web ui, no need to tweak anything in your own computer)?
There was a problem hiding this comment.
If we need to commit/push once in a while, what's the benefit w.r.t. directly editing readthedocs (which can even be done in the web ui, no need to tweak anything in your own computer)?
Would actually be nice if that page was built from the config file, but it's not (and it's severely out of date at this point 😆). GeosxConfig.hpp only shows up here, but its presence makes all #includes of it in the source clickable and lets users read the "documentation" of each macro. It's just a fairly small convenience.
… updating. Cleanup some version stuff.
klevzoff
force-pushed
the
cleanup/klevzoff/geos-config-for-doxygen
branch
from
1e06d70 to
eae5635
Compare
|
it this PR stuck or there is a chance to merge it? |
Just needs an approval from @rrsettgast |
| @@ -164,6 +155,15 @@ | |||
| /// Version information for suitesparse | |||
| #cmakedefine suitesparse_VERSION @suitesparse_VERSION@ | |||
|
|
|||
| /// Version information for hypre | |||
| #cmakedefine hypre_VERSION @hypre_VERSION@ | |||
There was a problem hiding this comment.
does this give equivalent information as was done by the removed code in HypreInterface.cpp
There was a problem hiding this comment.
Yes, it's taken from the same hypre macros, the only difference is it's now extracted at config-time rather than compile-time.
… updating. Cleanup some version stuff. (#2602) This PR adds an explicit target for updating docs/doxygen/GeosxConfig.hpp, and disables auto updating. No more diffs on every system. The file will be updated manually once in a while. Drive-by cleanup of some version-related stuff (mainly just to unify hypre version printout with the rest; also add petsc/trilinos versions). Originally I was hoping to move TPL version macros from GeosxConfig.hpp to GeosxVersion.hpp, but that doesn't work since the latter is generated at build time, not cmake time, so all those CMake version variables are not available to it.
This PR adds an explicit target for updating
docs/doxygen/GeosxConfig.hpp, and disables auto updating. No more diffs on every system. The file will be updated manually once in a while.Drive-by cleanup of some version-related stuff (mainly just to unify hypre version printout with the rest; also add petsc/trilinos versions).
Originally I was hoping to move TPL version macros from
GeosxConfig.hpptoGeosxVersion.hpp, but that doesn't work since the latter is generated at build time, not cmake time, so all those CMake version variables are not available to it.