search

conan-io / hooks

Official Conan client hooks
MIT License
32 stars 44 forks source link

Please include package name "cern-root" in allow list for #KB-H050: "DEFAULT SHARED OPTION VALUE" #252

Open davehadley opened 3 years ago

davehadley commented 3 years ago

I plan to submit a pull request to conan-center-index for a recipe for the ROOT data analysis framework (https://root.cern/). My draft recipe is at https://github.com/davehadley/conan-center-index/tree/feat-add-root-recipe.

ROOT does not currently support static builds (see: https://sft.its.cern.ch/jira/browse/ROOT-6446).

Can the package name root be added to the allow list to be compiled in shared only mode?

danimtb commented 3 years ago

Is this really needed? I think it enough to raise a ConanInvalidConfiguration exception for the configurations that are not supported by the library. Is that right @uilianries?

ericLemanissier commented 3 years ago

you don't want to throw a ConanInvalidConfiguration with the default option values, or it makes it non-trivial to be consumed from other recipes, and no dependants can be built on CCI. The simplest option in this case is simply to not provide a shared option.

Croydon commented 3 years ago

I would rather see a shared attribute with the default and only possible value True instead 🤔

ericLemanissier commented 3 years ago

It's more explicit, yes. But it means the package needs to be added to the hooks exception. Or do you mean make the hook not throw for recipes having shared option with only one possible value ?

Croydon commented 3 years ago

If this is possible it might be a good hook improvement, until then I would rather have it more explicit in the recipe + I don't think it is a huge deal to add it in the allow list

davehadley commented 3 years ago

Thank you for your help. In summary the suggestions are:

  1. Allow the option shared=False to be set but then raise ConanInvalidConfiguration.
  2. Remove the shared option entirely and hard-code shared=True in the recipe.
  3. Keep the default option with shared=True being the only valid value.

From the discussion (3) is the preferred option but requires modification to the conan-io/hooks repository (either add root recipe to the allow list or make the hook more intelligent).

For now, I have implemented solution (2) so that I can put together a draft pull request to test my recipe with the official CI. See https://github.com/conan-io/conan-center-index/pull/3732.

It is trivial to switch to solution (3). I can easily make this change to the recipe once the required changes to hooks are implemented.

davehadley commented 3 years ago

Updating this issue as the draft recipe in conan-io/conan-center-index#3732 was renamed from root to cern-root. so that is now the pattern that will need to go into the allow list to resolve this issue.