-
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
MPFR: partial Conan 2.0 Compatibility #14687
MPFR: partial Conan 2.0 Compatibility #14687
Conversation
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.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit ecf6011mpfr/4.1.0
mpfr/4.0.2
|
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.
After some length discussion...
Because this code path is not tested but CI, it's very likely to break in subsequent PRs - it has a higher risk of negatively impacting the larger consumers base (to the benefits of only a few). It adds a burden to the reviewers and maintainers to make future changes. We, are willing to accept some extra effort for individual contributors as long as it prevents pain on the larger community.
Our recommendation for user who need to build from Git source with CCI recipes is to git archive
the commit then and pull those in with a entry in conandata.yml
such that the normal workflow is preserved.
recipes/mpfr/all/conanfile.py
Outdated
@@ -20,155 +26,174 @@ class MpfrConan(ConanFile): | |||
options = { | |||
"shared": [True, False], | |||
"fPIC": [True, False], | |||
"exact_int": ["mpir", "gmp",] | |||
"exact_int": ["mpir", "gmp",], | |||
"autogen": [True, 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.
Please remove this option and the extra dependencies 🙏
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 @prince-chrismc,
Thanks for your feedback. Certainly, I will do as you request (and we will maintain a private fork of the recipe to meet our needs), but I wanted to clarify a couple of points:
- These changes work even without using Git source. They simply enable the regeneration of the configure script and Makefiles from their .ac and .in templates. AutotoolsToolchain even has a method for this: Autotools::autoreconf().
- It is unclear to me how the "git archive" approach would work unless one first runs autoreconf manually before archiving the resulting build area. The benefit of the pending change is that it leverages Conan to automatically ensure one has a self-consistent version of the autotools (autoconf, automake, libtool, etc.) needed to actually be able to run autoreconf to create configure and Makefiles from Git source. In jettisoning this code, CCI is loosing that information and tooling.
- I think this use case points out an opportunity for a Conan enhancement, which is the ability to add tags to patches in the conandata.yml file. Such a feature would permit selective application of patches on an as needed basis. For example, apply_conandata_patches applies Windows-specific patches regardless of what platform it is running on. Had such a feature existed, I would have used it to selectively apply the autogen patches.
Thanks
recipes/mpfr/all/conanfile.py
Outdated
# Only apply this patch when building from Git source to prevent Makefile from | ||
# invoking autoconf due to to changed .ac or .m4 file | ||
patch(self, patch_file=os.path.join(self.export_sources_folder, "patches", f'{self.version}-0001-configure.ac-fixes.patch')) |
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.
Please remove this logic and the subsequent autogen
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.
Removed, but please see message above about how an enhancement to apply_conandata_patches would have made this cleaner.
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.
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.
Looking really good, just a comment about test_v1_package
Co-authored-by: Uilian Ries <uilianries@gmail.com>
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.
LGTM
It's doesn't pass v2 pipeline for the moment, while claiming to be v2 compatible. |
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 has a lot of good work so I will give it a tick, we can polish the 2.0 to pass in the next PR :)
Conan v1 pipeline ✔️All green in build 44 (
Conan v2 pipeline (informative, not required for merge) ❌
The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future. See details:Failure in build 47 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
* MPFR Conan 2.0 Compatibility * Fixed lint errors * Missed one * Fixed lint error in test_package * Cleaned up patch records for older release * Added patch descriptions * Tweaked how win_bash is set * Added more patch metadata * Bump required_conan_version * Account for path differences with Conan 2.0 * Make work on Windows (but requires VS 2022) * Use NMake to work around MSBuild bug * Differentiate MSVC from Windows * Re-ordered apply_conandata_patches * Add call to fix_apple_shared_install_name * Deleted blank line to unstick CI * Remove blank line to spur CI * Removed redundant setting of --enable-shared and --enable-static * Use test_package.c from ../test_project * Use ../test_package/test_package.c * Apply suggestion from code review * Tweak previous change * Added find_package to v1 CMakeLists.txt * Try making conan_basic_setup(TARGETS) work * Add cmake_find_package_multi generator per template * Put back add_subdirectory * Another try at making v1 work with TARGETS * Removed extra blank link * Try setting CMAKE_RUNTIME_OUTPUT_DIRECTORY instead of add_subdirectory * Try using CMAKE_CURRENT_SOURCE_DIR * Leverage CONAN_PKG::mpfr from conan_basic_setup(TARGETS) * Restore use of PROJECT_NAME * Apply suggestions from code review Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com> * Various cleanup changes * Added autogen option to shield autoconf workflow * Fixed lint error * Removed autogen support * Fixed lint warnings * Bumped required_conan_version * Bumped required_conan_version * Update recipes/mpfr/all/test_v1_package/CMakeLists.txt Co-authored-by: Uilian Ries <uilianries@gmail.com> --------- Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com> Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Uilian Ries <uilianries@gmail.com>
* MPFR Conan 2.0 Compatibility * Fixed lint errors * Missed one * Fixed lint error in test_package * Cleaned up patch records for older release * Added patch descriptions * Tweaked how win_bash is set * Added more patch metadata * Bump required_conan_version * Account for path differences with Conan 2.0 * Make work on Windows (but requires VS 2022) * Use NMake to work around MSBuild bug * Differentiate MSVC from Windows * Re-ordered apply_conandata_patches * Add call to fix_apple_shared_install_name * Deleted blank line to unstick CI * Remove blank line to spur CI * Removed redundant setting of --enable-shared and --enable-static * Use test_package.c from ../test_project * Use ../test_package/test_package.c * Apply suggestion from code review * Tweak previous change * Added find_package to v1 CMakeLists.txt * Try making conan_basic_setup(TARGETS) work * Add cmake_find_package_multi generator per template * Put back add_subdirectory * Another try at making v1 work with TARGETS * Removed extra blank link * Try setting CMAKE_RUNTIME_OUTPUT_DIRECTORY instead of add_subdirectory * Try using CMAKE_CURRENT_SOURCE_DIR * Leverage CONAN_PKG::mpfr from conan_basic_setup(TARGETS) * Restore use of PROJECT_NAME * Apply suggestions from code review Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com> * Various cleanup changes * Added autogen option to shield autoconf workflow * Fixed lint error * Removed autogen support * Fixed lint warnings * Bumped required_conan_version * Bumped required_conan_version * Update recipes/mpfr/all/test_v1_package/CMakeLists.txt Co-authored-by: Uilian Ries <uilianries@gmail.com> --------- Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com> Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com> Co-authored-by: Uilian Ries <uilianries@gmail.com>
they were introduced in conan-io#14687 but never applied also, use more compact archives identified by conan-io#21146 and https://ericlemanissier.github.io/conan-center-lint-conandata/
they were introduced in #14687 but never applied also, use more compact archives identified by #21146 and https://ericlemanissier.github.io/conan-center-lint-conandata/
Specify library name and version: mpfr/4.1.0
Migrated recipe to Conan 2.0. Resolves #14666. Note that these changes are untested on Windows with Conan 2.0 because some of the required dependencies don't build with 20 yet