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

adding cpprestsdk/v2.10.15 #1191

Merged
merged 21 commits into from
Apr 23, 2020
Merged

Conversation

prince-chrismc
Copy link
Contributor

from bincrafters/conan-cpprestsdk@a322739

Specify library name and version: cpprestsdk/v2.10.15

  • 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.

The patches are very complicated, looking for guidance on how they can be improved!

@conan-center-bot
Copy link
Collaborator

Some configurations of 'cpprestsdk/2.10.14' failed in build 3 (70a0982ae4ea19d2c373c97da4c73131573413b8):

  • Linux x86_64, Release, gcc 5, libstdc++ . Options: cpprestsdk:shared-True
    • Hooks errors detected:
      • [HOOK - conan-center.py] post_package(): ERROR: [CMAKE-MODULES-CONFIG-FILES (KB-H016)] The conan-center repository doesn't allow the packages to contain CMake find modules or config files. The packages have to be located using generators and the declared cpp_infoinformation (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H016)
      • [HOOK - conan-center.py] post_package(): ERROR: [CMAKE-MODULES-CONFIG-FILES (KB-H016)] Found files:
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@conan-center-bot
Copy link
Collaborator

All green in build 4 (8812016f6c8fbe40856330b35875da3da95584f7)! 😊

prince-chrismc and others added 3 commits March 26, 2020 21:57
Co-Authored-By: Michael "Croydon" Keck <[email protected]>
Co-Authored-By: Michael "Croydon" Keck <[email protected]>
Co-Authored-By: Michael "Croydon" Keck <[email protected]>
Comment on lines 40 to 42
def configure(self):
if self.settings.compiler == 'Visual Studio':
del self.options.fPIC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll drop this here...

I ran a search to see how to deleted the fPIC option... However I have seen many different way is lots of recipes.

Which way is correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong. @uilianries
In config_options(), only the settings object is finished, so you can do:

if self.settings.os == "Windows":
    del self.options.fPIC

In the configure() method, the options object is finished, so you can do:

if self.options.shared:
    del self.options.fPIC

Copy link
Member

Choose a reason for hiding this comment

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

@madebr Yes. Thus, it should be moved to config_options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uilianries I noticed this is very inconsistent in most recipes. Could this be added to the hooks?

@conan-center-bot
Copy link
Collaborator

Failure in build 9 (091a6e73993b6928f78b428a0f3d0a63ca754db0):
cpprestsdk/2.10.14:

'conan export' command failed:

ERROR: Package recipe with version 2.10.14!=2.10.15

recipes/cpprestsdk/all/CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines 40 to 42
def configure(self):
if self.settings.compiler == 'Visual Studio':
del self.options.fPIC
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong. @uilianries
In config_options(), only the settings object is finished, so you can do:

if self.settings.os == "Windows":
    del self.options.fPIC

In the configure() method, the options object is finished, so you can do:

if self.options.shared:
    del self.options.fPIC

recipes/cpprestsdk/all/conanfile.py Outdated Show resolved Hide resolved
recipes/cpprestsdk/all/conanfile.py Outdated Show resolved Hide resolved
recipes/cpprestsdk/all/test_package/conanfile.py Outdated Show resolved Hide resolved
self._cmake.definitions["IOS"] = True
elif self.settings.os == "Android":
self._cmake.definitions["ANDROID"] = True
self._cmake.definitions["CONAN_LIBCXX"] = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for this?
Maybe a dependency sets wrong flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy paste from bincrafters, I have no clue =)

Copy link
Contributor

Choose a reason for hiding this comment

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

@SSE4
only 2 years ago
bincrafters/conan-cpprestsdk@c648cea 😈😄
Do you have any clues why this is/was needed?

I would comment the line out (+add comment) and see what happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the wiki, Android is not even listed. Is supporting android require for CCI?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not tested by CI, but patches are accepted as mobile devices are popular these days. (What is the official stance? @uilianries)

When you think about it, the IOS and ANDROID lines can be removed as well.
See https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html#variables-that-describe-the-system

Copy link
Contributor

Choose a reason for hiding this comment

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

When I search for the string "android" in their github cpprestsdk repo, I can find some hits.
So it is supported by the project (to some level at least)

Copy link
Member

Choose a reason for hiding this comment

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

Android support came from another contributor. Need to investigate if that recipe really requires those definitions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have opted for removing Android. If in the future it's needed we can always revisit the topic.

@madebr madebr mentioned this pull request Mar 27, 2020
4 tasks
@conan-center-bot
Copy link
Collaborator

All green in build 13 (334baa9c56cd26ba19e01607cc7c401352f5d85f)! 😊

@uilianries
Copy link
Member

@prince-chrismc take a look prince-chrismc#2

@conan-center-bot
Copy link
Collaborator

Some configurations of 'cpprestsdk/2.10.14' failed in build 15 (cac32213529329f463434547e5ded2da97d501dd):

@conan-center-bot
Copy link
Collaborator

Some configurations of 'cpprestsdk/2.10.14' failed in build 16 (7eb661e40df1527dbd4a1f89941ab8627107691b):

@conan-center-bot
Copy link
Collaborator

All green in build 17 (ca2e539f4e8d5eff58005901041e9e994ab82aae)! 😊

uilianries
uilianries previously approved these changes Apr 2, 2020
@uilianries uilianries requested a review from SSE4 April 2, 2020 15:11
SSE4
SSE4 previously approved these changes Apr 3, 2020
@prince-chrismc prince-chrismc dismissed stale reviews from SSE4 and uilianries via 5acbdad April 17, 2020 12:45
@conan-center-bot
Copy link
Collaborator

All green in build 18 (5acbdadfeff07acb23d638c2b2d52d893897bd8f)! 😊

@prince-chrismc
Copy link
Contributor Author

❤️ I really appreciate the work the whole CCI team put's in to make this project a reality. 🥇 It's greatly appreciated.

@danimtb I do have a small requests for tomorrow's "PR merging session"... here's a list off oldest approved PR which many seem ready to go.
There's 10 of them dating back to beginning of February. Hopefully this helps narrow down the ones that need attention.
It's a little disheartening seeing an approved PR sit for 3 weeks and go stale 😞 while the 16 newest PRs got merged today alone. (which is freaking impressive 😮 )
Maybe having the bot or actions tag PR's 'Ready' could be helpful for you guys sort them out more?

Regardless of what you do, I am incredibly grateful for the effort from the CCI team! 💘

@prince-chrismc prince-chrismc changed the title first draft for migrating cpprestsdk/v2.10.x@bincrafters/stable to CCI adding cpprestsdk/v2.10.15 Apr 23, 2020
@conan-center-bot
Copy link
Collaborator

All green in build 19 (5acbdadfeff07acb23d638c2b2d52d893897bd8f)! 😊

@danimtb
Copy link
Member

danimtb commented Apr 23, 2020

@prince-chrismc thanks a lot for the feedback. We have plans to automate ConanCenter and improve the review process involving enthusiastic users from the community like you. Of course, we will review the list of approved PRs and unblock other ones in the next wave, it just that sometimes there are so many things going on that you overlook some of the open PRs. Thanks a lot again for your contributions and for helping us review other PRs 😄

@danimtb danimtb merged commit 41f9410 into conan-io:master Apr 23, 2020
@prince-chrismc prince-chrismc deleted the add/cpprestsdk branch April 23, 2020 16:01
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.

7 participants