-
Notifications
You must be signed in to change notification settings - Fork 364
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
Consuming system requirements only when building a package #3980
Consuming system requirements only when building a package #3980
Conversation
Signed-off-by: Uilian Ries <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused about this, it seems it has some overlap with the other existing example with code, not as clear as the other one, like a bit of a mix.
examples/tools.rst
Outdated
@@ -14,3 +14,4 @@ Conan recipe tools examples | |||
tools/scm/git/capture_scm/git_capture_scm | |||
tools/microsoft/msbuild | |||
tools/system/package_manager | |||
tools/system/consuming_system_packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need to be put into the same tools/system
examples directory, not a first level entry for each one.
The entry point can summarize and redirect to the different examples below it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean only extending the file package_manager.rst
with this new content, instead of adding an entirely new file?
This new example/how-to is under the same examples tools/system folder as the other example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, every other item in this section is a sub-section, see https://docs.conan.io/2/examples/tools.html, containing further articles/examples. The missing part is the equivalent to https://docs.conan.io/2/examples/tools/google/bazel.html, but for "system-package-manager"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restructured on the commit 6f50b49
For those cases, there are few approaches that can be used to achieve this goal. | ||
|
||
|
||
Consume a Conan package wrapper for a system package using hidden visibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this mostly what is discussed in the other example, with the system wrapper? Why repeating it here, not better linking to the other example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you are referring to this comment: #3978 (comment)
In the previous example (https://docs.conan.io/2/examples/tools/system/package_manager.html) I didn't add any mention of how to skip the system package manager when installing, but only explained how to wrap a system package manager and consume it as a requirement.
This specific topic of skipping system package managers I added in a separate issue #3975 to avoid a much bigger PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly my point. We have a full working and reproducible example in https://docs.conan.io/2/examples/tools/system/package_manager.html. Why adding a new partial conanfile.py
here, just for the visible=True
? I think the best would be to provide the real world case:
- Provide a full
pkg
conanfile. - Then use the
tool_requires()
instead ofvisible=False
, it is more idiomatic and adequate for the task. conan create
the package- Consume the package, show the output to see that the
ncurses/system
is skipped, and that consequently theApt()
or equivalent is not called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified by the commit 95b4c57
cases and is good practice to avoid code duplication. | ||
|
||
|
||
Package the system requirement as a header-library or consume only the headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also reads like a derivative or particular case for the other example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a particular case related to the previous example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section is more confusing than beneficial at this point, I'd probably remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed by the commit 95b4c57
Signed-off-by: Uilian Ries <[email protected]>
Signed-off-by: Uilian Ries <[email protected]>
Added similar content as described on conan-io/conan#16625 (comment), but with more details.
closes #3975
Preview from commit 95b4c57