Skip to content

[backport 1.1] doxygen: use CMake build dir intead of source dir to host generated files#760

Open
valeriosetti wants to merge 8 commits intoMbed-TLS:tf-psa-crypto-1.1from
valeriosetti:backport-pr718
Open

[backport 1.1] doxygen: use CMake build dir intead of source dir to host generated files#760
valeriosetti wants to merge 8 commits intoMbed-TLS:tf-psa-crypto-1.1from
valeriosetti:backport-pr718

Conversation

@valeriosetti
Copy link
Copy Markdown
Contributor

@valeriosetti valeriosetti commented Apr 15, 2026 •
edited by gilles-peskine-arm
Loading

Description

Backport of #718. Resolves #381.

PR checklist

  • changelog provided
  • framework PR not required
  • TF-PSA-Crypto development PR provided doxygen: use CMake build dir intead of source dir to host generated files #718
  • TF-PSA-Crypto 1.1 PR here
  • mbedtls development PR not required because: no change there
  • mbedtls 4.1 PR not required because: no change there
  • mbedtls 3.6 PR not required because: no change there
  • tests not required because: no code change

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

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

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
README: update path of apidoc
f0393b0 
"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
b67abe4 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
doxygen: add quotes to directory paths to prevent failures due to whi…
7b435c2 
…tespaces

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti valeriosetti mentioned this pull request Apr 15, 2026
Open
6 tasks
@valeriosetti valeriosetti added 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) labels Apr 15, 2026
bjwtaylor
bjwtaylor reviewed Apr 16, 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.

Do we need to update framework/scripts/apidoc_full.sh:58, there appears to be at least a stale comment here. Though not sure if can just be removed or if there is a bigger issue?

@valeriosetti
Copy link
Copy Markdown
Contributor Author

valeriosetti commented Apr 16, 2026 •
edited
Loading

Do we need to update framework/scripts/apidoc_full.sh:58, there appears to be at least a stale comment here. Though not sure if can just be removed or if there is a bigger issue?

Thanks to this comment I made a quick comparison with what's being done in the mbedtls repo and I found that:

  1. in mbedtls doxygen files are not compiled by CMake, whereas in tf-psa-crypto they are compiled.
  2. in mbedtls the documentation is still built in-tree, whereas in tf-psa-crypto it won´t after this PR.

Point (2) suggest that:

  • I need to open the corresponding PRs also in mbedtls (development and 4.1) to fix this.
  • I can drop that comment (and the associated action) from the framework script since it won´t apply anymore

gilles-peskine-arm
gilles-peskine-arm reviewed Apr 16, 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.

This is a technically correct backport of #718. However, I'm not sure it's an acceptable change in an LTS branch.

IIRC you suggested creating an apidoc symlink in the source tree, I think that would be an acceptable compromise (doing nothing if apidoc is an existing directory). (Although that doesn't work on Windows.)

@valeriosetti
Copy link
Copy Markdown
Contributor Author

valeriosetti commented Apr 16, 2026

This is a technically correct backport of #718. However, I'm not sure it's an acceptable change in an LTS branch.

So just to be sure I understood correctly: the symlink should be done only for the LTS or also for development (i.e. #718)?

@gilles-peskine-arm
Copy link
Copy Markdown
Contributor

gilles-peskine-arm commented Apr 17, 2026

We don't have firm rules around build system changes. I think it's ok to keep things simple in the development branch, but I'm uncomfortable with changing the interface for the build of the documentation in an LTS branch. So my opinion is symlink in LTS, no symlink in development. But others may have a different opinion.

@valeriosetti
doxygen: create symlink to the apidoc in the build folder
38bf41f 
Add a new CMake's custom command that's executed after apidoc is built.
It places an in-tree symlink to the 'apidoc' folder in the binary
directory.

While at it also update the changelog note about this improvement.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
bjwtaylor
bjwtaylor previously approved these changes Apr 23, 2026
View reviewed changes
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.

LGTM except for in-tree builds

Comment thread CMakeLists.txt Outdated

# Create an in-tree symlink to the apidoc built in the binary folder. This is
# only performed on Unix platforms and if "apidoc" doesn't exist in-tree.
if(UNIX AND NOT EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/apidoc)
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.

This checks for the existence of apidoc when you run cmake, not when you build the documentation.

One consequence is that if you do an in-tree build from a fresh checkout, cmake decides that you do want to create apidoc, but then it fails because that's an existing directory.

So at least this part should be skipped if the source tree is the build tree.

Furthermore, if you do an in-tree build after an out-of-tree build, the out-of-tree build overwrites the apidoc in the out-of-tree build. Admittedly that would be pretty unusual (I guess most users either do an in-tree build or one or more out-of-tree builds but not both), but it's definitely not desirable behavior.

Also, if you do two out-of-tree builds, which one will “win” apidoc depends on whether you do cmake -B dir1; make -C dir1; cmake -B dir2; make -C dir2 or cmake -B dir1; cmake -B dir2; make -B dir1; make -B dir2. This isn't wrong (only one build can win and there's no particular reason to prefer a specific one), but it's weird.

Copy link
Copy Markdown
Contributor Author

@valeriosetti valeriosetti Apr 29, 2026

Choose a reason for hiding this comment

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

this should be addressed now in db8d277

Comment thread ChangeLog.d/doxygen-build-folder.txt Outdated
Comment on lines +5 to +6
directory.
Fixes #381.
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.

Minor:

Suggested change
directory.
Fixes #381.
directory. Fixes #381.

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members labels Apr 24, 2026
@valeriosetti
changelog: remove a new-line char from changelog message
a32d8fc 
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti
cmake: fix symlink creation for apidoc
db8d277 
- only create when doing an out-of-tree build.
- evaluate if the symlink is to be created at runtime. In case of multiple
  successive build, only the first one will create the symlink.

Signed-off-by: Valerio Setti <vsetti@baylibre.com>
@valeriosetti valeriosetti added the needs-review Every commit must be reviewed by at least two team members label Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjwtaylor bjwtaylor bjwtaylor left review comments

@gilles-peskine-arm gilles-peskine-arm Awaiting requested review from gilles-peskine-arm

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

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

Roadmap pull requests (new board)
Status: In Development

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@valeriosetti @gilles-peskine-arm @bjwtaylor