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

Vendor CLI11 library inside ignition-utils #2

Merged
merged 3 commits into from
Feb 8, 2021
Merged

Vendor CLI11 library inside ignition-utils #2

merged 3 commits into from
Feb 8, 2021

Conversation

mjcarroll
Copy link
Contributor

Depends on: gazebosim/gz-cmake#143

Signed-off-by: Michael Carroll [email protected]

Copy link
Contributor

@caguero caguero left a comment

Choose a reason for hiding this comment

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

It looks good to me assuming that we're convinced that we want to vendor this library. My only suggestion would be to create a basic example to show how to use it in a very basic way.

@mjcarroll
Copy link
Contributor Author

From @scpeters homebrew has a package: https:/Homebrew/homebrew-core/blob/master/Formula/cli11.rb

@scpeters
Copy link
Member

looks like it's in vcpkg as well: https://repology.org/project/cli11/versions

Base automatically changed from feature/impl_ptr to main January 11, 2021 20:29
@mjcarroll mjcarroll requested a review from azeey as a code owner January 11, 2021 20:29
@mjcarroll
Copy link
Contributor Author

mjcarroll commented Jan 12, 2021

@scpeters and I discussed this with @j-rivero offline. Since this will only be used in executables, is header only, and has no existing Debian package, it makes sense to include the CLI11 headers as part of the ign-utils install.

Additionally, rather than using the packaged versions for vcpkg and homebrew via cmake logic, we can use the same set of headers across all platforms for consistency.

This is still pending me developing a prototype for using this with ign-tools

@chapulina
Copy link
Contributor

Closing and reopening to see if DCO takes effect

@azeey
Copy link
Contributor

azeey commented Jan 27, 2021

How do we deal with side-by-side installs in the future? We have used inline namespaces for this in our other packages, but CLI will have its own namespace. This could potentially be a problem down the road if we want to update the version of CLI we vendor.

@scpeters
Copy link
Member

How do we deal with side-by-side installs in the future? We have used inline namespaces for this in our other packages, but CLI will have its own namespace. This could potentially be a problem down the road if we want to update the version of CLI we vendor.

are you concerned about conflicting header file locations or conflicting symbols? I would expect CLI11 to only be used in executables, so I wouldn't expect problems with symbols

@azeey
Copy link
Contributor

azeey commented Jan 27, 2021

Yeah, I think you're right. If we limit its usage only to executables, we shouldn't have a problem.

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

this seems to be working well enough with gazebosim/gz-transport#216

let's go ahead and merge and make a prerelease

@mjcarroll mjcarroll merged commit c195c59 into main Feb 8, 2021
@mjcarroll mjcarroll deleted the CLI11 branch February 8, 2021 18:08
@chapulina
Copy link
Contributor

let's go ahead and merge and make a prerelease

We could also use a nightly

@scpeters
Copy link
Member

scpeters commented Feb 8, 2021

we don't have to wait a day if we make a prerelease now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants