-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
New recipe: MAVSDK #25949
base: master
Are you sure you want to change the base?
New recipe: MAVSDK #25949
Conversation
MAVSDK is a collection of libraries for various programming languages to interface with MAVLink systems such as drones, cameras or ground systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks a lot for taking the time to create this recipe and the PR, we really appreciate it :)
I've taken an initial look and most things look good, only a few structural changes are necessary, happy to help if needed
get(self, **self.conan_data["sources"][self.version]["sdk"], strip_root=True) | ||
sources = self.conan_data["sources"][self.version]["mavlink"] | ||
mavlink_dir = join(self.source_folder, "src", "third_party", | ||
"mavlink", "include", "mavlink", "v2.0") | ||
git = Git(self, folder=mavlink_dir) | ||
git.fetch_commit(sources["url"], sources["commit"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get(self, **self.conan_data["sources"][self.version]["sdk"], strip_root=True) | |
sources = self.conan_data["sources"][self.version]["mavlink"] | |
mavlink_dir = join(self.source_folder, "src", "third_party", | |
"mavlink", "include", "mavlink", "v2.0") | |
git = Git(self, folder=mavlink_dir) | |
git.fetch_commit(sources["url"], sources["commit"]) | |
get(self, **self.conan_data["sources"][self.version]["sdk"], strip_root=True) | |
sources = self.conan_data["sources"][self.version]["mavlink"] | |
mavlink_dir = join(self.source_folder, "src", "third_party", | |
"mavlink", "include", "mavlink", "v2.0") | |
git = Git(self, folder=mavlink_dir) | |
git.fetch_commit(sources["url"], sources["commit"]) |
I'd like to understand a bit better the relationship between the sdk and the c library. When building the SDK, why do we need to provide the library separatedly? Is it a dependency? If so, maybe the best approach would be to split this into two - the c library, and the SDK that depends on it
Also, using git repositories in CCI is not a good idea, we should move the library to download a file just like the SDK does. If the library does not provide tags, we can still create versions from the commits themselves if needed :)
replace_in_file(self, join(self.source_folder, f), | ||
"JsonCpp::jsoncpp", "JsonCpp::JsonCpp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be better fixed by setting this in the generate
method:
deps = CMakeDeps(self)
deps.set_property("jsoncppp", "cmake_target_name", "JsonCpp::jsoncpp")
deps.generate()
This way Conan will change the generated target name to match those used by the project, removing the need to patch the sources themselves!
|
||
# TODO: create instead patch files to simplify building versions | ||
replace_in_file(self, join(self.source_folder, "CMakeLists.txt"), | ||
'message(STATUS "Version: ${VERSION_STR}")', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? How does this help the build process?
Either way, this might be better suited to be added as part of the CMakeToolchain
variables if the variable is needed, something like this in the generate
method if this is 100% needed:
tc = CMakeToolchain(self)
tc.variables["VERSION_STR"] = f"v{self.version}"
self.requires("jsoncpp/[>=1.9.5 <2]") | ||
self.requires("tinyxml2/[>=9.0.0 <10]") | ||
self.requires("libcurl/[>=7.86.0 <8]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now version ranges are limited to a select set of libraries, check https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/dependencies.md#version-ranges, which means that of these 3, only libcurl
can use version ranges. Sorry for the confussion :)
Also, if curl's 7.86 is the minimunm required, then great! But the lower the range, the more likely this won't cause issues to other elements on the graph (We have [>=7.78 <9]
documented as the desired range by default, for example)
def config_options(self): | ||
if self.settings.os == "Windows": | ||
del self.options.fPIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def config_options(self): | |
if self.settings.os == "Windows": | |
del self.options.fPIC | |
implements = ["auto_shared_fpic"] |
will take care of this, and the missing configure()
method that handles fpic values when building shared libraries
class MAVSDKConan(ConanFile): | ||
name = "mavsdk" | ||
license = "BSD-3-Clause" | ||
author = "SINTEF Ocean" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
author = "SINTEF Ocean" |
authors in CCI recipes don't have a clear meaning, so we don't add them
if self.options.shared: | ||
copy(self, "*mavsdk*.dll", dst=join(self.package_folder, "bin"), src=join(self.build_folder, "src"), keep_path=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the cmake installation not performing this step? How come?
#include <mavsdk.h> | ||
|
||
int main() | ||
{ | ||
mavsdk::Mavsdk mavsdk; | ||
std::string version = mavsdk.version(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <mavsdk.h> | |
int main() | |
{ | |
mavsdk::Mavsdk mavsdk; | |
std::string version = mavsdk.version(); | |
#include <mavsdk.h> | |
#include <iostream> | |
int main() | |
{ | |
mavsdk::Mavsdk mavsdk; | |
std::string version = mavsdk.version(); | |
std::cout << "MAVSDK version: " << version << std::endl; |
To ensure the compiler does not optimize these calls away
'message(STATUS "Version: ${VERSION_STR}")', | ||
f'set(VERSION_STR v{self.version})\nmessage(STATUS "Version: ${{VERSION_STR}}")') | ||
|
||
replace_in_file(self, join(self.source_folder, "src/core/connection.h"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a fix for an issue known upstream? Has it been reported? Having a link for traceability would be great!
@@ -0,0 +1,8 @@ | |||
sources: | |||
"0.39.0": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this version specifically? It's over 3 years old with version 2 already out
MAVSDK is a collection of libraries for various programming languages to interface with MAVLink systems such as drones, cameras or ground systems.
Summary
Fixes #22707
Details
This is a new package intended for those who wish to be able to pull MAVSDK from conan center.
I am new to conan (and new to contributing to open source projects!) so all help and guidance is welcome.