add aws-cdi-sdk 2.2.0 #7407

dvirtz · 2021-09-24T14:29:41Z

Specify library name and version: aws-cdi-sdk/2.2.0

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.
I've tried at least one configuration locally with the
conan-center hook activated.

dvirtz · 2021-09-25T18:54:35Z

@jgsogo @SSE4 @madebr
This package requires a non-default option (monitoring) of aws-sdk-cpp and thus fails to find a prebuilt binary.
Should I open a PR to change that default value to match this package?

SSE4 · 2021-09-26T07:43:00Z

@jgsogo @SSE4 @madebr
This package requires a non-default option (monitoring) of aws-sdk-cpp and thus fails to find a prebuilt binary.
Should I open a PR to change that default value to match this package?

I think so, if there is no other way, let's try to change the defaults of aws-sdk-cpp

dvirtz · 2021-09-30T19:07:22Z

@SSE4 this is not blocked any more

prince-chrismc · 2021-09-30T23:50:58Z

recipes/aws-cdi-sdk/all/conanfile.py

+        self.options["aws-sdk-cpp"].monitoring = True
+        self.options["aws-sdk-cpp"].shared = True
+        self.options["aws-libfabric"].shared = True


These can be overridden by consumers so if these are requirements then they should be moved to a validate method

prince-chrismc · 2021-09-30T23:54:25Z

recipes/aws-cdi-sdk/all/conanfile.py

+        # build aws-cpp-sdk-cdi
+        cmake = self._configure_cmake()
+        cmake.build()
+        cmake.install()


This should be called in the package method

Doing install in build breaks the following use case:

conan create . aws-cdi-sdk/2.2.0@ conan create . aws-cdi-sdk/2.2.0@ -kb

So you need to pass a CMAKE_INSTALL_PREFIX variable to cmake and add the --prefix= argument to autotools.

prince-chrismc · 2021-09-30T23:55:22Z

recipes/aws-cdi-sdk/all/conanfile.py

+        autotools = self._configure_autotools()
+        with tools.chdir(self._source_subfolder):
+            vars = autotools.vars
+            # configure autotools to find aws-cpp-sdk-cdi


Can you please explain why this is required? I do not understand why it's being build twice 😕

The package contains two components:

a AWS SDK plugin under the aws-cpp-sdk-cdi subfolder which is built with cmake

the library itself which is built with make

The library depends on the plugin and that's why I need to install it here and tell the library where to find it

prince-chrismc · 2021-09-30T23:57:07Z

recipes/aws-cdi-sdk/all/conanfile.py

+
+    def validate(self):
+        if self.settings.os != "Linux":
+            raise ConanInvalidConfiguration("The aws-cdi-sdk con only be built on Linux.")


Windows could be supported. https://github.com/aws/aws-cdi-sdk/blob/mainline/INSTALL_GUIDE_WINDOWS.md#build-the-aws-cdi-sdk

Suggested change

raise ConanInvalidConfiguration("The aws-cdi-sdk con only be built on Linux.")

raise ConanInvalidConfiguration("This recipe currently only supports Linux. Feel free to contribute other platforms!")

prince-chrismc · 2021-09-30T23:59:08Z

recipes/aws-cdi-sdk/all/patches/001-Makefile.patch

+-# Ensure that the location of the AWS SDK source code tree was specified unless explicitly opted out. Define and augment
+-# some variables needed for building and linking to the AWS SDK.
+-ifeq ($(require_aws_sdk),yes)


This does not seem required to build the library 🤔 can you explain what this patch is for please?

the original package builds libfabric and the AWS SDK itself and here I remove that part

I think you can reduce the patch size by surrounding the disabled parts with e.g. ifeq('1','0') and endif.
See https://www.gnu.org/software/make/manual/html_node/Conditionals.html#Conditionals

It makes porting the patch to newer releases easier, I think.

prince-chrismc · 2021-10-01T00:01:39Z

recipes/aws-cdi-sdk/all/conanfile.py

+        if self._cmake:
+            return self._cmake
+        self._cmake = CMake(self)
+        self._cmake.definitions["CPP_STANDARD"] = self._cmake.definitions.get("CONAN_CMAKE_CXX_STANDARD", "11")


I would have expected this to come from the users profile/settings

prince-chrismc

Excellent start!

I'd love to point you to our docs, https://github.com/conan-io/conan-center-index/blob/master/docs/how_to_add_packages.md#cmake for some examples

also #3951 which is not official but a great summary of what we've been following...
I fear your patches might be too large for us to accept since they will be very difficult to maintain

dvirtz · 2021-10-01T13:11:01Z

The way the original package is built is by downloading the AWS SDK, copying the aws-cpp-sdk-cdi under it and build the SDK together with the aws-cpp-sdk-cdi part.
To make it work with the conan AWS SDK I had to copy some files from AWS SDK to be able to build aws-cpp-sdk-cdi standalone.
That's the reason for the large patches.

prince-chrismc · 2022-02-10T04:39:19Z

recipes/aws-cdi-sdk/all/conanfile.py

+        tools.rmdir(os.path.join(self.package_folder, "lib", "pkgconfig"))
+
+    def package_info(self):
+        self.cpp_info.libs = self.collect_libs()


Looking at the logs I only saw one lib being build/installed

Suggested change

self.cpp_info.libs = self.collect_libs()

self.cpp_info.libs = ["aws-cpp-sdk-cdi"]

recipes/aws-cdi-sdk/all/conanfile.py

prince-chrismc · 2022-02-10T04:48:15Z

recipes/aws-cdi-sdk/all/conanfile.py

+        self.cpp_info.libs = self.collect_libs()
+        if self.settings.os == "Linux":
+            self.cpp_info.defines = ["_LINUX"]
+        self.cpp_info.requires = ["aws-sdk-cpp::monitoring", "aws-libfabric::aws-libfabric"]


bump https://github.com/conan-io/conan-center-index/pull/7407/files#r731926015

prince-chrismc · 2022-02-10T04:49:07Z

recipes/aws-cdi-sdk/all/patches/002-CMake.patch

+--- a/aws-cpp-sdk-cdi/CMakeLists.txt
+++ b/aws-cpp-sdk-cdi/CMakeLists.txt
+@@ -1,4 +1,39 @@
+-add_project(aws-cpp-sdk-cdi "C++ SDK for the AWS cdi service" aws-cpp-sdk-core)


Maybe the find_package could be called from the wrapper?

recipes/aws-cdi-sdk/all/test_package/CMakeLists.txt

recipes/aws-cdi-sdk/all/conanfile.py

SpaceIm · 2022-02-28T10:42:48Z

recipes/aws-cdi-sdk/all/conanfile.py

+    default_options = {
+        "aws-libfabric:shared": True,
+        "aws-sdk-cpp:shared": True
+    }


Don't do that. Force these values in configure(), then check in validate().

SpaceIm · 2022-02-28T10:44:56Z

recipes/aws-cdi-sdk/all/conanfile.py

+        cppSdk.set_property("cmake_file_name", "aws-cpp-sdk-cdi")
+        cppSdk.set_property("cmake_target_name", "aws-cpp-sdk-cdi")
+        cppSdk.set_property("pkg_config_name", "aws-cpp-sdk-cdi")
+
+        # TODO: to remove in conan v2 once cmake_find_package_* generators removed
+        # TODO: Remove the namespace on CMake targets
+        cppSdk.names["cmake_find_package"] = "aws-cpp-sdk-cdi"
+        cppSdk.names["cmake_find_package_multi"] = "aws-cpp-sdk-cdi"
+        cppSdk.names["pkg_config"] = "aws-cpp-sdk-cdi"


it's not consistent. CMakeDeps provides aws-cpp-sdk-cdi imported target, and cmake_find_package* aws-cdi-sdk::aws-cpp-sdk-cdi.
What is the correct imported target upstream?

I've set both to be AWS::aws-cpp-sdk-cdi similar to the aws-sdk-cpp package.
I couldn't anything existing practice upstream.

If you don't know how to deduce imported targets from upstream CMakeLists (not always obvious), you can inspect packaged files locally:

comment out the deletion of cmake files during packaging.

inspect lib/cmake/aws-cpp-sdk-cdi/aws-cpp-sdk-cdi-targets.cmake in package folder and grep add_library to see imported targets names, and therefore deduce cmake_target_name of each component.

From what I can see in build log, cmake_file_name is aws-cpp-sdk-cdi:

-- Installing: /home/conan/w/BuildSingleReference/.conan/data/aws-cdi-sdk/2.2.0/_/_/package/7e1489d0a9b824ecd6e0f2c6473d3bdf325c0375/lib/cmake/aws-cpp-sdk-cdi/aws-cpp-sdk-cdi-targets.cmake -- Installing: /home/conan/w/BuildSingleReference/.conan/data/aws-cdi-sdk/2.2.0/_/_/package/7e1489d0a9b824ecd6e0f2c6473d3bdf325c0375/lib/cmake/aws-cpp-sdk-cdi/aws-cpp-sdk-cdi-targets-release.cmake -- Installing: /home/conan/w/BuildSingleReference/.conan/data/aws-cdi-sdk/2.2.0/_/_/package/7e1489d0a9b824ecd6e0f2c6473d3bdf325c0375/lib/cmake/aws-cpp-sdk-cdi/aws-cpp-sdk-cdi-config.cmake -- Installing: /home/conan/w/BuildSingleReference/.conan/data/aws-cdi-sdk/2.2.0/_/_/package/7e1489d0a9b824ecd6e0f2c6473d3bdf325c0375/lib/cmake/aws-cpp-sdk-cdi/aws-cpp-sdk-cdi-config-version.cmake

And by the way, I forgot to say that cmake_file_name property at component level doesn't make sense for conan.

Thanks for your comment. aws-cpp-sdk-cdi is the C++ component. The main C component is named aws-cdi-sdk and doesn't have upstream cmake files installed.
I've removed file name properties from components but I don't see any other problem with the current package info. I'd be glad if you can suggest what is the correct info in your opinion.

I see. Very confusing build system: why do they mix CMakeLists to build aws-cpp-sdk-cdi & raw Makefile to build aws-cdi-sdk?

I guess different people worked on each component

SpaceIm · 2022-02-28T10:49:41Z

recipes/aws-cdi-sdk/all/conanfile.py

+        cSdk.set_property("cmake_file_name", "aws-cdi-sdk")
+        cSdk.set_property("cmake_target_name", "aws-cdi-sdk")
+        cSdk.set_property("pkg_config_name", "aws-cdi-sdk")
+
+        # TODO: to remove in conan v2 once cmake_find_package_* generators removed
+        # TODO: Remove the namespace on CMake targets
+        cSdk.names["cmake_find_package"] = "aws-cdi-sdk"
+        cSdk.names["cmake_find_package_multi"] = "aws-cdi-sdk"
+        cSdk.names["pkg_config"] = "aws-cdi-sdk"


Same comment than #7407 (comment)

SpaceIm · 2022-03-02T16:58:23Z

recipes/aws-cdi-sdk/all/conanfile.py

+        self.cpp_info.names["cmake_find_package"] = "AWS"
+        self.cpp_info.names["cmake_find_package_multi"] = "AWS"
+        self.cpp_info.filenames["cmake_find_package"] = "aws-cdi-sdk"
+        self.cpp_info.filenames["cmake_find_package_multi"] = "aws-cdi-sdk"
+
+        cppSdk = self.cpp_info.components["aws-cpp-sdk-cdi"]
+        cppSdk.libs = ["aws-cpp-sdk-cdi"]
+
+        cppSdk.requires = ["aws-sdk-cpp::monitoring", "aws-libfabric::aws-libfabric"]
+
+        cppSdk.set_property("cmake_file_name", "aws-cpp-sdk-cdi")
+        cppSdk.set_property("cmake_target_name", "AWS::aws-cpp-sdk-cdi")
+        cppSdk.set_property("pkg_config_name", "aws-cpp-sdk-cdi")
+
+        # TODO: to remove in conan v2 once cmake_find_package_* generators removed
+        # TODO: Remove the namespace on CMake targets
+        cppSdk.names["cmake_find_package"] = "aws-cpp-sdk-cdi"
+        cppSdk.names["cmake_find_package_multi"] = "aws-cpp-sdk-cdi"
+        cppSdk.names["pkg_config"] = "aws-cpp-sdk-cdi"
+
+        cSdk = self.cpp_info.components["cdisdk"]
+        cSdk.libs = ["cdisdk"]
+        cSdk.requires = ["aws-cpp-sdk-cdi"]
+        if self.settings.os == "Linux":
+            cSdk.defines = ["_LINUX"]
+
+        cSdk.set_property("cmake_file_name", "aws-cdi-sdk")
+        cSdk.set_property("cmake_target_name", "AWS::aws-cdi-sdk")
+        cSdk.set_property("pkg_config_name", "aws-cdi-sdk")
+
+        # TODO: to remove in conan v2 once cmake_find_package_* generators removed
+        # TODO: Remove the namespace on CMake targets
+        cSdk.names["cmake_find_package"] = "aws-cdi-sdk"
+        cSdk.names["cmake_find_package_multi"] = "aws-cdi-sdk"
+        cSdk.names["pkg_config"] = "aws-cdi-sdk"
+


still wrong and inconsistent. See #7407 (comment)

recipes/aws-cdi-sdk/all/conanfile.py

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>

conan-center-bot · 2022-03-13T22:41:33Z

All green in build 2 (9fa8200e33de32325e71e138f1784ef60a25ce0a):

aws-cdi-sdk/2.2.0@:
All packages built successfully! (All logs)

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Sep 24, 2021

This comment has been minimized.

Sign in to view

SSE4 added the blocked Affected by an external issue and waiting until it is solved label Sep 26, 2021

dvirtz mentioned this pull request Sep 26, 2021

aws-sdk-cpp: turn monitoring on #7418

Merged

4 tasks

add aws-cdi-sdk

a40d66f

dvirtz force-pushed the aws-cdi-sdk branch from d28d6d9 to a40d66f Compare September 29, 2021 06:42

This comment has been minimized.

Sign in to view

only support linux

696dc90

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Sep 29, 2021

conan-center-bot requested review from uilianries and SSE4 September 29, 2021 12:00

SSE4 removed the blocked Affected by an external issue and waiting until it is solved label Sep 30, 2021

prince-chrismc mentioned this pull request Sep 30, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

prince-chrismc reviewed Sep 30, 2021

View reviewed changes

prince-chrismc reviewed Oct 1, 2021

View reviewed changes

prince-chrismc suggested changes Oct 1, 2021

View reviewed changes

conan-center-bot requested a review from czoido October 1, 2021 11:57

apply review fixes

e599431

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Oct 1, 2021

prince-chrismc reviewed Feb 10, 2022

View reviewed changes

apply review comments

8e4406a

This comment has been minimized.

Sign in to view

conan-center-bot requested a review from SSE4 February 10, 2022 12:03

add file names

8b8e8aa

This comment has been minimized.

Sign in to view

SSE4 previously approved these changes Feb 21, 2022

View reviewed changes

prince-chrismc previously approved these changes Feb 25, 2022

View reviewed changes

SpaceIm reviewed Feb 28, 2022

View reviewed changes

address review comments

ea12bd8

dvirtz dismissed stale reviews from prince-chrismc and SSE4 via ea12bd8 February 28, 2022 20:19

This comment has been minimized.

Sign in to view

prince-chrismc previously approved these changes Mar 2, 2022

View reviewed changes

SpaceIm reviewed Mar 2, 2022

View reviewed changes

remove file name from components

cf6a967

dvirtz dismissed prince-chrismc’s stale review via cf6a967 March 2, 2022 21:21

This comment has been minimized.

Sign in to view

SpaceIm reviewed Mar 2, 2022

View reviewed changes

recipes/aws-cdi-sdk/all/conanfile.py Outdated Show resolved Hide resolved

address review comment

9fa8200

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>

This comment has been minimized.

Sign in to view

jgsogo added the Unexpected Error label Mar 10, 2022

conan-center-bot removed the Unexpected Error label Mar 13, 2022

prince-chrismc approved these changes Mar 14, 2022

View reviewed changes

SSE4 approved these changes Mar 15, 2022

View reviewed changes

jgsogo approved these changes Mar 15, 2022

View reviewed changes

conan-center-bot merged commit 9a6ab53 into conan-io:master Mar 15, 2022

	raise ConanInvalidConfiguration("The aws-cdi-sdk con only be built on Linux.")
	raise ConanInvalidConfiguration("This recipe currently only supports Linux. Feel free to contribute other platforms!")

	self.cpp_info.libs = self.collect_libs()
	self.cpp_info.libs = ["aws-cpp-sdk-cdi"]

add aws-cdi-sdk 2.2.0 #7407

add aws-cdi-sdk 2.2.0 #7407

Conversation

dvirtz commented Sep 24, 2021

This comment has been minimized.

This comment has been minimized.

dvirtz commented Sep 25, 2021

SSE4 commented Sep 26, 2021

This comment has been minimized.

This comment has been minimized.

dvirtz commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prince-chrismc left a comment

Choose a reason for hiding this comment

This comment has been minimized.

dvirtz commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

SpaceIm Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

SpaceIm Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

conan-center-bot commented Mar 13, 2022

SpaceIm Feb 28, 2022 •

edited

Loading

SpaceIm Feb 28, 2022 •

edited

Loading

SpaceIm Feb 28, 2022 •

edited

Loading

SpaceIm Mar 2, 2022 •

edited

Loading