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

2.28 only: Fix the build without check_config.h (inclusion of limits.h) #9153

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented May 17, 2024

Including mbedtls/check_config.h from mbedtls/config.h is optional. If done, limits.h gets included. If not done, we were missing the inclusion of limits.h in several source files. Fix this and add a test build that doesn't include mbedtls/check_config.h. Fixes #9152.

Not applicable to ≥3.0 because there check_config.h has no side effects (they're in build_info.h).

PR checklist

  • changelog provided
  • development N/A
  • 3.6 backport N/A
  • tests provided

Sorry, something went wrong.

Including `mbedtls/check_config.h` from `mbedtls/config.h` is optional. If
done, `limits.h` gets included. If not done, we were missing the inclusion
of `limits.h` in several source files. Fix this and add a test build that
doesn't include `mbedtls/check_config.h`.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels May 17, 2024
@paul-elliott-arm paul-elliott-arm self-requested a review May 29, 2024 08:51
@ronald-cron-arm ronald-cron-arm self-requested a review May 29, 2024 09:19
@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label May 29, 2024
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.

This looks good to me. But isn't it the case that we should also add the "#include <limits.h>" in 3.6 and development even if there is yet no build issue there.

@gilles-peskine-arm
Copy link
Contributor Author

@ronald-cron-arm Since 3.0, #include <limits.h> is automatically done everywhere through build_info.h. I don't see the point in making a patch for what is technically a convention violation, but a harmless one and just one of many,

@ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm Since 3.0, #include <limits.h> is automatically done everywhere through build_info.h. I don't see the point in making a patch for what is technically a convention violation, but a harmless one and just one of many,

okay I understand the context, fine by me.

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

@paul-elliott-arm paul-elliott-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 May 29, 2024
@gilles-peskine-arm gilles-peskine-arm added this pull request to the merge queue May 29, 2024
Merged via the queue into Mbed-TLS:mbedtls-2.28 with commit 9ebf9aa May 29, 2024
6 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 bug component-platform Portability layer and build scripts priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants