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

Initial hunter support #1391 #1393

Closed
wants to merge 0 commits into from
Closed

Initial hunter support #1391 #1393

wants to merge 0 commits into from

Conversation

bazfp
Copy link
Collaborator

@bazfp bazfp commented Apr 2, 2021

Added a simple hunter support for getting dependencies. Builds fine here

The lib variables are a bit jank, but not sure what to do when the default cmake aliases them to something different than the actual library!

Build with SUPERBUILD=OFF

@@ -9,7 +9,7 @@ target_link_libraries(mavsdk_camera
PUBLIC
mavsdk
PRIVATE
tinyxml2::tinyxml2
${TINYXML2_LIB_NAME}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we use the targets with Hunter? I think that's the modern way...

Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

I am not against supporting Hunter on the principle, but I think it should work with minimal changes. Dropping the targets syntax for instance seems like a regression. And what if somebody comes tomorrow and wants to support a package manager that requires the target syntax?

My second question would be about keeping it separate: if Hunter could use the target syntax (tinyxml2::tinyxml2), it seems like this could all be done out of the project (somehow at the superbuild level), because it seems like all Hunter does is provide hunter_add_package and then override find_package. So ideally anything lower than the root (e.g. src/CMakeLists.txt) should not even know that Hunter is being used...

Does that make sense?

@JonasVautherin
Copy link
Collaborator

JonasVautherin commented Apr 2, 2021

This said, we are applying a patch to jsoncpp 1.8.4, so it may not be the same as what Hunter provides. I looked into updating to 1.9.4 to be closer to upstream, and I think we would need this PR.

That's mainly FYI, I'll try to update jsoncpp as soon as possible.

Oh and tinyxml2 merged a PR to use the namespace, but did not release it: leethomason/tinyxml2#790. If it got released and used by Hunter, that may help, right?

@julianoes
Copy link
Collaborator

@bazfp FYI tinyxml2 is updated now.

@julianoes julianoes marked this pull request as draft May 26, 2021 04:58
@JonasVautherin
Copy link
Collaborator

For the record I think that we would like to have leethomason/tinyxml2#866 and leethomason/tinyxml2#865 merged, and released in a new version of tinyxml. The @bazfp could maybe get that new version into Hunter?

@bazfp
Copy link
Collaborator Author

bazfp commented May 26, 2021

@bazfp could maybe get that new version into Hunter?

Can do, still waiting on jsoncpp release

@bazfp
Copy link
Collaborator Author

bazfp commented Aug 31, 2021

Update to this - seems upstream hunter has accepted @julianoes forked release of jsoncpp

I will revisit this to confirm it builds with hunter

@julianoes
Copy link
Collaborator

@bazfp can we move on here or should we close this? I'm just trying to clean up stuff.

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.

3 participants