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

add KB-H078 in conan-center to test that system libs are all lowercase if host OS is Windows #480

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Jan 22, 2023

closes #479

AbrilRBS
AbrilRBS previously approved these changes Jan 23, 2023
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Nice!

hooks/conan-center.py Outdated Show resolved Hide resolved
Co-authored-by: Rubén Rincón Blanco <[email protected]>
hooks/conan-center.py Outdated Show resolved Hide resolved
@uilianries
Copy link
Member

It could be moved to pre_export, as you are actually checking system_libs only. Early failure is better than needing to wait all build step first.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Jan 23, 2023

It could be moved to pre_export, as you are actually checking system_libs only. Early failure is better than needing to wait all build step first.

It can't work, cpp_info.system_libs is not populated in conanfile object at pre_export time, so you would have to parse conanfile and understand branching and all the logic (with maybe lot of indirection). Moreover some recipes populate cpp_info.system_libs from a file created at package time (like abseil).

@uilianries
Copy link
Member

It can't work, cpp_info.system_libs is not populated in conanfile object at pre_export time, so you would have to parse conanfile and understand branching and all the logic (with maybe lot of indirection). Moreover some recipes populate cpp_info.system_libs from a file created at package time (like abseil).

Makes sense, parsing the file content is too much.

@uilianries
Copy link
Member

Doing a quick search on master branch, some recipes need to be updated before merging this hook, or it will fail for any new PR:

find recipes/ -name conanfile.py -exec grep system_libs {} + | grep -e '[A-Z]'
proj/all/conanfile.py:                self.cpp_info.components["projlib"].system_libs.append("Ole32")
sentry-native/all/conanfile.py:                self.cpp_info.system_libs.append("Version")
qt/5.x.x/conanfile.py:                    self.cpp_info.components["qtWidgets"].system_libs.append("UxTheme")
glu/all/conanfile.py:            self.cpp_info.system_libs = ["Glu32"]
taocpp-taopq/all/conanfile.py:            self.cpp_info.components["_taocpp-taopq"].system_libs = ["Ws2_32"]
openal/all/conanfile.py:            self.cpp_info.system_libs.extend(["winmm", "ole32", "shell32", "User32"])
baical-p7/all/conanfile.py:            self.cpp_info.components["p7"].system_libs .append("Ws2_32")
aeron/all/conanfile.py:            self.cpp_info.system_libs = ["wsock32", "ws2_32", "Iphlpapi"]
ixwebsocket/all/conanfile.py:                self.cpp_info.system_libs.append("Crypt32")

On the other hand, the hook could be changed to warning level.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Jan 23, 2023

Number of recipes is reasonable. They can be fixed quickly. I've already submited a PR to fix baical-p7 & glu.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Jan 23, 2023

I've submitted PRs for recipes you've listed.
There are 3 other recipes to fix: c-client, ffmpeg, cyclonedds. I've also submitted PRs for them.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Feb 7, 2023

@uilianries @RubenRBS I think that now, the impact on existing CCI recipes is quite limited, most of them have been fixed. There is still qt & sentry-native, and I have a PR for each of them. I've also seen a strange recipe archicad-apidevkit with uppercase windows system libs (but also propagated if macOS, so weird).
Anyway it's a straightforward fix for contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a post_package_info hook to check that all libs listed in system_libs are lower case if host os is Windows
4 participants