-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
flac: ignore DLL_EXPORT define in export.h #23680
Conversation
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 2 (
Conan v2 pipeline ✔️
All green in build 2 ( |
@@ -56,6 +56,7 @@ def generate(self): | |||
tc.variables["BUILD_EXAMPLES"] = False | |||
tc.variables["BUILD_DOCS"] = False | |||
tc.variables["BUILD_TESTING"] = False | |||
tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0077"] = "NEW" |
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.
What specific error is it fixing? There is no information in the description.
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.
Added it just to be safe. It's almost boilerplate, so I did not see a need to point it out.
The project uses CMake version < 3.12. Without setting the policy, any option()
values in CMakeLists.txt might not be overridden by Conan. Even if it has worked ok without it so far, it's bug-prone and can easily become an issue in new releases. It also tends to cause issues silently, if it does.
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.
It makes sense, found similar fix in other places.
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.
LGTM
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.
Approving after going over this with @uilianries, thanks!
The Windows export header checks both
FLAC__NO_DLL
andDLL_EXPORT
, but the latter overly broadly named define is only set by autotools and we are using the CMake to build the project.Fixes the
unresolved external symbol __imp_FLAC__stream_decoder_new
etc errors due to conflictingDLL_EXPORT
define being set in