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

[openssl] Don't switch from jom to nmake, fix cflags #23726

Merged
merged 5 commits into from
Mar 28, 2022

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Mar 23, 2022

@dg0yt dg0yt changed the title [openssl] Don't switch from jom to nmake [openssl] Don't switch from jom to nmake, fix cflags Mar 23, 2022
@LilyWangLL LilyWangLL removed their assignment Mar 23, 2022
@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Mar 23, 2022
Comment on lines 64 to 66
set(CFLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${BUILD_TYPE}}")
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
set(CFLAGS "-Wno-error=unused-command-line-argument ${CMAKE_C_FLAGS}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set(CFLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${BUILD_TYPE}}")
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
set(CFLAGS "-Wno-error=unused-command-line-argument ${CMAKE_C_FLAGS}")
set(CFLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${BUILD_TYPE}}")
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
set(CFLAGS "${CFLAGS} -Wno-error=unused-command-line-argument")

Otherwise the CFLAGS value above will be clobbered if the compiler is Clang (like when building for Android, for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. And for consistency, we should also switch to CMAKE_C_COMPILER_ID. (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FTR CMAKE_CXX_COMPILER_ID was never initialized because of project(openssl C)...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's a very good catch - yes, we should definitely be using CMAKE_C_COMPILER_ID.

@dg0yt dg0yt marked this pull request as ready for review March 24, 2022 08:17
@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Mar 25, 2022
@strega-nil-ms
Copy link
Contributor

Thanks @dg0yt!

@strega-nil-ms strega-nil-ms merged commit 3b3bd42 into microsoft:master Mar 28, 2022
@dg0yt dg0yt deleted the openssl branch March 29, 2022 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[openssl] respect build configuration when building for unix
6 participants