Skip to content

doxygen: use CMake build dir intead of source dir to host generated files#718

Open
valeriosetti wants to merge 6 commits intoMbed-TLS:developmentfrom
valeriosetti:doxygen-fix
Open

doxygen: use CMake build dir intead of source dir to host generated files#718
valeriosetti wants to merge 6 commits intoMbed-TLS:developmentfrom
valeriosetti:doxygen-fix

Conversation

@valeriosetti
Copy link
Copy Markdown
Contributor

@valeriosetti valeriosetti commented Mar 19, 2026 •
edited
Loading

Description

Use CMAKE_BINARY_DIR instead of CMAKE_CURRENT_SOURCE_DIR to hold the generated files for Doxygen.

Resolves #381

PR checklist

@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) needs-ci Needs to pass CI tests labels Mar 19, 2026
@valeriosetti valeriosetti mentioned this pull request Mar 19, 2026
Merged
@valeriosetti valeriosetti added needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests labels Mar 20, 2026
@valeriosetti
Copy link
Copy Markdown
Contributor Author

valeriosetti commented Mar 20, 2026

There is only a single failure in the CI in tf_psa_crypto_check_doxygen_warnings:

02:13:35  warning: tag INCLUDE_PATH: include path `../drivers/builtin/include' does not exist
02:13:35  warning: tag INPUT: input source `../tests/include/alt-dummy' does not exist
02:13:35  warning: source ../drivers/builtin/include is not a readable file or directory... skipping.
02:13:35  warning: source ../tests/include/alt-dummy is not a readable file or directory... skipping.

but I cannot regenerate it locally even though I use the Ubuntu 16.04 Docker image. Any idea of what might be the reason for such difference?

@valeriosetti valeriosetti moved this to Triage in in Community Mar 23, 2026
@davidhorstmann-arm
Copy link
Copy Markdown
Contributor

davidhorstmann-arm commented Mar 23, 2026 •
edited
Loading

Unsure precisely why this is failing but there is an ominous comment here

 # The documentation is built in the source tree thus we can delete the
 # build tree.

It's possible your framework has changes that you haven't committed?

@davidhorstmann-arm
Copy link
Copy Markdown
Contributor

davidhorstmann-arm commented Mar 23, 2026

Size-xs but needs CI rework. Why does this have the framework update? It doesn't have a linked framework PR (although it may need one in the end).

@davidhorstmann-arm davidhorstmann-arm moved this from Triage in to Scoped in Community Mar 23, 2026
@gilles-peskine-arm gilles-peskine-arm added needs-work needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review labels Apr 10, 2026
gilles-peskine-arm
gilles-peskine-arm requested changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Unfortunately this doesn't work.

Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
@@ -71,12 +71,12 @@ set(version_number_files
doxygen/tfpsacrypto.doxyfile)
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm Apr 10, 2026

Choose a reason for hiding this comment

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

This is a bug fix, because you can have different cmake build trees with different configurations and therefore different documentation, so having the documentation in the source tree is incorrect.

But it's also an incompatible change. For users who use a single library configuration, but compile for multiple platforms, or who just use a separate build directory because it's the preferred way with cmake, having the documentation in the source tree was fine, and we're moving their cheese.

So we at the very least need to explain the change in a changelog entry. And I wonder if we should provide some kind of backward compatibility path. But I'm not sure what that would be.

Copy link
Copy Markdown
Contributor Author

@valeriosetti valeriosetti Apr 14, 2026

Choose a reason for hiding this comment

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

So we at the very least need to explain the change in a changelog entry. And I wonder if we should provide some kind of backward compatibility path. But I'm not sure what that would be.

No idea either. I can add a symlink in-tree to link to the folder in the build directory, but that doesn't work well for the 1st scenario you described, i.e. you can have different cmake build trees with different configurations and therefore different documentation. In this case only the last one would be kept. However this is also the same behavior we had before and since no one complained about that (at least no one that I'm aware of), can we assume it's a bug nobody noticed?

Comment thread CMakeLists.txt
@bjwtaylor bjwtaylor self-requested a review April 13, 2026 09:19
bjwtaylor
bjwtaylor reviewed Apr 13, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@bjwtaylor bjwtaylor left a comment

Choose a reason for hiding this comment

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

Adding myself as a reviewer for when this one is ready. If you re-request my review when you've actioned @gilles-peskine-arm comments I'll take a look.

@valeriosetti
doxygen: use CMake build dir intead of source dir to host generated f…
fbaf701 
…iles

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
doxygen: use PROJECT_BINARY_DIR to represent the root path of TF-PSA-…
fb112fd 
…Crypto

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
README: update path of apidoc
7d513e3 
"apidoc" is now built into the Cmake's build directory and not in the
source tree.
While at it also remove references to "apidoc/topic.html" because the file
is not generated as part of "tfpsacrypto-apidoc" target build.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
changelog: add note about apidoc build folder change
e775742 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti valeriosetti requested a review from bjwtaylor April 14, 2026 10:32
bjwtaylor
bjwtaylor reviewed Apr 14, 2026
View reviewed changes
Comment thread doxygen/tfpsacrypto.doxyfile.in Outdated
INPUT = @PROJECT_SOURCE_DIR@/include input @PROJECT_SOURCE_DIR@/tests/include/alt-dummy
FILE_PATTERNS = *.h
EXCLUDE = ../include/mbedtls/private
EXCLUDE = @PROJECT_SOURCE_DIR@/include/mbedtls/private
Copy link
Copy Markdown
Contributor

@bjwtaylor bjwtaylor Apr 14, 2026

Choose a reason for hiding this comment

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

I think it might be useful to have these in quotes as they are absolute paths and may contain spaces which might cause issues? Same with Line #9 and #24

@valeriosetti
doxygen: add quotes to directory paths to prevent failures due to whi…
6c68e92 
…tespaces

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
bjwtaylor
bjwtaylor previously approved these changes Apr 15, 2026
View reviewed changes
gilles-peskine-arm
gilles-peskine-arm approved these changes Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-work needs-ci Needs to pass CI tests labels Apr 15, 2026
@valeriosetti valeriosetti mentioned this pull request Apr 15, 2026
Open
7 tasks
gilles-peskine-arm
gilles-peskine-arm requested changes Apr 24, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Please add the issue number to the changelog entry.

Comment thread ChangeLog.d/doxygen-build-folder.txt Outdated
@@ -0,0 +1,3 @@
Bugfix
* Doxygen documentation `apidoc` is now built into CMake's binary folder,
not in-tree.
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm Apr 24, 2026

Choose a reason for hiding this comment

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

Suggested change
not in-tree.
not in-tree. Fixes #381.

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Apr 24, 2026
@valeriosetti
changelog: add issue number in changelog
aec10bc 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
gilles-peskine-arm
gilles-peskine-arm approved these changes Apr 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members and removed needs-work labels Apr 29, 2026
@valeriosetti valeriosetti requested a review from bjwtaylor April 29, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@bjwtaylor bjwtaylor bjwtaylor approved these changes

Assignees

No one assigned

Labels

needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)

Projects

Community
Status: Scoped

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Doxygen documentation built in the source tree instead of the build tree

4 participants

@valeriosetti @davidhorstmann-arm @gilles-peskine-arm @bjwtaylor