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

Add cpprestsdk options for underlying implementation #1930

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

garethsb
Copy link
Contributor

@garethsb garethsb commented Jun 16, 2020

Add Conan options for C++ REST SDK, equivalent to its CMake cache variables, CPPREST_PPLX_IMPL, CPPREST_HTTP_CLIENT_IMPL, CPPREST_HTTP_LISTENER_IMPL, which each support two different values on the Windows platform.

I studied the documentation for config_options and read related issues such as conan-io/conan#3519, since I originally planned to only add these options for some platforms (so that other platforms report exception) and/or add platform-specific default values in config_options, but that didn't seem to work as I expected, so I followed the doc example of removing the options on other platforms instead.

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

…iables, CPPREST_PPLX_IMPL, CPPREST_HTTP_CLIENT_IMPL, CPPREST_HTTP_LISTENER_IMPL, which support different values on the Windows platform
@conan-center-bot
Copy link
Collaborator

All green in build 1 (bdad34b6c40b083790a02b198b641e95ae789d50)! 😊

@SSE4 SSE4 requested review from uilianries and danimtb June 16, 2020 14:16
@@ -40,6 +46,10 @@ def _build_subfolder(self):
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the default values above only apply on Windows, would it be better to remove "pplx_impl": "win", etc. from default_options and in config_options replace the else with the following code under the if self.settings.os == "Windows": branch?

            if self.options.get_safe("pplx_impl") == None:
                self.options["pplx_impl"] = "win"
            if self.options.get_safe("http_client_impl") == None:
                self.options["http_client_impl"] = "winhttp"
            if self.options.get_safe("http_listener_impl") == None:
                self.options["http_listener_impl"] = "httpsys"

Copy link
Member

Choose a reason for hiding this comment

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

You can't remove default options, Conan will break when building.

It can be improved:

if self.options.get_safe("http_listener_impl") == None:
    self.options["http_listener_impl"] = "httpsys"
if not self.options.get_safe("http_listener_impl"):
    self.options["http_listener_impl"] = "httpsys"

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

You can't remove default options, Conan will break when building.

I read this: https://docs.conan.io/en/latest/reference/conanfile/attributes.html#default-options which says "You can also set the options conditionally to a final value with config_options() instead of using default_options". I was asking if that would be more appropriate here?

Copy link
Member

Choose a reason for hiding this comment

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

You can remove self.options, not self.default_options.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Almost good, only simplify cmake definitions

Comment on lines +81 to +86
if self.options.get_safe("pplx_impl"):
self._cmake.definitions["CPPREST_PPLX_IMPL"] = self.options.pplx_impl
if self.options.get_safe("http_client_impl"):
self._cmake.definitions["CPPREST_HTTP_CLIENT_IMPL"] = self.options.http_client_impl
if self.options.get_safe("http_listener_impl"):
self._cmake.definitions["CPPREST_HTTP_LISTENER_IMPL"] = self.options.http_listener_impl
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if self.options.get_safe("pplx_impl"):
self._cmake.definitions["CPPREST_PPLX_IMPL"] = self.options.pplx_impl
if self.options.get_safe("http_client_impl"):
self._cmake.definitions["CPPREST_HTTP_CLIENT_IMPL"] = self.options.http_client_impl
if self.options.get_safe("http_listener_impl"):
self._cmake.definitions["CPPREST_HTTP_LISTENER_IMPL"] = self.options.http_listener_impl
self._cmake.definitions["CPPREST_PPLX_IMPL"] = self.options.get_safe("pplx_impl", False)
self._cmake.definitions["CPPREST_HTTP_CLIENT_IMPL"] = self.options.get_safe("http_client_impl", False)
self._cmake.definitions["CPPREST_HTTP_LISTENER_IMPL"] = self.options.get_safe("http_listener_impl", False)

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 don't think that has the same effect? I don't want to set the CMake definitions if the Conan option is not specified.

Copy link
Member

Choose a reason for hiding this comment

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

Same effect, but simplified. Setting to False is not a problem, if you are following the correct default options.

Copy link
Contributor

Choose a reason for hiding this comment

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

@uilianries
These options are no bools

@uilianries uilianries merged commit 4d167ec into conan-io:master Jun 17, 2020
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.

6 participants