New modernized recipes for zlib and bzip2 #6474

memsharded · 2021-07-23T16:13:55Z

Proposing changes for zlib and bzip2

memsharded · 2021-07-23T16:16:53Z

recipes/bzip2/all/test_package/conanfile.py



 class TestPackageConan(ConanFile):
    settings = "os", "compiler", "arch", "build_type"
-    generators = "cmake", "cmake_find_package"
+    generators = "CMakeDeps", "CMakeToolchain", "VirtualRunEnv"


This VirtualRunEnv, we could discuss if we want it explicit or implicit.
Its goal is to be able to run with shared libraries, the conan create . -o lib:shared=True works with this.
There is [conf] to automatically use it without explicit instantiation, but the invocation self.run("%s --help" % cmd, env=["conanrunenv"]) still needs to specify conanrunenv, and this is not injected by default.

madebr · 2021-07-23T16:45:01Z

@memsharded Can you split this pr into one for bzip2 and one for zlib?
Current c3i can only build one recipe per pr.

recipes/bzip2/all/conanfile.py

ericLemanissier · 2021-07-23T16:49:21Z

recipes/bzip2/all/conanfile.py

+        tools.get(**self.conan_data["sources"][self.version], destination=self.source_folder, strip_root=True)
+        # FIXME: this is failing after the export export to "src" in local folder
+        try:
+            os.rename("CMakeLists.txt", "src/CMakeLists.txt")


Suggested change

os.rename("CMakeLists.txt", "src/CMakeLists.txt")

tools.rename("CMakeLists.txt", "src/CMakeLists.txt")

Is it capable of moving files ?

Yes, but only when the target directory is already created.

Croydon · 2021-07-23T18:39:43Z

recipes/bzip2/all/conanfile.py

        self.cpp_info.builddirs.append(self._module_subfolder)
        self.cpp_info.build_modules["cmake_find_package"] = [os.path.join(self._module_subfolder, self._module_file)]
        self.cpp_info.libs = ["bz2"]
-
-        if self.options.build_executable:
-            bin_path = os.path.join(self.package_folder, "bin")


Got appending the path implicit somehow? If yes, through what?

We expect the consumers to use the VirtualRunEnv when need it instead of filling the PATH by default with stuff with might never need.

uilianries · 2021-07-23T21:17:07Z

recipes/bzip2/all/conanfile.py


-required_conan_version = ">=1.33.0"
+required_conan_version = ">=1.38.0"


First, we need to update the CI. I think we are running 1.37.2

SSE4 · 2021-07-24T09:01:18Z

recipes/bzip2/all/test_package/conanfile.py

+    def configure(self):
+        del self.settings.compiler.libcxx
+
+    def layout(self):


seems to be unused (e.g. never called), it is needed? if so, why?

It is the new layout() built-in Conan method to define the package layout, it is called internally by Conan, like other methods: https://docs.conan.io/en/latest/reference/conanfile/methods.html#layout

reading about feature, I don't get why is it needed for test package?

You can declare a layout() method in the recipe to describe the package contents, not only the final package in the cache but also the package while developing. As the package will have the same structure in the cache and in our local directory, the recipe development becomes easier.

It is not "needed" but nice to use always the same layout, more clear, and easy to explore the build files in case you need.

SSE4 · 2021-07-24T09:01:47Z

@memsharded the current CI system will not allow to build PR for several recipes (zlib + bzip2)
maybe you can split into 2 PRs?
another thing, which should make reviews easier, as there are lots of changes in lots of files: can you isolate only modernizing changes and leave cosmetic changes out (like, removing conan topic)
and yes, we don't have 1.38 yet, so it will not pass for a while.

SSE4 · 2021-07-24T12:01:58Z

recipes/bzip2/all/test_package/conanfile.py

    def test(self):
        if not tools.cross_building(self.settings):
-            bin_path = os.path.join("bin", "test_package")
-            self.run("%s --help" % bin_path, run_environment=True)
+            cmd = os.path.join(self.cpp.build.bindirs[0], "test_package")


I don't know if it's safe, e.g. if bindirs is an empty

It is a bindir of the current package. As you are declaring the layout(), you know there is always a bindir. Actually were always a bindir unless you remove it from the cppinfo.

memsharded · 2021-07-26T11:34:34Z

@madebr @SSE4 @uilianries lets wait until @davidsanfal comes up with some CI for this new new-cmake-toolchain branch, to split it or not. At the moment this is just a preview of things that could be modernized, NO intent to merge this yet in master branch

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>

uilianries · 2021-07-26T11:51:11Z

The split thing is not only due CCI restriction, but also to keep a better. 1 recipe is easier to review than 2. I don't like the idea supporting multiple recipes on same PR, but as this PR is a conception, it's okay keeping all together.

madebr · 2021-07-26T20:30:09Z

It would have been nice to see c3i succeed/fail on this 😄

lasote · 2021-08-02T06:58:18Z

recipes/bzip2/all/conanfile.py

@@ -1,8 +1,10 @@
 import os
 import textwrap
-from conans import ConanFile, CMake, tools
+from conans import ConanFile, tools


Should we start to try to replace all the tools.xxx also? (in general all the conans imports)

lasote · 2021-08-02T09:11:41Z

recipes/zlib/1.2.11/conanfile.py

-            tools.patch(**patch)
+        with tools.chdir(".."):  # FIXME: This need to go to parent folder is not very nice.
+            for patch in self.conan_data["patches"][self.version]:
+                tools.patch(**patch, base_path=self.source_folder)


See conan-io/conan#9361
apply_conandata_patches can be used to follow the layout automatically.

jgsogo · 2021-08-03T07:40:21Z

(Same comment for all these PRs)

I totally support to start testing new toolchains and build-helpers... but this is not the way/repository to do it. If we want to test them using the current Conan version in ConanCenter then we can open drafts to master with a warning about the experimental level (we can also keep up-to-date Conan version in CCI if needed). If we want to test them using Conan HEAD version then we can use a fork and different CI. Maybe we can connect it to a branch in the tribe repository or in Conan repository, which are the places where development around these new features is taking place.

With this approach, we only get notifications with errors in the backend because it is not using master branch, and the output from some of the jobs running behind the scenes is not visible to reviewers... so they cannot even know if the recipe is valid.

memsharded added 10 commits July 15, 2021 17:51

wip

058cb1f

wip

beb9ab4

wip

af53122

wip

6c8eb21

wip

d114fab

wip

2a533f0

wip

e700c5e

Merge branch 'new-cmake-toolchain' into experiment/new_cmake_integration

a428c69

wip

594f74e

new recipes for bzip2 and zlib

e84b92a

memsharded requested review from uilianries, czoido, franramirez688, SSE4 and tapia July 23, 2021 16:14

memsharded commented Jul 23, 2021

View reviewed changes

ericLemanissierBot mentioned this pull request Jul 23, 2021

[service] Potentially conflicting recipes #3571

Open

ericLemanissier reviewed Jul 23, 2021

View reviewed changes

Croydon reviewed Jul 23, 2021

View reviewed changes

uilianries reviewed Jul 23, 2021

View reviewed changes

SSE4 reviewed Jul 24, 2021

View reviewed changes

SSE4 closed this Jul 24, 2021

SSE4 reopened this Jul 24, 2021

SSE4 reviewed Jul 24, 2021

View reviewed changes

Update recipes/bzip2/all/conanfile.py

90bad33

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>

This was referenced Jul 30, 2021

[workspaces] Workspaces should be Super Builds conan-io/conan#5762

Closed

[question] How to show dependency graph with full options? conan-io/conan#9334

Open

lasote reviewed Aug 2, 2021

View reviewed changes

jgsogo deleted the branch conan-io:new-cmake-toolchain August 2, 2021 14:11

jgsogo closed this Aug 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New modernized recipes for zlib and bzip2 #6474

New modernized recipes for zlib and bzip2 #6474

memsharded commented Jul 23, 2021

memsharded Jul 23, 2021

madebr commented Jul 23, 2021

ericLemanissier Jul 23, 2021

uilianries Jul 23, 2021

Croydon Jul 23, 2021

lasote Aug 2, 2021

uilianries Jul 23, 2021

SSE4 Jul 24, 2021

memsharded Jul 26, 2021

SSE4 Jul 26, 2021 •

edited

lasote Aug 2, 2021

SSE4 commented Jul 24, 2021

SSE4 Jul 24, 2021

lasote Aug 2, 2021

memsharded commented Jul 26, 2021

uilianries commented Jul 26, 2021

madebr commented Jul 26, 2021

lasote Aug 2, 2021

lasote Aug 2, 2021

jgsogo commented Aug 3, 2021

	os.rename("CMakeLists.txt", "src/CMakeLists.txt")
	tools.rename("CMakeLists.txt", "src/CMakeLists.txt")


		required_conan_version = ">=1.33.0"
		required_conan_version = ">=1.38.0"

New modernized recipes for zlib and bzip2 #6474

New modernized recipes for zlib and bzip2 #6474

Conversation

memsharded commented Jul 23, 2021

Choose a reason for hiding this comment

madebr commented Jul 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SSE4 Jul 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SSE4 commented Jul 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memsharded commented Jul 26, 2021

uilianries commented Jul 26, 2021

madebr commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo commented Aug 3, 2021

SSE4 Jul 26, 2021 •

edited