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

Fix compilation with MinGW32 #4211

Merged
merged 5 commits into from
May 16, 2022
Merged

Fix compilation with MinGW32 #4211

merged 5 commits into from
May 16, 2022

Conversation

ccawley2011
Copy link
Contributor

@ccawley2011 ccawley2011 commented Mar 8, 2021 •
edited by daverodgman
Loading

Description

Fixes #2087. MinGW-w64 should be unaffected by this change.

Status

READY

Requires Backporting

No - not worth backporting to an LTS

Migrations

NO

Todos

  • Tests
  • Changelog updated

@daverodgman daverodgman added enhancement bug Community 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 size-s Estimated task size: small (~2d) and removed enhancement labels Mar 15, 2021
@chris-jones-arm chris-jones-arm self-requested a review April 23, 2021 14:31
chris-jones-arm
chris-jones-arm reviewed Apr 23, 2021
View reviewed changes
Copy link
Contributor

@chris-jones-arm chris-jones-arm left a comment

Choose a reason for hiding this comment

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

A few things to address here including a bug (although it was not created by you). Unfortunately my knowledge is limited when it comes to mingw and windows builds so if you or anyone else can provide some suggestions that would be appreciated.

library/platform_util.c Outdated Show resolved Hide resolved
library/platform_util.c Outdated Show resolved Hide resolved
tests/suites/helpers.function Show resolved Hide resolved
@daverodgman daverodgman added good-first-issue Good for newcomers needs-adoption stalled PR that someone should pick up and complete labels Jul 6, 2021
@daverodgman daverodgman added the historical-reviewing Currently reviewing (for legacy PR/issues) label May 6, 2022
@daverodgman daverodgman self-assigned this May 6, 2022
tom-cosgrove-arm
tom-cosgrove-arm reviewed May 6, 2022
View reviewed changes
library/platform_util.c Outdated
#define PLATFORM_UTIL_USE_GMTIME
#endif /* ! ( defined(_WIN32) && !defined(EFIX64) && !defined(EFI32) ) */
#endif /* ! ( defined(_WIN32) && !defined(EFIX64) && !defined(EFI32) ) || \
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just remove the cruft after #endif - the #if is so close this is unnecessary

@daverodgman daverodgman removed needs-review Every commit must be reviewed by at least two team members, Product Backlog needs-reviewer This PR needs someone to pick it up for review needs-adoption stalled PR that someone should pick up and complete labels May 6, 2022
@daverodgman
Copy link
Contributor

Added fix for #4015

I think all review comments have been addressed, so should be ready for review.

@daverodgman daverodgman removed needs-work good-first-issue Good for newcomers labels May 6, 2022
@daverodgman daverodgman added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels May 6, 2022
@gilles-peskine-arm
Copy link
Contributor

If this is a bug fix does it need backporting?

The primary audience of long-term support branches is people who are already using Mbed TLS in a particular setting, and want it to keep working. The focus is on problems that these people always had, but didn't know they were having, such as bad behavior on certain inputs which had always been possible but had never been noticed.

In terms of platform portability, this means that fixing an incompatibility with a newer version of an already-supported platform is important — we want users to be able to upgrade their compiler or operating system and keep running their application. Fixing an incompatibility that had always existed is less important since it only affects users who are doing something new (porting their application to a new platform).

Since this patch is of the latter kind, and there's some risk that it would break a platform where the library does work, I'm inclined to not backport.

ccawley2011 and others added 4 commits May 10, 2022 13:46
@ccawley2011 @daverodgman
Fix compilation with MinGW32
ea5496c 
Signed-off-by: Cameron Cawley <[email protected]>
@daverodgman
Remove redundant comment
ad8dc48 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Add changelog entry
3383e43 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Handle platform differences in gmtime_s
4bfb007 
MSVC and C11 specify different arguments and return value
meaning for gmtime_s.

Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Copy link
Contributor

Rebased against development to try and address CI issue

@daverodgman daverodgman added needs-ci Needs to pass CI tests 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 May 10, 2022
@daverodgman daverodgman added the priority-medium Medium priority - this can be reviewed as time permits label May 11, 2022
@daverodgman
Add comment
8b65420 
Signed-off-by: Dave Rodgman <[email protected]>
chris-jones-arm
chris-jones-arm approved these changes May 16, 2022
View reviewed changes
Copy link
Contributor

@chris-jones-arm chris-jones-arm left a comment

Choose a reason for hiding this comment

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

Looks like all the comments are addressed so LGTM.

library/platform_util.c Outdated Show resolved Hide resolved
tests/suites/helpers.function Show resolved Hide resolved
@daverodgman daverodgman 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 16, 2022
@gilles-peskine-arm gilles-peskine-arm merged commit 9b7e296 into Mbed-TLS:development May 16, 2022
@daverodgman daverodgman mentioned this pull request May 16, 2022
Closed
@ccawley2011 ccawley2011 deleted the mingw branch May 16, 2022 11:11
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

@daverodgman daverodgman daverodgman left review comments

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@chris-jones-arm chris-jones-arm chris-jones-arm approved these changes

Assignees

@daverodgman daverodgman

Labels
approved Design and code approved - may be waiting for CI or backports bug historical-reviewing Currently reviewing (for legacy PR/issues) priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mingw doesn't have gmtime_s()
6 participants
@ccawley2011 @daverodgman @tom-cosgrove-arm @gilles-peskine-arm @chris-jones-arm @bensze01