-
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
Add Corrade 2020.06 #2109
Add Corrade 2020.06 #2109
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@danimtb Thanks, just did that! |
Still waiting for early access. |
This comment has been minimized.
This comment has been minimized.
… values, and wrong names for 'Visual Studio compatibility' definitions).
…etup correct preprocessor defines) for consumers.
1ee937f
to
4499dc4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
add_cmake_option("BUILD_STATIC", not self.options.shared) | ||
add_cmake_option("BUILD_STATIC_PIC", not self.options.shared and self.options.get_safe("fPIC") == True) |
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.
add_cmake_option("BUILD_STATIC", not self.options.shared) | |
add_cmake_option("BUILD_STATIC_PIC", not self.options.shared and self.options.get_safe("fPIC") == True) | |
self._cmake.definitions["BUILD_STATIC"] = not self.options.shared | |
self._cmake.definitions["BUILD_STATIC_PIC"] not self.options.shared and self.options.get_safe("fPIC", False) |
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.
Why not reuse the add_cmake_option(...)
method?
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.
keep it simple and readable like other recipes. you don't need an extra method for a simple logic.
self._cmake.definitions["MSVC2015_COMPATIBILITY"] = "ON" if self.settings.compiler.version == "14" else "OFF" | ||
self._cmake.definitions["MSVC2017_COMPATIBILITY"] = "ON" if self.settings.compiler.version == "15" else "OFF" | ||
self._cmake.definitions["MSVC2019_COMPATIBILITY"] = "ON" if self.settings.compiler.version == "16" else "OFF" |
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.
self._cmake.definitions["MSVC2015_COMPATIBILITY"] = "ON" if self.settings.compiler.version == "14" else "OFF" | |
self._cmake.definitions["MSVC2017_COMPATIBILITY"] = "ON" if self.settings.compiler.version == "15" else "OFF" | |
self._cmake.definitions["MSVC2019_COMPATIBILITY"] = "ON" if self.settings.compiler.version == "16" else "OFF" | |
self._cmake.definitions["MSVC2015_COMPATIBILITY"] = self.settings.compiler.version | |
self._cmake.definitions["MSVC2017_COMPATIBILITY"] = self.settings.compiler.version | |
self._cmake.definitions["MSVC2019_COMPATIBILITY"] = self.settings.compiler.version == "16" |
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 happened to == "14"
and == "15"
?
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.
Also I could've reused add_cmake_option(...)
here as well.
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.
again, don't need an extra method for a very simple logic.
self.copy("CorradeLibSuffix.cmake", src=share_cmake, dst=os.path.join(self.package_folder, "lib", "cmake", "Corrade")) | ||
self.copy("FindCorrade.cmake", src=share_cmake, dst=os.path.join(self.package_folder, "lib", "cmake", "Corrade")) |
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 macro do you need from Corrade? Conan CmakeFindPackage generator should be enough. If there is no specific macro there, please remove FindCorrade/CorradeConfig
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.
All the different options passed to CMake when building corrade (that are relevant for consumers) gets written to a configure.h
which is packaged along all other includes. This file is then parsed by FindCorrade.cmake
. The alternative is to extract the necessary pieces and add to a CMake script that's part of the recipe (if I'm correct it's only the parsing of configure.h
that's relevant for consumers).
Or alternatively mimic it by directly in package_info()
setup all these different preprocessor directives. Though this is even more error prone than above, 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.
I can't find a real evidence of their usage as cmake definitions. For instance, BUILD_STATIC will be "renamed" to CORRADE_BUILD_STATIC, and only the unit tests are using it. The defines in configure.h are okay, they will be processed by the consumer. Do you see a real usage for them?
https://github.com/mosra/corrade/search?q=CORRADE_UTILITY_USE_ANSI_COLORS
https://github.com/mosra/corrade/search?q=CORRADE_BUILD_STATIC
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.
There were some comments here regarding some macros defined (that one is in UseCorrade.cmake
though). But yeah, FindCorrade.cmake
should not be included, but instead just the bare minimal required by consumers.
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
This comment has been minimized.
This comment has been minimized.
self.cpp_info.build_modules.append(os.path.join("lib", "cmake", "Corrade", "CorradeLibSuffix.cmake")) | ||
self.cpp_info.build_modules.append(os.path.join("lib", "cmake", "Corrade", "CMakeLists.txt")) |
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.
self.cpp_info.build_modules.append(os.path.join("lib", "cmake", "Corrade", "CMakeLists.txt")) |
CMakeLists.txt can be removed, it only aggregate other cmake files, Conan will pass them as build modules
self.cpp_info.builddirs = [os.path.join(self.package_folder, "lib", "cmake", "Corrade")] | ||
|
||
bindir = os.path.join(self.package_folder, "bin") | ||
self.output.info("Appending PATH environment variable: {}".format(bindir)) |
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.
Keep the output info.
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.
So which is it: Keep it, or remove it..?
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.
keep.
self.copy("CorradeLibSuffix.cmake", src=share_cmake, dst=os.path.join(self.package_folder, "lib", "cmake", "Corrade")) | ||
self.copy("FindCorrade.cmake", src=share_cmake, dst=os.path.join(self.package_folder, "lib", "cmake", "Corrade")) |
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.
I can't find a real evidence of their usage as cmake definitions. For instance, BUILD_STATIC will be "renamed" to CORRADE_BUILD_STATIC, and only the unit tests are using it. The defines in configure.h are okay, they will be processed by the consumer. Do you see a real usage for them?
https://github.com/mosra/corrade/search?q=CORRADE_UTILITY_USE_ANSI_COLORS
https://github.com/mosra/corrade/search?q=CORRADE_BUILD_STATIC
Please, don't forget to sign the CLA #2109 (comment) |
This comment has been minimized.
This comment has been minimized.
The hooks message is incomplete:
it does not show the actual files |
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. |
@helmesjo You might want to close the PR for ~10 seconds and re-open it then to re-trigger CI |
Some configurations of 'corrade/2019.10' failed in build 10 (
|
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. |
Also, thank you for your work... would be awesome to see both Corrade and Magnum popping up in Conan one time. |
hey @helmesjo, can you check the CLA issue? That will help us to go forward with this PR as we can make it work if you don't have the time. Thank you |
This comment has been minimized.
This comment has been minimized.
no, everything is fine for you in #4, just check the CLA check here #2109 (comment) |
Ah, forgot about that one. Alright, signed (with the assumption that it is clear that nothing with this PR violates any license provided by corrade or it's dependencies ;) ). |
Some configurations of 'corrade/2020.06' failed in build 12 (
|
Failure in build 13 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
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. |
Bad bot! |
Specify library name and version: corrade/2020.06
Add v2020.06
Fix
To use Corrade with MSVC 20XX, build it with MSVC20XX_COMPATIBILITY enabled
issue for consumers.Fixed so that consumers get the cmake scripts provided by corrade (used to read configs and setup correct preprocessor defines).
I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.