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

[BACKPORT 2.28] tests: add a test for pkg-config files #9175

Open
wants to merge 1 commit into
base: mbedtls-2.28
Choose a base branch
from
Open

[BACKPORT 2.28] tests: add a test for pkg-config files #9175

wants to merge 1 commit into from
+43 −0

Conversation

billatarm
Copy link
Contributor

@billatarm billatarm commented May 23, 2024 •
edited
Loading

Backport of #8988

Add a test that does some basic validation of the pkg-config files.

Note: this is a port of https://github.com/Mbed-TLS/mbedtls/pull/8988
but is moved into a different test component so cmake shared
infrastucture isn't needed.

Example run:
./tests/scripts/all.sh test_cmake_shared
******************************************************************
* test_cmake_shared: build/test: cmake shared
* Wed May 29 18:41:19 UTC 2024
******************************************************************
cmake -DUSE_SHARED_MBEDTLS_LIBRARY=On .
make
<snip>
testing package config file: mbedtls ... passed
testing package config file: mbedx509 ... passed
testing package config file: mbedcrypto ... passed
make clean

PR checklist

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

  • changelog provided, or not required (Tests only, feature was previously added)
  • 3.6 backport done, or not required (This is the 2.28 backport)
  • 2.28 backport done, or not required (This is the 2.28 backport)
  • tests provided , or not required

@billatarm billatarm mentioned this pull request May 23, 2024
Open
4 tasks
@paul-elliott-arm paul-elliott-arm self-assigned this May 24, 2024
@paul-elliott-arm paul-elliott-arm added enhancement needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels May 24, 2024
@gilles-peskine-arm gilles-peskine-arm 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 May 29, 2024
@gilles-peskine-arm
Copy link
Contributor

I'm afraid there are build failures.

@billatarm
Copy link
Contributor Author

I'm afraid there are build failures.

Yeah I've been looking into them to reproduce.

@billatarm
Copy link
Contributor Author

I'm afraid there are build failures.

Yeah I've been looking into them to reproduce.

Fun its a cmake version issue.

@billatarm
Copy link
Contributor Author

I'm afraid there are build failures.

Yeah I've been looking into them to reproduce.

Fun its a cmake version issue.

The easiest solution was to just move this test into a different component rather than untangling the nightmare that would be needed for cmake_as_a_package related stuff.

@gilles-peskine-arm
Copy link
Contributor

Fun its a cmake version issue.

As a reminder, Mbed TLS 2.28 claims to support CMake 2.8.12, but our CI only has CMake 3.5.1 and above. New features or test-only code don't need to support ancient CMake, but they must not break existing build systems that uses the basic features.

tom-cosgrove-arm
tom-cosgrove-arm approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

tom-cosgrove-arm
tom-cosgrove-arm requested changes Jun 3, 2024
View reviewed changes
tests/scripts/all.sh Outdated
cmake -DUSE_SHARED_MBEDTLS_LIBRARY=On .
make
ldd programs/util/strerror | grep libmbedcrypto
make test
programs/test/dlopen_demo.sh
PKG_CONFIG_PATH="${root_dir}/pkgconfig" ${root_dir}/tests/scripts/pkgconfig.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is failing CI on FreeBSD, so shouldn't be done there I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it should be skipped there, I'll have to figure out how to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I have it skipping non-linux systems now for this port, lets see if the CI is happy

@paul-elliott-arm
Copy link
Member

Everything looks good apart from one thing:

Tabs present: tests/scripts/all.sh: 2994

@billatarm
Copy link
Contributor Author

Everything looks good apart from one thing:

Tabs present: tests/scripts/all.sh: 2994

have that fixed locally and didn't push

@billatarm
tests: add a test for pkg-config files
937c7c5 
Add a test that does some basic validation of the pkg-config files.

Note: this is a port of Mbed-TLS#8988
but is moved into a different test component so cmake shared
infrastucture isn't needed.

Note this port of the patch detects the OS and skips so things like
freebsd do not fail.

Example run:
./tests/scripts/all.sh test_cmake_shared
******************************************************************
* test_cmake_shared: build/test: cmake shared
* Wed May 29 18:41:19 UTC 2024
******************************************************************
cmake -DUSE_SHARED_MBEDTLS_LIBRARY=On .
make
<snip>
testing package config file: mbedtls ... passed
testing package config file: mbedx509 ... passed
testing package config file: mbedcrypto ... passed
make clean

Signed-off-by: Bill Roberts <bill.roberts@arm.com>
@gilles-peskine-arm gilles-peskine-arm changed the title [BACKPORT 2.28] add pc test [BACKPORT 2.28] tests: add a test for pkg-config files Jun 11, 2024
@gilles-peskine-arm
Copy link
Contributor

Hi @billatarm, could you please push the fixed version? Hopefully that would give us a passing CI run and then we could merge the pull request.

@billatarm
Copy link
Contributor Author

Hi @billatarm, could you please push the fixed version? Hopefully that would give us a passing CI run and then we could merge the pull request.

Sorry about that, I thought I did, but it was the wrong destination branch.

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Jul 5, 2024
@gilles-peskine-arm gilles-peskine-arm added the needs-reviewer This PR needs someone to pick it up for review label Jul 5, 2024
@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jul 5, 2024
paul-elliott-arm
paul-elliott-arm approved these changes Jul 5, 2024
View reviewed changes
Copy link
Member

@paul-elliott-arm paul-elliott-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

tom-cosgrove-arm
tom-cosgrove-arm reviewed Jul 8, 2024
View reviewed changes
tests/scripts/all.sh
cmake -DUSE_SHARED_MBEDTLS_LIBRARY=On .
make
ldd programs/util/strerror | grep libmbedcrypto
make test
programs/test/dlopen_demo.sh
if [[ "$OSTYPE" == linux* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

So why don't we need something like this in the original PR? If it's just because it so happens that we don't run that test on non-Linux at the moment, we are setting ourselves up for a regression later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, and that seems right AFAICT. I can port this logic onto 3.6 and development branches if you'd like?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm requested changes

@paul-elliott-arm paul-elliott-arm paul-elliott-arm approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@paul-elliott-arm paul-elliott-arm

Labels
enhancement needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Non-roadmap pull requests
Status: Has Approval
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@billatarm @gilles-peskine-arm @paul-elliott-arm @tom-cosgrove-arm