-
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
BugFix: Allow usage of clang+mingw with zlib #18283
Conversation
🤖 Beep Boop! This pull request is making changes to 'recipes/zlib//'. 👋 @Hopobcn you might be interested. 😉 |
This comment has been minimized.
This comment has been minimized.
I detected other pull requests that are modifying zlib/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
This comment has been minimized.
This comment has been minimized.
That was marked as stale before being seen to completion. It also doesn't address cygwin. |
This comment has been minimized.
This comment has been minimized.
@@ -29,7 +29,11 @@ class ZlibConan(ConanFile): | |||
|
|||
@property | |||
def _is_mingw(self): | |||
return self.settings.os == "Windows" and self.settings.compiler == "gcc" | |||
is_windows = self.settings.get_safe("os") == "Windows" |
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 feels like a lot of "work" that will result in false in probably most cases (I don't have numbers but I assume mingw has quite a bit lower market share compared to gcc and msvc"
I guess it would still be better to get the short circuit behaviour compared to the old code where it returns false as soon as some condition is reached that makes it false.
I see later it is used for "if windows then _is_mingw()" but still thinks that it can do a lot lees most of the time.
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 a copy of conan-io/conan#12678 which still hasn't been merged. If you have any issues with it I recommend posting them there.
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.
Am I alright to resolve this?
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 comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 3 (
Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future. See details:Sorry, the build is only launched for Access Request users. You can request access writing in this issue. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
Specify library name and version: lib/1.0
Without this change the following occurs:
This occurs because in zlib, you have a detection of mingw which was not correct. It didn't identify mingw when it is used with clang. I've added a fix for this.