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

Improve AWS-LC tests #863

Merged
merged 3 commits into from
Mar 3, 2025
Merged

Improve AWS-LC tests #863

merged 3 commits into from
Mar 3, 2025

Conversation

junaruga
Copy link
Member

@junaruga junaruga commented Feb 25, 2025 •
edited
Loading

Cc: @samuel40791765

This PR is on top of another PR #862. So, please review the #862 first. Essentially 3 commits. This PR is to improve tests related to the AWS-LC non-FIPS case.

The 1st commit is to disable the cache for the AWS-LC case on CI. In the current logic of the cache key, we cannot enable the cache to get the latest version of the AWS-LC.

The 2nd commit is to specify the library directory as "lib" explicitly. The logic of compiling the AWS-LC is useful to see how to compile it. Because I saw the "lib64" directory was created without the -DCMAKE_INSTALL_LIBDIR=lib option on Fedora Linux 41.

The 3rd commit is to remove meaningless aws_lc?. As I mentioned at #860 (comment), we can safely use the method omit_on_fips and omit_on_non_fips in AWS-LC cases. If the aws_lc? exists in the test_new_break_on_non_fips test to run on the AWS-LC FIPS case, that confuses me. I don't want to do it.

rhenium
rhenium reviewed Feb 26, 2025
View reviewed changes
@rhenium
Copy link
Member

rhenium commented Feb 26, 2025 •
edited
Loading

The 3rd commit is to remove meaningless aws_lc?. As I mentioned at #860 (comment), we can safely use the method omit_on_fips and omit_on_non_fips in AWS-LC cases. If the aws_lc? exists in the test_new_break_on_non_fips test to run on the AWS-LC FIPS case, that confuses me. I don't want to do it.

I don't think these changes are correct. All test cases currently pass with AWS-LC main built with -DFIPS=1. Applying this patch breaks this.

junaruga added 2 commits March 3, 2025 14:54
@junaruga
CI: Exclude aws-lc-latest not to use cache.
7517bd3 
Because aws-lc-latest is a rolling release. If the cache is enabled, CI just
downloads and compiles only at once, then will never do again.
@junaruga
CI: AWS-LC: Specify the library directory lib.
e6758ab 
Added the `-DCMAKE_INSTALL_LIBDIR=lib` option to specify the library directory
explicitly. While the CI AWS-LC case creates the "lib" directory without the
option, I observed the `lib64` directory was created on my local environment.
So, this change is useful to provide information to create a local environment.
@junaruga junaruga force-pushed the wip/fix-aws-lc-test branch from b07f6aa to a72e513 Compare March 3, 2025 13:55
@junaruga
Copy link
Member Author

junaruga commented Mar 3, 2025 •
edited
Loading

The 3rd commit is to remove meaningless aws_lc?. As I mentioned at #860 (comment), we can safely use the method omit_on_fips and omit_on_non_fips in AWS-LC cases. If the aws_lc? exists in the test_new_break_on_non_fips test to run on the AWS-LC FIPS case, that confuses me. I don't want to do it.

I don't think these changes are correct. All test cases currently pass with AWS-LC main built with -DFIPS=1. Applying this patch breaks this.

I rebaesd the PR again. The 1st and 2nd commit are same. The 4th commit is to rename the tests a bit to reflect the fact.

If we can pass the fips tests in AWS-LC -DFIPS=1, shall we add the case to the CI now? Otherwise we may break the tests in the case unintentionally. So, I added the AWS-LC FIPS on the 3rd commit.

What do you think?

@rhenium
Copy link
Member

rhenium commented Mar 3, 2025

I'd prefer not to slow down our CI unless there's a real need. We can support it on a best-effort basis. There are so many different configurations that can break our test suite in different ways (latest being 6d0ea81 from 3 days ago), but we can't realistically test all. IMO this falls into this category.

@junaruga
test_pkey_dh.rb: Merge 2 test_new_break tests to one test.
446ff3e 
Merge 2 test_new_break tests to one test because it's easy to maintain the test.
@junaruga junaruga force-pushed the wip/fix-aws-lc-test branch from a72e513 to 446ff3e Compare March 3, 2025 15:10
@junaruga
Copy link
Member Author

junaruga commented Mar 3, 2025

I'd prefer not to slow down our CI unless there's a real need. We can support it on a best-effort basis. There are so many different configurations that can break our test suite in different ways (latest being 6d0ea81 from 3 days ago), but we can't realistically test all. IMO this falls into this category.

All right. That makes sense. I removed the previous 3rd commit about adding the AWS-LC FIPS case.

rhenium
rhenium approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member

@rhenium rhenium 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, thanks!

@rhenium rhenium merged commit 97911e6 into ruby:master Mar 3, 2025
54 checks passed
@junaruga
Copy link
Member Author

junaruga commented Mar 4, 2025

Looks good to me, thanks!

Thanks for your review!

@junaruga junaruga deleted the wip/fix-aws-lc-test branch March 4, 2025 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhenium rhenium rhenium approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@junaruga @rhenium