-
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
[aws-sdk-cpp] package files for building SDK plugins #7830
Conversation
|
||
|
||
int main() { | ||
using namespace Aws; | ||
using namespace Auth; |
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.
You already test the plugin functionality in test_package/aws-sdk-cpp-plugin/AwsSdkCppPlugin.cpp
.
Can this example remain as-is?
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.
In order to have some code in the plugin and avoid code duplication I've moved this code to the plugin and linked the example against it.
0048f07
to
6b19235
Compare
This comment has been minimized.
This comment has been minimized.
recipes/aws-sdk-cpp/all/conanfile.py
Outdated
tools.replace_in_file(os.path.join("cmake", "utilities.cmake"), "cmakeProjectConfig.cmake", "cmakeProjectConf.cmake") | ||
|
||
# create a cmake module to load the files above | ||
contents = textwrap.dedent(""" |
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 think you copied this from cmake/AWSSDKConfig.cmake
?
I believe this contains potential version depdendent logic.
Can we move this to a patch, and copy that file here instead?
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 tried to have the minimum amount of CMake code required to build a single SDK.
it's copied from different parts of the CMake code. That specific file is actually not seemed to be used anymore by the package.
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'm not sure how to achieve that with a patch
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.
The following patch creates a file containing 3 lines:
--- /dev/null
+++ some_file
@@ -0,0 +1,3 @@
+line1
+line2
+line3
Using the same format, you can create a patch for a longer file.
This comment has been minimized.
This comment has been minimized.
4288446
to
5a21dec
Compare
This comment has been minimized.
This comment has been minimized.
need to wait for dependencies to be updated due to #7763 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@toge do you know what dependency versions I put for the CI to find them? |
1bdbf7c
to
50c287c
Compare
This comment has been minimized.
This comment has been minimized.
I detected other pull requests that are modifying aws-sdk-cpp/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. |
@madebr @uilianries can you please re-review? |
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.
LTGM
recipes/aws-sdk-cpp/all/conanfile.py
Outdated
self.requires("aws-crt-cpp/0.14.3") | ||
else: | ||
self.requires("aws-c-event-stream/0.1.5") | ||
self.requires("aws-c-event-stream/0.2.7") | ||
if self.settings.os != "Windows": | ||
self.requires("openssl/1.1.1l") |
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.requires("openssl/1.1.1l") | |
self.requires("openssl/1.1.1m") |
738fd2f
to
c1171a1
Compare
@dvirtz You have merge conflicts ! |
Please add the recipe in the title of the PR pretty please 🙏 |
@dvirtz please, solve these git conflicts when possible. |
f5fad7c
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@ericLemanissier I love the linting ❤️ |
* add aws-cdi-sdk * only support linux * apply review fixes * must have shared aws-sdk-cpp * apply review comments * fix review comments * take advantage of #7830 * apply review comments * add file names * address review comments * remove file name from components * address review comment Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com> Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
Specify library name and version: aws-sdk-cpp
See #7407 (comment)
Enable building AWS SDK "plugins" (as done for example by AWS CDI SDK) by packaging build files to be used by those plugins
conan-center hook activated.