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 more options for xerces-c recipe #6959

Merged

Conversation

FMeinicke
Copy link
Contributor

Specify library name and version: xerces-c/3.2.3

As specified in http://xerces.apache.org/xerces-c/build-3.html#CMake
there are a number of project specific options for configuring xerces-c.
Currently, only the XMLCh type could be configured through a conan
option. This commit adds corresponding conan options for all of the
other options (i.e. network accessor, transcoder, message loader and
mutex manager) that were previously hardcoded. Since not every value of
these options is valid on every platform this commit also adds proper
validation of the user input to detect invalid configurations. Setting
the default value for the conan options is a bit more complicated
because for some options this value is not the same for all platforms.
Therefore, the default_options have been dropped in favor of setting
the options manually from within config_options() if not set by the
user.


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

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2021

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

As specified in http://xerces.apache.org/xerces-c/build-3.html#CMake
there are a number of project specific options for configuring xerces-c.
Currently, only the `XMLCh` type could be configured through a conan
option. This commit adds corresponding conan options for all of the
other options (i.e. network accessor, transcoder, message loader and
mutex manager) that were previously hardcoded. Since not every value of
these options is valid on every platform this commit also adds proper
validation of the user input to detect invalid configurations. Setting
the default value for the conan options is a bit more complicated
because for some options this value is not the same for all platforms.
Therefore, the `default_options` have been dropped in favor of setting
the `options` manually from within `config_options()` if not set by the
user.
@FMeinicke FMeinicke force-pushed the xercesc-improve-configuration branch from ba21f60 to d5775db Compare August 23, 2021 05:56
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

As far as it is possible, we try to keep compatibility with Python2, which is also supported by Conan v1.x. So, please, avoid those f-strings and type hints.

(I would love to use them everywhere, but it will only be possible when we migrate definitively away from some Conan v1.x constraints)

Thanks!

@jgsogo
Copy link
Contributor

jgsogo commented Aug 27, 2021

There are also some general comments about the changes themselves:

  • Try not to assign different option values for different settings. It is better to raise if some option values are incompatible and force the user to provide different (and explicit) input.
  • If some option is not supported in some OS, it's better to remove it (typically inside config_options()). Conan should take care of the error message. Then you can use self.options.get_safe('xxxxx', <default>) to retrieve the value of the option (no need to check if it exists first).

@FMeinicke
Copy link
Contributor Author

  • Try not to assign different option values for different settings. It is better to raise if some option values are incompatible and force the user to provide different (and explicit) input.

The problem is that depending on the OS the default value of certain options is different. I could set the network_accessor to socket by default but then a simple conan install would fail on Windows as socket is not supported there.
My goal was that a user would get the best default experience from a conan install without any options regardless of which OS their using. But if it's more conan-like to be explicit then I'm happy to change the default values to be all the same on all OSes and raise if the value is not valid.

  • If some option is not supported in some OS, it's better to remove it (typically inside config_options()). Conan should take care of the error message. Then you can use self.options.get_safe('xxxxx', <default>) to retrieve the value of the option (no need to check if it exists first).

That's not really the case here, I think. All options are supported on all OSes but the values an option can have depend on the OS.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

The problem is that depending on the OS the default value of certain options is different.

There are other examples of this in CCI and we usually put a value in the default and then change it in config_options

examples off the top of my head

self.options.with_ssl = "darwinssl" if tools.is_apple_os(self.settings.os) else "openssl"

def config_options(self):

recipes/xerces-c/all/conanfile.py Outdated Show resolved Hide resolved
recipes/xerces-c/all/conanfile.py Show resolved Hide resolved
recipes/xerces-c/all/conanfile.py Outdated Show resolved Hide resolved
to keep Pyhton 2 compatibility for Conan v1.x
and set different defaults for different OSes in `config_options`
if `curl` is used as `network_accessor`
@FMeinicke FMeinicke requested a review from jgsogo August 30, 2021 08:12
@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I think we can simplify that if condition if we use always tuples.

recipes/xerces-c/all/conanfile.py Outdated Show resolved Hide resolved
recipes/xerces-c/all/conanfile.py Outdated Show resolved Hide resolved
Apply suggestions from code review

Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
@conan-center-bot

This comment has been minimized.

plus use CMake target to pull in the extra deps since CCI curl is build with options
@prince-chrismc
Copy link
Contributor

Please consider FMeinicke#1

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

prince-chrismc
prince-chrismc previously approved these changes Sep 6, 2021
recipes/xerces-c/all/conanfile.py Outdated Show resolved Hide resolved
@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 7, 2021

Please reopen this review (don't close review when there is no consensus): #6959 (comment)

SpaceIm and others added 3 commits September 7, 2021 14:47
Copy link
Contributor

@madebr madebr left a comment

Choose a reason for hiding this comment

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

@conan-center-bot
Copy link
Collaborator

All green in build 9 (2eb723e2ed15717ea212c27c4d709be2ebeb09f2):

  • xerces-c/3.2.2@:
    All packages built successfully! (All logs)

  • xerces-c/3.2.3@:
    All packages built successfully! (All logs)

@uilianries
Copy link
Member

@jgsogo ping

@conan-center-bot conan-center-bot merged commit 1c13ff8 into conan-io:master Sep 17, 2021
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.

None yet

10 participants