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 component requires information #1682

Merged
merged 11 commits into from May 6, 2020

Conversation

danimtb
Copy link
Member

@danimtb danimtb commented Apr 29, 2020

@danimtb danimtb added this to the 1.25 milestone Apr 29, 2020
@danimtb danimtb requested a review from memsharded Apr 29, 2020
Copy link
Member

@jgsogo jgsogo left a comment

I would promote this "Components" subsection inside cpp_info attribute to a dedicated section in the "Creating packages" one, it should contain all the considerations, detailed information, examples, notes, warnings,... and make this reference about cpp_info more straightforward with a link to the other section.

The section about components will grow with more examples when generators implement their part.

wdyt?

reference/conanfile/attributes.rst Outdated Show resolved Hide resolved
Co-Authored-By: Anonymous Maarten <madebr@users.noreply.github.com>
@jgsogo jgsogo requested a review from solvingj Apr 30, 2020
@@ -933,30 +933,58 @@ Components

This is a **experimental** feature subject to breaking changes in future releases.

Copy link
Member

@jgsogo jgsogo May 4, 2020

Choose a reason for hiding this comment

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

IMO, I'd remove all this "Components" section and substitute it by a paragraph (if you feel like, move more things to the other section):

"More fine grained control over individual libraries available in a single Conan package can be achieved using components (LINK). Components allow you define a cpp_info object per each of those libraries and also requirements between them and to components of other packages:"

Python with example.

"Note that components should declare explicitly that they require other packages...."

Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

Kind of agree. This section still reads a bit like tutorial, not reference. Maybe moving some parts to the other doc, and leaving here just the components fields reference.

creating_packages/package_information.rst Outdated Show resolved Hide resolved
.. important::

Note that component requires should **explicitly** define requirements to other package (``zlib::zlib``) or other package components
(``openssl::ssl``).
Copy link
Member

@jgsogo jgsogo May 4, 2020

Choose a reason for hiding this comment

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

I think this is important and it is not clear enough: "By default components won't link against any other package required by the recipe, the requires list has to be populated explicitly with the list of components from other packages to use: it can be a single component (example) or the full requirement (example)."

creating_packages/package_information.rst Show resolved Hide resolved
@danimtb danimtb requested a review from jgsogo May 5, 2020
Copy link
Member

@memsharded memsharded left a comment

I like the new section there in "creating packages", good!

@@ -933,30 +933,58 @@ Components

This is a **experimental** feature subject to breaking changes in future releases.

Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

Kind of agree. This section still reads a bit like tutorial, not reference. Maybe moving some parts to the other doc, and leaving here just the components fields reference.

self.cpp_info.components["ssl"].includedirs = ["include/headers_ssl"]
self.cpp_info.components["ssl"].libs = ["libssl"]
self.cpp_info.components["ssl"].requires = ["crypto"]

Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

I think at this moment it would be very good to highlight as "important" note, that yes, this is a bit of repeated information of what the package might have in its build scripts, but this is better:

  • Not a big deal, typically define it once, not that terrible repetition, and it is nicely explicit and relatively intuitive
  • The most important thing: This is generic and can be consumed by other build systems, different ones from the used one.

Copy link
Member Author

@danimtb danimtb May 5, 2020

Choose a reason for hiding this comment

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

I added the "important" box at the top as a general message. Please check it and I can move it down here if you want. thanks

Copy link
Member

@jgsogo jgsogo left a comment

Check comments.

And we need a link from the section in reference/conanfile/methods.html#package-info to the new components' one in package_information_components

creating_packages/package_information.rst Outdated Show resolved Hide resolved

The structure of the ``Component`` object is the same as the one used by the ``cpp_info`` object and has **the same default directories**.
dictionary). The information of components is not lost but aggregated to the *global* scope and the usage of components should be
transparent right now to consumers and generators.
Copy link
Member

@jgsogo jgsogo May 5, 2020

Choose a reason for hiding this comment

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

I'd remove this important and add a link to the package_information_components. Take into account the seealso note below, probable both of them can be joined.

danimtb and others added 3 commits May 5, 2020
jgsogo
jgsogo approved these changes May 5, 2020
@danimtb danimtb merged commit a35112a into conan-io:develop May 6, 2020
2 checks passed
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

4 participants