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.

Sign up for GitHub

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

Jump to bottom

Out of source build development #121

Merged
merged 3 commits into from
Jan 7, 2025
Merged

Out of source build development #121

merged 3 commits into from
Jan 7, 2025

Conversation

Harry-Ramsey
Copy link
Contributor

Description

Closes #9755

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog not required because: testing API changes.
  • development PR provided.
  • framework PR provided #77
  • 3.6 PR not required because: for repo split.
  • tests 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 Dec 17, 2024
@Harry-Ramsey Harry-Ramsey added the size-s Estimated task size: small (~2d) label Dec 17, 2024
@Harry-Ramsey Harry-Ramsey force-pushed the out-of-source-build-development branch 2 times, most recently from 35bc548 to d93d138 Compare December 20, 2024 15:18
ronald-cron-arm
ronald-cron-arm requested changes Dec 20, 2024
View reviewed changes
tests/scripts/components-build-system.sh Outdated Show resolved Hide resolved
tests/scripts/components-build-system.sh Outdated Show resolved Hide resolved
@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Dec 20, 2024 •
edited
Loading

In the CI, the three TF-PSA-Crypto components fail. I think this is because of:

if [ -d ../include/mbedtls -a -d ../framework ]; then                           
    # Running inside an mbedtls checkout: get the framework from mbedtls.       
    FRAMEWORK="$PWD/../framework"                                               
else                                                                            
    # Running standalone: use our own framework.                                
    FRAMEWORK="$PWD/framework"                                                  
fi

in tf-psa-crypto/tests/scripts/all.sh
It means I think that while running the CI, the framework used by tf-psa-crypto/tests/scripts/all.sh is the mbedtls one that does not set TF_PSA_CRYPTO_ROOT_DIR or create automatically the directory for out-of-tree build. @mpg could probably help to resolve that properly.

@mpg
Copy link
Contributor

mpg commented Dec 23, 2024

@mpg could probably help to resolve that properly.

I don't think this is complicated to resolve: this block of code was written based on the assumption that we wanted tf-psa-crypto to use the framework from Mbed TLS - but that's not the direction we're taking now. (Also, something had to be done because the framework subdir of tf-psa-crypto did not exist when that code was written.)

So, I'd suggest simply replacing the quoted if else block with FRAMEWORK="$PWD/framework" and I think we should be OK.

@Harry-Ramsey Harry-Ramsey force-pushed the out-of-source-build-development branch 2 times, most recently from bc2f4e9 to 5793984 Compare December 23, 2024 17:01
@Harry-Ramsey
Remove duplicate out of source code
593901e 
This commit removes duplicate code to build TF-PSA-Crypto out of source
which has now been moved to mbedtls-framework.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey Harry-Ramsey force-pushed the out-of-source-build-development branch 3 times, most recently from ebfcd14 to 6fe6d7a Compare December 24, 2024 14:32
@Harry-Ramsey Harry-Ramsey mentioned this pull request Jan 6, 2025
Merged
6 tasks
mpg
mpg requested changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Please do the change requested here: #121 (comment)

@Harry-Ramsey
Copy link
Contributor Author

Please do the change requested here: #121 (comment)

Sorry, I missed that entirely. Fixed now

mpg
mpg reviewed Jan 6, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM. I'm just waiting for the CI results before I formally approve.

@ronald-cron-arm
Copy link
Contributor

The same on my side, LGTM, waiting for the CI.

@ronald-cron-arm ronald-cron-arm mentioned this pull request Jan 6, 2025
Merged
ronald-cron-arm
ronald-cron-arm previously approved these changes Jan 6, 2025
View reviewed changes
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.

@Harry-Ramsey
Update framework submodule for CMake TF-PSA-Crypto
0568ab1 
This commit updates the framework submodule to include commits relevant
to running TF-PSA-Crypto using CMake build system.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey
Remove obsolete if statement from all.sh
30120e3 
This commit removes an obsolete if statement from all.sh which would set
the FRAMEWORK path accordingly. Since TF-PSA-Crypto has its own version
of FRAMEWORk this code was no longer necessary

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey Harry-Ramsey force-pushed the out-of-source-build-development branch from 2ef089a to 30120e3 Compare January 6, 2025 14:28
ronald-cron-arm
ronald-cron-arm approved these changes Jan 6, 2025
View reviewed changes
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

@ronald-cron-arm ronald-cron-arm requested a review from mpg January 6, 2025 15:25
mpg
mpg approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@mpg mpg 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!

@mpg mpg added the approved Design and code approved - may be waiting for CI or backports label Jan 7, 2025
@mpg mpg added this pull request to the merge queue Jan 7, 2025
Merged via the queue into Mbed-TLS:development with commit e95def8 Jan 7, 2025
3 checks passed
@Harry-Ramsey Harry-Ramsey deleted the out-of-source-build-development branch January 9, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

@mpg mpg mpg approved these changes

Assignees

@Harry-Ramsey Harry-Ramsey

Labels
approved Design and code approved - may be waiting for CI or backports size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tf-psa-crypto all.sh: make the core manage the build directory
3 participants
@Harry-Ramsey @ronald-cron-arm @mpg