M4 Conan 2.0 compatibility #14544
M4 Conan 2.0 compatibility #14544
Conversation
This comment has been minimized.
This comment has been minimized.
recipes/m4/all/conanfile.py
Outdated
| # Support building with source from Git reop | ||
| with chdir(self, self.source_folder): | ||
| command = "./bootstrap" | ||
| if os.path.exists(command) and not os.path.exists("configure"): | ||
| self.run(command) |
There was a problem hiding this comment.
but we don't build from git repo...
There was a problem hiding this comment.
Hi @SpaceIm,
While CCI doesn't build from Git source, I hope that shouldn't preclude the ability to use these recipes in other contexts that do.
There was a problem hiding this comment.
Sure it should. This recipe builds with a specific source, trying to support another one is a burden maintenance and won't be tested anyway. To be able to build from another source, you have to change recipe anyway, so I don't see the point of these modifications in conan-center.
Please do not resolve open discussions.
There was a problem hiding this comment.
Hi @SpaceIm,
Sorry for marking discussion as resolved. I'm still learning the proper etiquette as it was unclear to me when issues get marked as resolved.
Regarding the technical issues, with Conan 2.0, it is easy to use the "build" and "export-pkg" commands to maintain a private repo while using Git for the source along with the CCI recipes when they are compatible with building directly from source. Most are, but a few, like this one, require an extra bootstrapping step to account for the build steps that were taken prior to creating the distribution tar balls. Building directly from Git-maintained source is preferable when one needs to fix bugs or make custom modifications to the libraries. I don't think the extra few lines of recipe code cause harm, as they eliminate the need to maintain private forks for the recipes themselves and the functionality may prove useful to others who wish to do the same.
There was a problem hiding this comment.
@System-Arch the point is, we need to keep the behavior as building from CCI to keep the reproducibility. Using bootstrap you are running a case which is not required for the CI. Please revert it
There was a problem hiding this comment.
@uilianries,
This bootstrap logic is only active if one is building from a Git repo in which case the build area will lack a pre-existing "configure" file. If that file is already present (as is the case with a tar ball), the bootstrap step is skipped and this extra couple of lines of code become a no-op. Thus I don't believe they have any impact on "reproducibility."
On a deeper philosophical note, is the goal of the CCI recipes only to populate CCI or is it to encourage the wide-scale adoption of Conan (and especially the new and improved Conan 2.0, which has better intrinsic support for interfacing with Git). I have been assuming it is the latter and that the CCI community would applaud efforts to make recipes as broadly applicable as possible. Otherwise, organizations are forced to maintain private recipe forks, which reduces the incentive to upstream their efforts back to CCI. Thanks for reconsidering your position here.
There was a problem hiding this comment.
I added this to our shepherds meetings to revisit as a topic :)
There was a problem hiding this comment.
I missed last weeks meeting but the team decided against this, @jcar87 was supposed to add a comment :)
for now let's remove these so we can get the amazing work in hopefully once some of the other problems are cleared there's more room this things
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.
|
Hi @prince-chrismc, |
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.
|
I detected other pull requests that are modifying m4/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. |
ghost
mentioned this pull request
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.
@System-Arch Yes, there are two main points:
|
* M4 Conan 2.0 compatibility * Restore v1 support for M4 env. var. * Set env_info.PATH in Conan 2 compatible manner * Removed cond. logic for self.output.PATH.append (see conan-io/conan#12673) * Added workaround for Conan issue conan-io#12685 * Tweaked how win_bash is set * Removed workaround for Conan issue conan-io#12685 (fixed in beta8) * Fixed lint issue * Conan 2.0 renamed output in self.run() * Bumped required_conan_version to clear "failed" label * Removed optional bootstrap support per CCI decision * Removed import for chdir * Removed handling of COPYING as symlink (which it is in Git repo)
Specify library name and version: m4/1.4.19
Updated so that test_package passes with Conan 2.0 (resolves #14540). Also added support to be able to build if source is obtained from Git repo (rather than pre-digested tar ball).