Skip to content

Conversation

@parisiale
Copy link
Contributor

Removing leatherman's git submodule. Updating CMake, Appveyor, and
Travis configuration scripts. Updating leatherman's dependency on
README.

@parisiale parisiale force-pushed the PCP-209 branch 3 times, most recently from 002439f to e5a5577 Compare January 18, 2016 16:06
@mruzicka
Copy link
Contributor

LGTM.
@richardc Can you please review this too?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems new, and not something we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's needed by the leatherman 0.3.5 package we're using; its leatherman/lib/cmake/leatherman/LeathermanConfig.cmake includes leatherman_component(curl) which was breaking the build. See https://ci.appveyor.com/project/puppetlabs/cpp-pcp-client/build/1.0.116.

I was planning to get this in and then make a ticket to fix this useless dependency.
Do you have an alternative to fix? Perahps, @branan or @MikaelSmith, I'm doing something wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's a binary artifact of leatherman? I guess we can live with it for now, though we could consider using a source build of leatherman as we do under travis?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a mistake in Leatherman. This PR does use a binary artifact on both platforms, but it's pulling in a required dependency on libcurl even though you're not requesting the Leatherman.curl component. I'll file a ticket to fix it.

https://tickets.puppetlabs.com/browse/LTH-76

@MikaelSmith
Copy link
Contributor

Looks good, just a minor nit.

Removing leatherman's git submodule. Updating CMake, Appveyor, and
Travis configuration scripts. Updating leatherman's dependency on
README.
@parisiale
Copy link
Contributor Author

Thanks. I removed the BOOST_STATIC def.

mruzicka added a commit that referenced this pull request Jan 19, 2016
(PCP-209) Unvendor and rely on installed leatherman
@mruzicka mruzicka merged commit 4b64604 into puppetlabs:master Jan 19, 2016
@parisiale
Copy link
Contributor Author

Just for reference, I've created PCP-246 to follow LTH-76.

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.

4 participants