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

Modify lcov.sh to work in tf-psa-crypto as well #8395

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Modify lcov.sh to work in tf-psa-crypto as well #8395

merged 2 commits into from
Nov 2, 2023

Conversation

tom-daubney-arm
Copy link
Contributor

@tom-daubney-arm tom-daubney-arm commented Oct 19, 2023 •
edited by paul-elliott-arm
Loading

Description

This PR solves part one of Mbed-TLS/TF-PSA-Crypto#41.

Add repository detection (credit to davidhorstmann-arm for adding this in all.sh previously) and use repository detection to set the library directory and title variables.

I have tested this script locally against both mbedtls and tf-psa-crypto and the results were as expected.

Limitations: The modifications rely on the build directory being named "build" in tf-psa-crypto which may not always be the case. Perhaps this is ok for now, but I will document it. Otherwise perhaps we could supply the build directory name as a command line argument. Open to suggestions.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog not required (non user-visible change)
  • backport not required, new feature.
  • tests existing testing is sufficient.

Notes for the submitter

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

@tom-daubney-arm tom-daubney-arm added enhancement component-platform Portability layer and build scripts needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Oct 19, 2023
@tom-daubney-arm
Modify lcov.sh to work in tf-psa-crypto as well
11120f9 
Add repository detection (credit to davidhorstmann-arm
for adding this in all.sh previously) and use repository
detection to set the library directory and title
variables.

Signed-off-by: Thomas Daubney <thomas.daubney@arm.com>
@tom-daubney-arm tom-daubney-arm force-pushed the modify_lcov_script_tf_psa_crypto branch from 81407df to 11120f9 Compare October 19, 2023 14:55
@tom-daubney-arm tom-daubney-arm 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 and removed needs-ci Needs to pass CI tests labels Oct 20, 2023
ronald-cron-arm
ronald-cron-arm requested changes Oct 20, 2023
View reviewed changes
scripts/lcov.sh Outdated Show resolved Hide resolved
@tom-daubney-arm tom-daubney-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 20, 2023
ronald-cron-arm
ronald-cron-arm requested changes Oct 20, 2023
View reviewed changes
scripts/lcov.sh Outdated Show resolved Hide resolved
scripts/lcov.sh Outdated Show resolved Hide resolved
@gilles-peskine-arm
Copy link
Contributor

The modifications rely on the build directory being named "build" in tf-psa-crypto which may not always be the case

The script is supposed to be invoked from the build directory. It doesn't need to know the name of the build directory. The name of the build directory can be arbitrary; it's common to have multiple build directories with different configurations or compiler options.

@tom-daubney-arm
Copy link
Contributor Author

The modifications rely on the build directory being named "build" in tf-psa-crypto which may not always be the case

The script is supposed to be invoked from the build directory. It doesn't need to know the name of the build directory. The name of the build directory can be arbitrary; it's common to have multiple build directories with different configurations or compiler options.

Roger, thank you. I was confused over that but makes sense now. Will modify it.

@tom-daubney-arm
Make lcov.sh run from the build directory
c0ae569 
lcov.sh can now be called from any build directory and
also still works with in-place builds too.

Signed-off-by: Thomas Daubney <thomas.daubney@arm.com>
@tom-daubney-arm tom-daubney-arm added needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, needs-work and removed needs-work needs-ci Needs to pass CI tests labels Oct 23, 2023
@tom-daubney-arm
Copy link
Contributor Author

I've just noticed that the gendesc step of lcov.sh is failing on the psa repository, but not on mbedtls. I need to figure out why and push a change before this can be reviewed.

@tom-daubney-arm
Copy link
Contributor Author

I've just noticed that the gendesc step of lcov.sh is failing on the psa repository, but not on mbedtls. I need to figure out why and push a change before this can be reviewed.

The tests/Descriptions.txt needed copying over from mbedtls into tf-psa-crytpo. Once this was done the coverage report was produced correctly, so no modifications are required to this script and it can now be reviewed.

Thanks to @davidhorstmann-arm for helping me to get to the bottom of that.

@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Oct 31, 2023
ronald-cron-arm
ronald-cron-arm approved these changes Oct 31, 2023
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.

gabor-mezei-arm
gabor-mezei-arm approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-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

@gabor-mezei-arm gabor-mezei-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 2, 2023
@paul-elliott-arm paul-elliott-arm added this pull request to the merge queue Nov 2, 2023
Merged via the queue into Mbed-TLS:development with commit fc31cb2 Nov 2, 2023
1 check passed
@tom-daubney-arm tom-daubney-arm mentioned this pull request Nov 3, 2023
Merged
@mpg mpg mentioned this pull request Dec 4, 2024
Open
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 left review comments

@gabor-mezei-arm gabor-mezei-arm gabor-mezei-arm approved these changes

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

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts enhancement priority-high High priority - will be reviewed soon
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tom-daubney-arm @gilles-peskine-arm @ronald-cron-arm @gabor-mezei-arm @paul-elliott-arm