Skip to content
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.

Sign up for GitHub

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

M4 Conan 2.0 compatibility #14544

Merged
merged 13 commits into from
Jan 27, 2023
Merged

M4 Conan 2.0 compatibility #14544

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7e61249
M4 Conan 2.0 compatibility
System-Arch Dec 2, 2022
1fddd2a
Restore v1 support for M4 env. var.
System-Arch Dec 3, 2022
eaba4f9
Set env_info.PATH in Conan 2 compatible manner
System-Arch Dec 4, 2022
a341f34
Removed cond. logic for self.output.PATH.append (see conan-io/conan#1…
System-Arch Dec 8, 2022
020fa8c
Added workaround for Conan issue #12685
System-Arch Dec 23, 2022
1f0fe19
Tweaked how win_bash is set
System-Arch Dec 23, 2022
1d63c0d
Removed workaround for Conan issue #12685 (fixed in beta8)
System-Arch Dec 27, 2022
7ef9e63
Fixed lint issue
System-Arch Dec 30, 2022
d401d6d
Conan 2.0 renamed output in self.run()
System-Arch Jan 3, 2023
acb9b9f
Bumped required_conan_version to clear "failed" label
System-Arch Jan 13, 2023
4dd6071
Removed optional bootstrap support per CCI decision
System-Arch Jan 18, 2023
fdbd3e6
Removed import for chdir
System-Arch Jan 20, 2023
f97567a
Removed handling of COPYING as symlink (which it is in Git repo)
System-Arch Jan 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions recipes/m4/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
from conan import ConanFile
from conan.tools.env import VirtualBuildEnv
from conan.tools.files import apply_conandata_patches, copy, export_conandata_patches, get, rmdir, save
from conan.tools.files import apply_conandata_patches, copy, export_conandata_patches, get, chdir, rmdir, save
from conan.tools.gnu import Autotools, AutotoolsToolchain
from conan.tools.layout import basic_layout
from conan.tools.microsoft import is_msvc, unix_path
from conan.tools.scm import Version
import os
import shutil

required_conan_version = ">=1.52.0"
required_conan_version = ">=1.53.0"


class M4Conan(ConanFile):
name = "m4"
package_type = "application"
description = "GNU M4 is an implementation of the traditional Unix macro processor"
topics = ("macro", "preprocessor")
homepage = "https://www.gnu.org/software/m4/"
Expand Down Expand Up @@ -87,20 +89,32 @@ def generate(self):

def _patch_sources(self):
apply_conandata_patches(self)
# dummy file for configure
help2man = os.path.join(self.source_folder, "help2man")
save(self, help2man, "#!/usr/bin/env bash\n:")
if os.name == "posix":
os.chmod(help2man, os.stat(help2man).st_mode | 0o111)
if shutil.which("help2man") == None:
# dummy file for configure
help2man = os.path.join(self.source_folder, "help2man")
save(self, help2man, "#!/usr/bin/env bash\n:")
if os.name == "posix":
os.chmod(help2man, os.stat(help2man).st_mode | 0o111)

def build(self):
# 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we don't build from git repo...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@SpaceIm SpaceIm Dec 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to our shepherds meetings to revisit as a topic :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self._patch_sources()
autotools = Autotools(self)
autotools.configure()
autotools.make()

def package(self):
copy(self, "COPYING", src=self.source_folder, dst=os.path.join(self.package_folder, "licenses"))
if os.path.islink(os.path.join(self.source_folder, "COPYING")):
# Use mkdir and copyfile when "COPYING" is a symlink as it is when building with source from Git repo
os.mkdir(os.path.join(self.package_folder, "licenses"))
shutil.copyfile(os.path.join(self.source_folder, os.readlink(os.path.join(self.source_folder, "COPYING"))),
os.path.join(self.package_folder, "licenses", "COPYING"))
else:
copy(self, "COPYING", src=self.source_folder, dst=os.path.join(self.package_folder, "licenses"))
System-Arch marked this conversation as resolved.
Show resolved Hide resolved
autotools = Autotools(self)
# TODO: replace by autotools.install() once https://github.com/conan-io/conan/issues/12153 fixed
autotools.install(args=[f"DESTDIR={unix_path(self, self.package_folder)}"])
Expand All @@ -116,8 +130,3 @@ def package_info(self):
self.runenv_info.define_path("M4", m4_bin)
self.buildenv_info.define_path("M4", m4_bin)

# TODO: to remove in conan v2
bin_path = os.path.join(self.package_folder, "bin")
self.output.info(f"Appending PATH environment variable: {bin_path}")
self.env_info.PATH.append(bin_path)
self.env_info.M4 = m4_bin
System-Arch marked this conversation as resolved.
Show resolved Hide resolved