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

cli11: Compile by default, add header_only option #25741

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

vasama
Copy link
Contributor

@vasama vasama commented Oct 28, 2024

Summary

CLI11 supports static library builds. This PR changes the recipe to build a static library by default and adds a header only option.

Motivation

Fixes #25737.

Details


@CLAassistant
Copy link

CLAassistant commented Oct 28, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

uilianries
uilianries previously approved these changes Oct 28, 2024
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

uilianries
uilianries previously approved these changes Oct 28, 2024
@conan-center-bot

This comment has been minimized.

"header_only": [True, False]
}
default_options = {
"header_only": False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"header_only": False
"header_only": True

Keep header-only as True, so the Package ID will be the same as for previous versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That really sucks because header only builds are very undesirable. It ends up breaking unrelated code due to leaked implementation details like included system headers. That's what prompted me to create the issue and PR in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

The main issue here is that upstream also defaults to a single-header installation, and the compilation behaviour is opt-in, so with this we would be following the choices on upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a very user hostile default though, only motivated by ease of distribution, which is hardly a concern if one is using Conan.

Copy link
Member

Choose a reason for hiding this comment

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

@vasama You still can build using the option that you need in your configuration as well. In CCI we mostly follow upstream default behavior, as it will affect all users and will reflect the upstream build as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware of that. I just want to make it clear that header only is a bad and pointless default.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Signed-off-by: Uilian Ries <[email protected]>
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

Warning

Conan Center will stop receiving updates for Conan 1.x packages soon - please see announcement.

All green in build 11 (827b40b9303f1f9faf65c71f6ce20e6db42ac3ff):

  • cli11/2.4.1:
    All packages built successfully! (All logs)

  • cli11/2.4.2:
    All packages built successfully! (All logs)

  • cli11/2.4.0:
    All packages built successfully! (All logs)

  • cli11/2.3.0:
    All packages built successfully! (All logs)

  • cli11/1.9.1:
    Built 5 packages out of 16 (All logs)

  • cli11/2.1.1:
    Built 5 packages out of 16 (All logs)

  • cli11/2.0.0:
    Built 5 packages out of 16 (All logs)

  • cli11/2.3.2:
    All packages built successfully! (All logs)

  • cli11/2.1.2:
    Built 5 packages out of 16 (All logs)

  • cli11/2.3.1:
    All packages built successfully! (All logs)

  • cli11/2.2.0:
    Built 5 packages out of 16 (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 11 (827b40b9303f1f9faf65c71f6ce20e6db42ac3ff):

  • cli11/2.4.2:
    All packages built successfully! (All logs)

  • cli11/2.4.1:
    All packages built successfully! (All logs)

  • cli11/2.3.0:
    All packages built successfully! (All logs)

  • cli11/2.3.1:
    All packages built successfully! (All logs)

  • cli11/2.4.0:
    All packages built successfully! (All logs)

  • cli11/2.1.2:
    Built 4 packages out of 9 (All logs)

  • cli11/2.3.2:
    All packages built successfully! (All logs)

  • cli11/1.9.1:
    Built 4 packages out of 9 (All logs)

  • cli11/2.1.1:
    Built 4 packages out of 9 (All logs)

  • cli11/2.2.0:
    Built 4 packages out of 9 (All logs)

  • cli11/2.0.0:
    Built 4 packages out of 9 (All logs)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM.

@uilianries uilianries self-assigned this Oct 31, 2024
@uilianries uilianries requested a review from AbrilRBS October 31, 2024 07:28
@conan-center-bot conan-center-bot merged commit 9cbd329 into conan-io:master Oct 31, 2024
48 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.

[package] CLI11 should not be header only
5 participants