-
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
nvcloth: migrate to Conan v2 #18626
nvcloth: migrate to Conan v2 #18626
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.
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.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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. |
Conan v1 pipeline ✔️All green in build 8 (
Conan v2 pipeline ✔️
All green in build 7 (
|
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.
@jstzwj thank you for your PR! I'm concerned about the shared option managing. I need more information about this change first.
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.
We no longer use the CMake wrapper, please, use Cmaketoolchain instead: https://docs.conan.io/2/reference/tools/cmake/cmaketoolchain.html
@@ -5,8 +5,8 @@ sources: | |||
patches: | |||
"1.1.6": | |||
- patch_file: "patches/0001-PsAllocator-include-typeinfo.patch" | |||
base_path: "source_subfolder" | |||
base_path: "." |
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.
base_path: "." |
It's not needed anymore with new tools like apply_conandata_patch
for patch in self.conan_data.get("patches", {}).get(self.version, []): | ||
self.copy(patch["patch_file"]) | ||
|
||
copy(self, "CMakeLists.txt", self.recipe_folder, os.path.join(self.export_sources_folder, "src")) |
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.
copy(self, "CMakeLists.txt", self.recipe_folder, os.path.join(self.export_sources_folder, "src")) |
def configure(self): | ||
if self.settings.os in ["Windows", "Macos"]: | ||
# static builds are not supported | ||
self.options.rm_safe("shared") |
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's not clear how is it not supported. I see the default option was shared by default, but where can I find an evidence of this breaking change?
if self.settings.os in ["Windows", "Macos"]: | ||
# static builds are not supported | ||
self.options.rm_safe("shared") | ||
self.package_type = "shared-library" |
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.package_type = "shared-library" |
Please, do not change the package-type in case both type are supported in any OS.
self._remove_samples() | ||
|
||
env = Environment() | ||
env.define("GW_DEPS_ROOT", os.path.abspath(self.source_folder)) |
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 cant you pass it directly to CMake as a variable?
self.cpp_info.includedirs = ['include'] | ||
self.cpp_info.libdirs = ['lib'] | ||
self.cpp_info.bindirs = ['bin'] | ||
|
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.includedirs = ['include'] | |
self.cpp_info.libdirs = ['lib'] | |
self.cpp_info.bindirs = ['bin'] |
These are configured already by default: https://docs.conan.io/2/reference/conanfile/methods/package_info.html#cpp-info-library-and-build-information
I detected other pull requests that are modifying nvcloth/1.1.6 recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
Specify library name and version: nvcloth/1.1.6
migrate to Conan v2