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

Improve markdown generator #10673

Merged
merged 7 commits into from Mar 1, 2022
Merged

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Feb 28, 2022

Changelog: Feature: Improve markdown generator instructions.
Docs: conan-io/docs#2423

This started as just adding the language to the code blocks but this generator clearly needed some extra work, there were links to old integrations, it did not render well and there were some mistakes.

image

image

This does not include the XcodeDeps/XcodeToolchain as there's still a pending PR for those.

@czoido czoido added this to the 1.46 milestone Feb 28, 2022
@czoido
Copy link
Contributor Author

czoido commented Feb 28, 2022

This is what we previously had:

image

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Great job, thanks very much!

@SpaceIm
Copy link
Contributor

SpaceIm commented Feb 28, 2022

Looks nice.

Maybe not the good place, but I have one comment: I'm a little bit concerned that conan suggests in this file to link to Poco::Poco. It's a global target we can't avoid in poco recipe of conan-center recipe, even with set_property() , but this target doesn't exist upstream.
I know it could make generators more complex but it would be nice to be able to disable creation of the global target in package_info() when there are components (and fix the consequences in generators, because I understand that global target is used under the hood instead of using union of components targets).

Copy link
Contributor

@SSE4 SSE4 left a comment

Choose a reason for hiding this comment

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

quite long and verbose, but I think it's fine, and helpful for newcomers

@memsharded memsharded merged commit dff225b into conan-io:develop Mar 1, 2022
@jgsogo
Copy link
Contributor

jgsogo commented Mar 2, 2022

Looks nice.

Maybe not the good place, but I have one comment: I'm a little bit concerned that conan suggests in this file to link to Poco::Poco. It's a global target we can't avoid in poco recipe of conan-center recipe, even with set_property() , but this target doesn't exist upstream. I know it could make generators more complex but it would be nice to be able to disable creation of the global target in package_info() when there are components (and fix the consequences in generators, because I understand that global target is used under the hood instead of using union of components targets).

IIRC, generators (CMake ones) use the components of their requirements, that's why you need to list ...requires = "req1::cmp1", "req2::cmp1",..., the recipe writer chooses which components to consume (targets to link) from the requirements. Of course, it can also be the global one with req1::req1. @danimtb did some drawings taken from CMake outputs while developing the feature, maybe they can be found in the original PR.

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

7 participants