DOC: inconsistent dependencies for documentation build#6550
DOC: inconsistent dependencies for documentation build#6550albert-github wants to merge 1 commit into
Conversation
| add_custom_command( | ||
| OUTPUT | ||
| "${ITK_BINARY_DIR}/Documentation/Doxygen/Examples.dox" | ||
| Examples.dox | ||
| COMMAND | ||
| ${CMAKE_COMMAND} -D "PROJECT_SOURCE_DIR:PATH=${ITK_SOURCE_DIR}" -D | ||
| "OUTPUT_FILE:PATH=${ITK_BINARY_DIR}/Documentation/Doxygen/Examples.dox" -P | ||
| "OUTPUT_FILE:PATH=${ITK_EXAMPLES}" -P | ||
| "${ITK_SOURCE_DIR}/Utilities/Doxygen/GenerateExamplesDox.cmake" | ||
| WORKING_DIRECTORY "${ITK_SOURCE_DIR}/Examples" | ||
| #WORKING_DIRECTORY "${ITK_SOURCE_DIR}/Examples" | ||
| WORKING_DIRECTORY ${ITK_BINARY_DIR}/Documentation/Doxygen | ||
| DEPENDS | ||
| "${ITK_SOURCE_DIR}/Examples" | ||
| ${EXAMPLES_LIST} | ||
| "${ITK_SOURCE_DIR}/Utilities/Doxygen/GenerateExamplesDox.cmake" | ||
| ) | ||
| set_source_files_properties(${ITK_EXAMPLES} PROPERTIES GENERATED 1) | ||
| add_custom_target( | ||
| ITKDoxygenExamplesDox | ||
| ITKDoxygenExamplesDox | ||
| ALL | ||
| DEPENDS | ||
| "${ITK_BINARY_DIR}/Documentation/Doxygen/Examples.dox" | ||
| Examples.dox | ||
| ) |
There was a problem hiding this comment.
Match generated output
OUTPUT Examples.dox and the target dependency are relative to this CMake directory’s binary tree, but the script writes to ${ITK_EXAMPLES} under Documentation/Doxygen. With Ninja/Make, the declared output is never produced, so the custom command remains out of date or fails after generation even though the intended file exists.
There was a problem hiding this comment.
@albert-github Can you please address this greptile concern?
There was a problem hiding this comment.
I will look into it, but it will take a bit over a week before I have time to really look into it (and I don't have ninja).
There was a problem hiding this comment.
Greptile is right, and I confirmed with a minimal Ninja reproduction that this fails on a fresh build tree. Two problems:
- A relative
OUTPUT Examples.doxresolves to${ITK_BINARY_DIR}/Utilities/Doxygen/Examples.dox(this directory's binary dir), while the script writes to${ITK_EXAMPLES}underDocumentation/Doxygen— so the declared output is never produced. - Ninja does not create
WORKING_DIRECTORY, socd .../Documentation/Doxygenfails before the script even runs. SinceGenerateExamplesDox.cmakeuses only absolute paths, theWORKING_DIRECTORYcan be dropped entirely.
Suggested replacement (verified to build and be a no-op on rebuild with Ninja):
add_custom_command(
OUTPUT
${ITK_EXAMPLES}
COMMAND
${CMAKE_COMMAND} -D "PROJECT_SOURCE_DIR:PATH=${ITK_SOURCE_DIR}" -D
"OUTPUT_FILE:PATH=${ITK_EXAMPLES}" -P
"${ITK_SOURCE_DIR}/Utilities/Doxygen/GenerateExamplesDox.cmake"
DEPENDS
${EXAMPLES_LIST}
"${ITK_SOURCE_DIR}/Utilities/Doxygen/GenerateExamplesDox.cmake"
)
add_custom_target(ITKDoxygenExamplesDox ALL DEPENDS ${ITK_EXAMPLES})The set_source_files_properties(... GENERATED 1) block can also be removed — add_custom_command outputs are automatically marked GENERATED — and please delete the commented-out #WORKING_DIRECTORY line rather than keeping it.
There was a problem hiding this comment.
Thanks for looking into the problem.
With the given change it looks like (for me with nmake):
- it runs with a clean build
- it runs also when compared to previous build an example file is added
- It looks like there is a problem when a file is removed from the list of examples.
There was a problem hiding this comment.
@albert-github Thanks for testing and for reporting the removal case — it is a real problem, and it is not specific to nmake. I reproduced it with Ninja and tracked down the cause: it is caused by my suggested patch, and the reason the current code appears to handle removal is the very bug Greptile flagged. Both are the same finding. Patch below resolves all of it; verified locally on a real ITK documentation build.
Why removal appeared to work before, and breaks with my patch
The current OUTPUT Examples.dox is relative, so it resolves to ${ITK_BINARY_DIR}/Utilities/Doxygen/Examples.dox, while the script writes to ${ITK_EXAMPLES} under Documentation/Doxygen. The declared output is therefore never created, so the command is permanently out of date and reruns on every build — which incidentally always picks up removals. That is why removal looks fine today: it is a side effect of the path bug, not a working dependency.
Once the path is corrected, the command is properly up to date, and the real gap shows: CONFIGURE_DEPENDS does its job (Ninja prints GLOB mismatch! The following files were removed: and CMake re-runs, so EXAMPLES_LIST is correct), but that only updates the dependency list. Deleting a dependency cannot make an existing output out of date — Examples.dox is newer than every remaining input and the command line is unchanged, so nothing reruns and the deleted example stays listed forever.
The fix is to depend on something whose content changes when the glob does: a stamp file holding the list. file(CONFIGURE) is write-if-different, so it does not reintroduce rebuild-every-time.
Patch (replaces the block from the set(ITK_EXAMPLES ...) line through add_custom_target)
set(ITK_EXAMPLES ${ITK_BINARY_DIR}/Documentation/Doxygen/Examples.dox)
# Removing an example changes only EXAMPLES_LIST, which cannot make
# Examples.dox out of date; depend on a stamp holding the list itself so a
# deletion is seen. file(CONFIGURE) rewrites only when the content differs.
set(
ITK_EXAMPLES_LIST_STAMP
${ITK_BINARY_DIR}/Documentation/Doxygen/ExamplesList.txt
)
string(REPLACE ";" "\n" ITK_EXAMPLES_LIST_CONTENT "${EXAMPLES_LIST}")
file(
CONFIGURE
OUTPUT ${ITK_EXAMPLES_LIST_STAMP}
CONTENT "${ITK_EXAMPLES_LIST_CONTENT}\n"
)
# Custom command to generate a examples page which include all ITK examples
add_custom_command(
OUTPUT
${ITK_EXAMPLES}
COMMAND
${CMAKE_COMMAND} -D "PROJECT_SOURCE_DIR:PATH=${ITK_SOURCE_DIR}" -D
"OUTPUT_FILE:PATH=${ITK_EXAMPLES}" -P
"${ITK_SOURCE_DIR}/Utilities/Doxygen/GenerateExamplesDox.cmake"
DEPENDS
${EXAMPLES_LIST}
${ITK_EXAMPLES_LIST_STAMP}
"${ITK_SOURCE_DIR}/Utilities/Doxygen/GenerateExamplesDox.cmake"
)
add_custom_target(ITKDoxygenExamplesDox ALL DEPENDS ${ITK_EXAMPLES})Relative to what is pushed, this: makes OUTPUT and the target dependency absolute; drops WORKING_DIRECTORY (Ninja does not create it, and GenerateExamplesDox.cmake uses only absolute paths) together with the commented-out line above it; drops set_source_files_properties(... GENERATED 1) (add_custom_command outputs are marked GENERATED automatically); and adds the list stamp.
The formatting above is already gersemi-clean, so it should pass the style hook as-is.
Local testing
Real ITK tree, -DITK_BUILD_DOCUMENTATION=ON, CMake 4.2.1 / Ninja 1.13.2 / Linux. A = this PR as pushed, B = my earlier suggestion, C = the patch above.
| scenario | A (pushed) | B (earlier suggestion) | C (patch above) |
|---|---|---|---|
| clean build | 293 listed ✅ | 293 ✅ | 293 ✅ |
| add an example | 294 ✅ | 294 ✅ | 294 ✅ |
| remove an example | 292 ✅ | 293 ❌ stale | 292 ✅ |
| no-op rebuild | ❌ regenerates every build | ✅ no work to do | ✅ no work to do |
For C also checked: removing a tracked example (Examples/Filtering/BinaryThresholdImageFilter.cxx) drops it from Examples.dox and restoring it brings it back; a forced reconfigure with no source change leaves the stamp mtime byte-identical and the build still reports no work to do. pre-commit run --all-files exits 0.
I have not verified this on nmake — since you do not have Ninja and I do not have nmake, it would be worth a quick check on your side that the removal case now passes there too.
There was a problem hiding this comment.
Thanks for the new patch this indeed looks like to work prpoerly.
|
Preferably squash into one commit, either upon merge, or before merge. |
|
Indeed should be / can be squashed. |
Handling the addition and removal of example files for the file `Examples.dox` Fixes InsightSoftwareConsortium#6545.
de895e4 to
3861a6c
Compare
|
I squashed and rebased. |
Handling the addition and removal of example files for the file
Examples.doxfixes #6545
PR Checklist