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

[feat] Recipes declare what they provide (and this can create a conflict) #7337

Merged
merged 15 commits into from Jul 21, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Jul 10, 2020

Changelog: Feature: Add provides attribute to ConanFile: recipes can declare what they provide and Conan will fail if several recipes provide the same functionality (ODR violation).
Docs: conan-io/docs#1786

If nothing is provided explicitly, the provides attribute takes one single value equal to the reference name. Two recipes providing the same functionality in the same context (public closure) will produce an error.

Q: Error or warning?

Q: Transitive private requirements, is it an ODR violation?

close #7252

@jgsogo jgsogo self-assigned this Jul 10, 2020
@jgsogo jgsogo added this to the 1.28 milestone Jul 10, 2020
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jul 10, 2020

Q: Error or warning?

Q: Transitive private requirements, is it an ODR violation?

A draft because I'm not sure about the two previous questions, but it is ready for review/feedback. Is what you expect from the feature, @SSE4 @memsharded ?

@jgsogo jgsogo requested review from SSE4 and memsharded Jul 13, 2020
Copy link
Member

@memsharded memsharded left a comment

Q: Error or warning?

I'd say an error, warnings are often skipped, and in theory this shouldn't change if provides is not defined.

Q: Transitive private requirements, is it an ODR violation?

Private requirements already allow different versions of the same lib, that is the core definition of a private requirment. So in this case, provides shouldn't error either.

There is a (probably very small) risk that a bug in validate_graph_provides can break things with build_requires, privates, or other stuff. The way to eliminate it would be to make provides explicit always, even for providers (so recipes that could conflict, should declare it explicitly, i.e. libjpeg should declare that it provides libjpeg if there are others that can provide it too. As this seems a bit less elegant, we should probably assume the risk and rely on the tests, that look good.

conans/client/graph/graph_manager.py Show resolved Hide resolved
conans/client/graph/graph_manager.py Outdated Show resolved Hide resolved
@jgsogo jgsogo marked this pull request as ready for review Jul 15, 2020
SSE4
SSE4 approved these changes Jul 15, 2020
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jul 17, 2020

Concern here after comments in conan-io/conan-center-index#1207 (comment)

If recipe xorg/system declares:

class xorg(ConanFile):
    provides = `boost`, `fontconfig`, `freetype`

Conan will raise a conflict if boost is somewhere else in the graph.

What can we recommend to the user in order to workaround this error?

@Minimonium
Copy link
Contributor

@Minimonium Minimonium commented Jul 17, 2020

@jgsogo The situation is similar to a one where there are two transitive dependencies, one with ^1 and one with ^2. There is no much Conan could suggest other than to ask users to remove or update a dependency.

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 17, 2020

we probably need to make provides a method instead of an attribute, because for xorg I'm not sure all linux distributions actually install boost fontconfig and freetype. Also, this could depend on options and settings.

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jul 17, 2020

we probably need to make provides a method instead of an attribute, because for xorg I'm not sure all linux distributions actually install boost fontconfig and freetype. Also, this could depend on options and settings.

I think you can modify/set that value in the configure() method... probably it deserves a test

@ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Jul 17, 2020

alternatively, maybe recipes can define the following ?

@property
def provides(self):
    return whatever

@jgsogo jgsogo requested review from memsharded and SSE4 Jul 17, 2020
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jul 17, 2020

The thing is that we need self.settings and self.options to be available and evaluated, but we can safely add/override provides in the configure() method

@memsharded memsharded requested review from czoido and danimtb Jul 21, 2020
def _build_requires_line(self):
def _build_requirements_method(self):
Copy link
Member

@danimtb danimtb Jul 21, 2020

Choose a reason for hiding this comment

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

Why not keeping both?

Copy link
Member

@memsharded memsharded Jul 21, 2020

Choose a reason for hiding this comment

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

I will change it to include both, I still would like recipes created with GenConanfile() to be able to define build_requires as attributes

czoido
czoido approved these changes Jul 21, 2020
@memsharded memsharded merged commit eb5ff51 into conan-io:develop Jul 21, 2020
2 checks passed
@jgsogo jgsogo deleted the feat/recipe-provides branch Aug 3, 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.

7 participants