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.

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

Improve handling of CFLAGS #712

Merged
merged 11 commits into from
Mar 10, 2025
Merged

Conversation

justsmth
Copy link
Contributor

@justsmth justsmth commented Feb 27, 2025

Issues

Addresses:

Description of changes:

  • Previously we "harvested" the compiler options used by cc to populate CFLAGS. This is unnecessary, as it's also done by cmake-rs. This change identifies any supplementary compiler options as BuildOption values. These can be applied to either a cc or cmake build.
  • cmake-rs strips optimization and debug options out of the CFLAGS environment variable. This can cause build failure when the build environment produces warnings due to lack of these optimizations.
  • Aligning with the change made in AWS-LC, we're also bumping our CMake minimum version for v3.5.
  • Fix for macro definitions applied after compiler checks.
  • Defer the setting of CMAKE_BUILD_TYPE to cmake-rs

Callouts

  • A test replicating the scenario in #710 is being prepared in #721.
  • rustls integ test failures are being address in #717.

Testing

  • Added test reproducing the scenario from #707
  • Added test for various versions of CMake. #716

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.86%. Comparing base (c358484) to head (741f80b).
Report is 181 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #712      +/-   ##
==========================================
- Coverage   95.80%   92.86%   -2.94%     
==========================================
  Files          61       70       +9     
  Lines        8143     9561    +1418     
  Branches        0     9561    +9561     
==========================================
+ Hits         7801     8879    +1078     
- Misses        342      413      +71     
- Partials        0      269     +269     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@justsmth justsmth force-pushed the improve-cflags-handling branch 8 times, most recently from 0bec6fa to 326d900 Compare March 5, 2025 13:44
@justsmth justsmth force-pushed the improve-cflags-handling branch 16 times, most recently from 27383e2 to 70ee030 Compare March 6, 2025 22:54
@justsmth justsmth marked this pull request as ready for review March 6, 2025 23:12
@justsmth justsmth requested a review from a team as a code owner March 6, 2025 23:12
@justsmth justsmth changed the title [DRAFT] Improve handling of CFLAGS Improve handling of CFLAGS Mar 6, 2025
@justsmth justsmth force-pushed the improve-cflags-handling branch 4 times, most recently from 9187eac to fe7a32f Compare March 7, 2025 17:39
@justsmth justsmth force-pushed the improve-cflags-handling branch 2 times, most recently from 1235a7c to 52f4957 Compare March 7, 2025 21:15
@justsmth justsmth force-pushed the improve-cflags-handling branch from 52f4957 to 741f80b Compare March 10, 2025 11:12
@justsmth justsmth merged commit 21f1861 into aws:main Mar 10, 2025
258 of 268 checks passed
@justsmth justsmth deleted the improve-cflags-handling branch March 10, 2025 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants