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.

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

TF-PSA-Crypto Doxygen Adaptations #115

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

Harry-Ramsey
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey commented Jan 8, 2025

Description

Adapt doxygen scripts to work for TF-PSA-Crypto.

PR checklist

Please add the numbers (or links) of the associated pull requests for consuming branches. You can omit branches where this pull request is not needed.

  • crypto PR Mbed-TLS/TF-PSA-Crypto: #142
  • development PR Mbed-TLS/mbedtls: not required.
  • 3.6 PR Mbed-TLS/mbedtls: not required.

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

@Harry-Ramsey Harry-Ramsey self-assigned this Jan 8, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the tf-psa-crypto-doxygen branch 2 times, most recently from 3a2fde9 to b22c1b6 Compare January 10, 2025 10:29
@Harry-Ramsey Harry-Ramsey force-pushed the tf-psa-crypto-doxygen branch 2 times, most recently from fe80451 to 41789ad Compare January 10, 2025 12:56
scripts/apidoc_full.sh Outdated Show resolved Hide resolved
scripts/doxygen.sh Outdated Show resolved Hide resolved
@Harry-Ramsey Harry-Ramsey force-pushed the tf-psa-crypto-doxygen branch from 41789ad to a86eefa Compare January 23, 2025 16:49
@Harry-Ramsey Harry-Ramsey force-pushed the tf-psa-crypto-doxygen branch from 59e9177 to f16b29c Compare January 30, 2025 16:53
@ronald-cron-arm ronald-cron-arm self-requested a review February 3, 2025 10:58
scripts/all-core.sh Outdated Show resolved Hide resolved
scripts/apidoc_full.sh Show resolved Hide resolved
scripts/apidoc_full.sh Outdated Show resolved Hide resolved
scripts/check-doxy-blocks.pl Show resolved Hide resolved
scripts/check-doxy-blocks.pl Outdated Show resolved Hide resolved
@Harry-Ramsey Harry-Ramsey force-pushed the tf-psa-crypto-doxygen branch 3 times, most recently from a256c44 to fa1bc66 Compare February 5, 2025 15:50
scripts/apidoc_full.sh Outdated Show resolved Hide resolved
scripts/apidoc_full.sh Outdated Show resolved Hide resolved
@Harry-Ramsey Harry-Ramsey force-pushed the tf-psa-crypto-doxygen branch 2 times, most recently from 7eea683 to 8660359 Compare February 6, 2025 13:32
mv $CONFIG_BAK $CONFIG_H
elif in_tf_psa_crypto_repo; then
scripts/config.py realfull
cmake -DCMAKE_BUILD_TYPE:String=Check -DGEN_FILES=ON .
Copy link
Contributor

Choose a reason for hiding this comment

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

Having played with it, it would be better to build out of tree in a temporary directory. Otherwise when we run doxygen.sh locally the source tree ends up with all the CMake artefacts. mktemp is it seems the way to create a temporary directory in bash.

Copy link
Contributor

Choose a reason for hiding this comment

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

tmp=$(mktemp -d) creates a temporary directory in a user-configurable location (defaulting to /tmp) and stores the location in $tmp. tmp=$(TMPDIR=/path/to/parent mktemp -d) creates a temporary directory under /path/to/parent. Avoid other options to mktemp for portability to older Linux and *BSD.

But do we really need a temporary directory name? We want to keep the Doxygen build products around, so why not use a fixed directory name like doxygen/build-apidoc-full?

Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation is currently generated at the root of the source tree in the apidoc directory thus we do not need to keep the build tree. A fixed build directory in the source tree would not really improve things regarding not modifying the build tree thus not sure it would be an improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

A fixed build directory is slightly easier to program, and is easier to inspect and clean if an error occurs.

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm Feb 10, 2025

Choose a reason for hiding this comment

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

okay the main thing I wanted to avoid here was to build in the source tree. I am fine with a build directory doxygen/build-apidoc-full.

@valeriosetti valeriosetti self-requested a review February 10, 2025 10:02
valeriosetti
valeriosetti previously approved these changes Feb 10, 2025
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

To fix the CI test_full_cmake_clang and test_psa_compliance jobs you need to rebase on top of main.

CRYPTO_CONFIG_H='include/psa/crypto_config.h'
if in_mbedtls_repo; then
CONFIG_H='include/mbedtls/mbedtls_config.h'
CRYPTO_CONFIG_H='tf-psa-crypto/include/psa/crypto_config.h'
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not work in 3.6 case.

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

A few things to fix, otherwise we do not plan to have mbedtls PRs for this thus we need a green CI and for that we need to rebase against latest main.


scripts/config.py realfull
make apidoc
if in_tf_psa_crypto_repo || (in_mbedtls_repo && !in_3_6_branch); then
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if in_tf_psa_crypto_repo || (in_mbedtls_repo && !in_3_6_branch); then
if in_tf_psa_crypto_repo || (in_mbedtls_repo && ! in_3_6_branch); then

without a space between the exclamation mark and "in_3_6_branch" it does not work.

Comment on lines 17 to 25
if in_mbedtls_repo; then
CONFIG_H='include/mbedtls/mbedtls_config.h'
if [ -r $CONFIG_H ]; then :; else
echo "$CONFIG_H not found" >&2
fi
fi

CONFIG_BAK=${CONFIG_H}.bak
cp -p $CONFIG_H $CONFIG_BAK
if in_mbedtls_repo; then
CRYPTO_CONFIG_H='tf-psa-crypto/include/psa/crypto_config.h'
if [ -r $CRYPTO_CONFIG_H ]; then :; else
echo "$CRYPTO_CONFIG_H not found" >&2
exit 1
fi
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

if in_mbedtls_repo; then                                                        
    CONFIG_H='include/mbedtls/mbedtls_config.h'                                 
    if [ -r $CONFIG_H ]; then :; else                                           
        echo "$CONFIG_H not found" >&2                                          
    fi                                                                          
    if ! in_3_6_branch; then                                                    
        CRYPTO_CONFIG_H='tf-psa-crypto/include/psa/crypto_config.h'             
    fi                                                                          
fi

Comment on lines 32 to 38
if in_tf_psa_crypto_repo; then
CRYPTO_CONFIG_H='include/psa/crypto_config.h'
if [ -r $CRYPTO_CONFIG_H ]; then :; else
echo "$CRYPTO_CONFIG_H not found" >&2
exit 1
fi
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if in_tf_psa_crypto_repo; then
CRYPTO_CONFIG_H='include/psa/crypto_config.h'
if [ -r $CRYPTO_CONFIG_H ]; then :; else
echo "$CRYPTO_CONFIG_H not found" >&2
exit 1
fi
fi
if in_tf_psa_crypto_repo; then
CRYPTO_CONFIG_H='include/psa/crypto_config.h'
fi

Comment on lines 40 to 38
if in_tf_psa_crypto_repo || (in_mbedtls_repo && !in_3_6_branch); then
CRYPTO_CONFIG_BAK=${CRYPTO_CONFIG_H}.bak
cp -p $CRYPTO_CONFIG_H $CRYPTO_CONFIG_BAK
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if in_tf_psa_crypto_repo || (in_mbedtls_repo && !in_3_6_branch); then
CRYPTO_CONFIG_BAK=${CRYPTO_CONFIG_H}.bak
cp -p $CRYPTO_CONFIG_H $CRYPTO_CONFIG_BAK
fi
if in_tf_psa_crypto_repo || (in_mbedtls_repo && ! in_3_6_branch); then
if [ -r $CRYPTO_CONFIG_H ]; then :; else
echo "$CRYPTO_CONFIG_H not found" >&2
exit 1
fi
CRYPTO_CONFIG_BAK=${CRYPTO_CONFIG_H}.bak
cp -p $CRYPTO_CONFIG_H $CRYPTO_CONFIG_BAK
fi

cd $TF_PSA_CRYPTO_ROOT_DIR
fi

if in_tf_psa_crypto_repo || (in_mbedtls_repo && !in_3_6_branch); then
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if in_tf_psa_crypto_repo || (in_mbedtls_repo && !in_3_6_branch); then
if in_tf_psa_crypto_repo || (in_mbedtls_repo && ! in_3_6_branch); then

This commit adapts check-doxy-blocks to run for TF-PSA-Crypto.

Signed-off-by: Harry Ramsey <[email protected]>
This commit adapts the scripts apidoc_full.sh and doxygen.sh to run for
TF-PSA-Crypto out of source builds.

Signed-off-by: Harry Ramsey <[email protected]>
Copy link
Contributor

@ronald-cron-arm ronald-cron-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, thanks.

@ronald-cron-arm ronald-cron-arm merged commit 655a117 into Mbed-TLS:main Feb 11, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants