Skip to content

Conversation

@bjwtaylor
Copy link

@bjwtaylor bjwtaylor commented Oct 15, 2025

Description

Remove lcov.sh as this will be moved to the framework contributes Mbed-TLS/mbedtls-framework#93

This PR is part of a multistage PR, however the code changes involved appear to not effect the CI so it can be merged in any order. However it is suggested it is merged in the following order.

  1. Add adapted lcov.sh to the framework mbedtls-framework#224
  2. Update lcov.sh paths in make files TF-PSA-Crypto#536
  3. Move lcov 3.6 #10452
  4. Remove lcov.sh as this will be moved to the framework #10450

PR checklist

Release job for coverage testing: https://ci.trustedfirmware.org/view/Mbed-TLS/job/mbedtls-release-ci-testing/32/

@bjwtaylor bjwtaylor 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 size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits needs-preceding-pr Requires another PR to be merged first labels Oct 15, 2025
@bjwtaylor bjwtaylor marked this pull request as ready for review October 15, 2025 08:42
@valeriosetti valeriosetti self-requested a review October 16, 2025 11:37
@mpg mpg removed the needs-preceding-pr Requires another PR to be merged first label Oct 24, 2025
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.

Looks good to me except I think we need to update the framework pointer in order for the script to be present in its new location.

@mpg mpg added needs-work 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 Oct 24, 2025
mpg
mpg previously approved these changes Oct 24, 2025
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!

@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 24, 2025
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

I apologize for the last minute change, but I think I've found something worth to be fixed

CMakeLists.txt Outdated
# 3. Run framework/scripts/lcov.sh to generate an HTML report.
ADD_CUSTOM_TARGET(lcov
COMMAND scripts/lcov.sh
COMMAND framework/scripts/lcov.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Despite my previous approval I think I would suggest a change here. Can we make the framework path a bit more generic using MBEDTLS_FRAMEWORK_DIR as you did in the development branch PR?

@mpg mpg added the needs-ci Needs to pass CI tests label Nov 4, 2025
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 other than the point already noted by Valerio.

@mpg mpg added needs-work and removed needs-ci Needs to pass CI tests labels Nov 4, 2025
@davidhorstmann-arm
Copy link
Contributor

I have cherry-picked the change of framework -> ${MBEDTLS_FRAMEWORK_DIR} from #10450. This should address everything.

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 approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests and removed needs-work needs-review Every commit must be reviewed by at least two team members, labels Nov 4, 2025
@mpg
Copy link
Contributor

mpg commented Nov 6, 2025

The release job succeeded, removing 'needs-ci'.

@mpg mpg removed the needs-ci Needs to pass CI tests label Nov 6, 2025
@mpg mpg added this pull request to the merge queue Nov 6, 2025
Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 1d0ccfa Nov 6, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants