-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fixed windows builds #639
fixed windows builds #639
Conversation
af30126
to
9b0e2fb
Compare
11ef913
to
efc341d
Compare
@@ -93,13 +93,13 @@ def glog_library(namespace = "google", with_gflags = 1, **kwargs): | |||
] | |||
|
|||
windows_only_copts = [ | |||
"-DGLOG_NO_ABBREVIATED_SEVERITIES", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it could break users. Can you add a #define to port.h instead, along the lines of the docs? https://github.com/google/glog#notes-for-windows-users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thoughts, as it's in copts rather than defines, it's unlike to affect users but it would still be cleaner to use the approach in the docs IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that the define is transitive, i.e., it is forwarded to library's consumer? Either way, CMake always defines GLOG_NO_ABBREVIATED_SEVERITIES
on Windows to avoid conflicts due to ERROR
:
Lines 598 to 600 in a79416b
if (WIN32) | |
target_compile_definitions (glog PUBLIC GLOG_NO_ABBREVIATED_SEVERITIES) | |
endif (WIN32) |
If I recall correctly, on Windows glog without GLOG_NO_ABBREVIATED_SEVERITIES
is unusable anyway once windows.h
is included (which is in many cases true). Regardless, we should probably sync the approach taken by both build systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defines are transitive (useful when needed in public headers) but copts are not. If CMake does it too then this LGTM, thanks!
No description provided.