Upgrade to Conan 2 #747
Upgrade to Conan 2 #747
Conversation
This fixes issue #718 (and incidentally fixes #702 too). The modified `<gsl/util>` imports and various changes from `int` to `std::size_t` are a consequence of using a newer version of MS GSL. I changed the dependency version so we can use an upstream CMake package configuration file which is compatible with the one generated by Conan 2's `CMakeDeps` generator. This should enable us to use the same `find_package()` commands and target names regardless of whether the build is running under Conan or not. This also removes the ability to run Conan from within CMake, because upstream hasn't released a version of the cmake-conan helper script with full support for all Conan 2 features yet. (Notably, support for the `CMakeToolchain` generator is missing.)
|
Seems like this PR closes several build-related issues. I linked them now. |
| self.options["xerces-c"].shared = self.options.shared | ||
| if self.options.shared: | ||
| self.options.rm_safe("fPIC") | ||
| self.options["*"].shared = self.options.shared |
There was a problem hiding this comment.
Is this for setting shared option for all dependencies? Also can you check if ["*"] is correct?
There was a problem hiding this comment.
Basically, it's the same as setting the top-level attribute
default_options = {"*:shared": True}It's a suggestion for what the shared option for all dependencies (that have such an option) should be, but it can be overridden by the user or superseded by other packages in the dependency tree.
There was a problem hiding this comment.
And it is correct, it was taken from an example on this page of the Conan docs: https://docs.conan.io/2.0/reference/conanfile/methods/configure.html
| self.requires("boost/[~1.81]", transitive_headers=True) # Required by Thrift | ||
| else: | ||
| self.requires("boost/[>=1.71]", transitive_headers=True) |
There was a problem hiding this comment.
It seems libcosim consumers need to explicitly declare self.requires("boost/[~1.81]") to use boost in their code, for example boost::timer. Otherwise linking fails.
I probably not fully understand the "traits" from the conan doc yet, but may be transitive_libs allows the consumers to link boost declared here? So that "downstream" (e.g. libcosim consumers) can also use the same boost version in their code, without accidentally declaring mismatching boost version.
There was a problem hiding this comment.
The way I see it, consumers who use Boost directly in their own code should list it as an explicit dependency and not rely on the indirect dependency from libcosim. However, if they only need Boost to be able to link with libcosim, then it should be handled automatically. I'm actually not sure whether transitive_libs is required in that case; I'll try to find out.
There was a problem hiding this comment.
I think the reason I specified transitive_headers is because libcosim headers include some Boost headers, so client code must have them on the include path.
There was a problem hiding this comment.
It turns out you were right about this, @davidhjp01. Since we use Boost in some libcosim headers, and more specifically Boost libraries that have binary components (i.e., are not header-only), such as Boost.Log, then we need to have transitive_libs=True here. Otherwise, client code won't have access to those binary components for linking. I'll make a new pull request.
There was a problem hiding this comment.
Should be resolved by #753.
… the `package_info` method. This is because otherwise CMakeDeps incorrectly generates the dependency in the target cmake file as `libcosim::libcosim`. (i.e. <consumer>-Target-release.cmake, see https://docs.conan.io/2/reference/tools/cmake/cmakedeps.html#generated-files). This would not be found via `find_package(libcosim REQUIRED)`.
Edit |
gcc9 builds are not available from conancenter. Built libraries are uploaded to osp jfrog.
|
I have addressed the most of the issues related to the failing builds. I suggest to merge and release libcosim and proxyfmu so that other projects dependent on conan2 library can consume them. |
|
@davidhjp01, sorry for dropping the ball on this at the end of last year, and huge thanks to you for picking it up! It's really good to finally get this merged. |
|
Following up on your question (too late for this PR, unfortunately):
Yes, I think that sounds like a good idea. I don't think I can remember a case where the release builds succeeded while the debug builds failed or vice versa. |
This fixes issue #718, “Conan v2 compatibility”. (Note that it also destroys Conan 1.x compatibility – it was simply too difficult to maintain support for both major versions.)
Some collateral changes warrant further explanation:
<gsl/util>includes and various changes frominttostd::size_tare a consequence of using a newer version of MS GSL. I changed the dependency version so we can use an upstream CMake package configuration file which is compatible with the one generated by Conan 2'sCMakeDepsgenerator. This should enable us to use the samefind_package()commands and target names regardless of whether the build is running under Conan or not.CMakeDeps, which was my main motivation. The MS GSL package mentioned in the previous point is one such. (Also, the GitHub runners are such a noisy and unpredictable build environment that I figured this would be more efficient than try to fix compatibility issues there.)CMakeToolchaingenerator is missing.)The pull request depends on open-simulation-platform/proxy-fmu#92, which should be merged first. Then, the
osp/testing-…channel should be replaced in theproxyfmurequirement specification inconanfile.pybefore merging the present PR.Incidentally, this PR also fixes issue #702.