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

Propagate cmake arguments to superbuild dependencies #876

Merged
merged 2 commits into from
Oct 17, 2019

Conversation

JonasVautherin
Copy link
Collaborator

This propagates the cmake arguments to the superbuild dependencies. It works by extracting the arguments into ${CL_ARGS} (before project() is called!), and then propagating that to the dependencies (through build_target.cmake).

The CMAKE_ARGS defined in each dependency's CMakeLists.txt are overriding values. For instance, we always want to build our dependencies as static libs, and therefore we set BUILD_SHARED_LIBS=OFF, no matter what the command line argument was. That's why ${CL_ARGS} appears at the top of the lists of arguments.

Note that we still need to manually set CMAKE_PREFIX_PATH and CMAKE_INSTALL_PREFIX, because they come from our DEPS_INSTALL_PATH option, and are therefore independent from the cmake command line arguments.

As an example, if we now set an Android-specific argument like ANDROID_STL, it automatically gets propagated to all our dependencies.

I believe that's an improvement, but I'm open to discussion if you think it is convoluted, @julianoes 😅.

julianoes
julianoes previously approved these changes Oct 15, 2019
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

I suppose that's fine.

@JonasVautherin
Copy link
Collaborator Author

Finally passed the CI, there were connection issues yesterday.

Two things to note:

  • I had to hardcode tinyxml2 to CMAKE_BUILD_TYPE=Release, because otherwise it generates libtinyxml2d.a and Ubuntu 16 doesn't find it. We could probably fix the find module, but I think it's not a big deal anyway.
  • The path given to CMAKE_TOOLCHAIN_FILE now has to be absolute, because it is propagated to the dependencies. For some reason it was working (and is still working in current develop) with a relative path, but I really don't understand why it works. Or why the relative path doesn't work anymore.

@julianoes: As mentioned already, I'm not completely sure about this change. I like the idea of not having to go through all the dependencies everytime I want to support a new option (like -DANDROID_STL in this case), but in the other hand this solution is not as clean as I was hoping it to be. Your call then.

Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Let's try it!

@julianoes julianoes merged commit 91e634c into develop Oct 17, 2019
@JonasVautherin JonasVautherin deleted the propagate-cmake-args branch October 17, 2019 15:09
irsdkv pushed a commit to irsdkv/MAVSDK that referenced this pull request Oct 24, 2019
* propagate cmake arguments to superbuild dependencies

* update tinyxml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants