-
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
gtsam: add v4.2.0a9, update package options #17933
Conversation
I detected other pull requests that are modifying gtsam/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
This comment has been minimized.
This comment has been minimized.
3cdaad6
to
eb041bc
Compare
This comment has been minimized.
This comment has been minimized.
eb041bc
to
9e7c0e3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Covers all options that could be found in GTSAM build scripts. - Added links to source code since they are hard to find otherwise. - Added `allow_deprecated` option and deprecated `allow_deprecated_since_V4` as the wording of the GTSAM variable keeps changing but the meaning has stayed the same. - Added vars: GTSAM_DEFAULT_ALLOCATOR GTSAM_SLOW_BUT_CORRECT_BETWEENFACTOR GTSAM_USE_SYSTEM_METIS GTSAM_MEX_BUILD_STATIC_MODULE
7317737
to
26e7ded
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Thanks a lot! Not approving yet as I still need to check the package_info method but ran out of time, will circle back to it in a few days, but so far so good!
- patch_file: "patches/4.2.0a9-0001-fix-invalid-include.patch" | ||
patch_description: "Fix an invalid include for pre-C++17 compatibility" | ||
patch_type: "portability" | ||
patch_source: "https://github.com/borglab/gtsam/pull/1545" |
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.
Great catch! Thanks for porting it upstream :)
recipes/gtsam/all/conanfile.py
Outdated
self.options.rm_safe("use_vendored_metis") | ||
elif not self.options.use_vendored_metis: | ||
# GTSAM expects 32-bit integers for Metis | ||
self.options["metis"].with_64bit_types = 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.
Could you post logs of this configuration building correctly please? CCI's CI won't get to try these combinations, and it would be great to have some proof for future maintenance which shows that some of the new options were tested properly
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.
Hmm, it no longer builds with the new metis
recipe, unfortunately. I'm reverting that feature for now and will possibly add it in a future PR instead.
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 @valgur! Is this happening because of the latest Metis recipe version? Could we fix that before removing that option?
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.
I think so, yes. I intend to look into the Metis issue, but I would leave that for a separate PR, since this one is quite comprehensive already. It's not affecting any existing functionality in the GTSAM recipe anyway.
It no longer builds with the new metis recipe.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 6 (
Conan v2 pipeline ✔️
All green in build 6 ( |
@@ -1,14 +1,28 @@ | |||
sources: | |||
"4.2.0a9": |
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.
@valgur I realized that the official version is 4.2a9
, should not it be better to put the same one? Am I missing something?
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.
I based it on the full version number in CMakeLists.txt: https://github.com/borglab/gtsam/blob/4.2a9/CMakeLists.txt#L10-L13
My reasoning was that the official release is going to be 4.2.0
, so this would be slightly more consistent and would avoid potential ambiguity when sorting the version numbers.
I don't have anything against renaming it to 4.2a9
either, though.
* gtsam: add missing Boost::program_options requirement See https://github.com/borglab/gtsam/blob/4.1.1/cmake/HandleBoost.cmake#L26 * gtsam: add transitive_headers/libs to deps * gtsam: drop test_v1_package * gtsam: backport a patch for C++17 compatibility * gtsam: add v4.2.0a9 https://github.com/borglab/gtsam/releases/tag/4.2a9 * gtsam: update package options - Covers all options that could be found in GTSAM build scripts. - Added links to source code since they are hard to find otherwise. - Added `allow_deprecated` option and deprecated `allow_deprecated_since_V4` as the wording of the GTSAM variable keeps changing but the meaning has stayed the same. - Added vars: GTSAM_DEFAULT_ALLOCATOR GTSAM_SLOW_BUT_CORRECT_BETWEENFACTOR GTSAM_USE_SYSTEM_METIS GTSAM_MEX_BUILD_STATIC_MODULE * gtsam: tweak formatting * gtsam: get rid of a duplicate patch * gtsam: handle `build_type_postfixes` option correctly * gtsam: correct metis lib name in 4.0 * gtsam: correct build_type_postfixes behavior * gtsam: fix a bug in 4.2.0a9 * gtsam: add 'lib' prefix to libraries on Windows * gtsam: add patch_source info to conandata.yml * gtsam: apply "lib" prefix on Windows correctly * gtsam: drop v4.0.2 To limit the load on CI. Superseded by v4.0.3. * gtsam: update onetbb and boost versions onetbb/2020.3 did not support Conan v2. * gtsam: better TBB validation * gtsam: add tcmalloc support with gperftools * gtsam: fix Eigen version check * gtsam: enable shared builds on Windows for v4.2 * gtsam: shared builds on Windows are broken in v4.2.0a9 * gtsam: document options with options_description * gtsam: add use_vendored_metis option * gtsam: simplify Conan v1 boilerplate * gtsam: improve default_allocator handling * gtsam: remove use_vendored_metis option It no longer builds with the new metis recipe. --------- Co-authored-by: Francisco Ramírez <franchuti688@gmail.com>
transitive_headers=True
andtransitive_libs=True
where necessary.allow_deprecated_since_V4
in favor ofallow_deprecated
. This param gets replaced with an incremented new param in each version of the library while the meaning stays the same, so it made more sense to me to simplify it to one stable option name.program_options
Boost component requirement.