mp-units 0.7.0 support added #5517

mpusz · 2021-05-11T19:56:17Z

Specify library name and version: mp-units/0.7.0

Well, I expect it will be an interesting review as a lot of new Conan experimental features are being used here. First of all, I found them superior to the old way to do things. Secondly, their usage makes the recipe as close as possible to the original one here https://github.com/mpusz/units/blob/master/conanfile.py which for sure will make future maintenance easier.

The old recipe had to be dropped and moved to another directory as too many changes were needed to make the new release work (there would be too many branches in a recipe to make it generic).

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

mpusz · 2021-05-11T20:13:53Z

Please advise me what is the correct way to add a new version with many changes in the recipe and other files when all directory already exists?

ericLemanissier · 2021-05-12T11:07:15Z

you need to do one PR to change the files in recipes/mp-units/all, and another to add the files in recipes/mp-units/newVersion

mpusz · 2021-05-12T12:23:36Z

@ericLemanissier, just to ensure if understood correctly. After that, I will end up with

all that serves only for 0.6.0
0.7.0 that serves for 0.7.0 and possibly next future releases
My intent was to do the opposite one. Rename all -> 0.6.0 and add new all for the latest and future releases. However, if it is not possible I can proceed as suggested. In such a case no changes to all are needed so only one PR will suffice.

ericLemanissier · 2021-05-12T12:42:47Z

You can try to do a first PR only renaming all into 0.6.0, but I'm not sure it'll be accepted by the bot. If it is, then you will be able to do a second PR to add the new versions in a folder named all without problem.

ericLemanissier · 2021-05-12T12:58:14Z

Which conan version is required for this 0.7.0 recipe ?

mpusz · 2021-05-12T15:03:10Z

1.33 should work just fine. This is why there is:

required_conan_version = ">=1.33.0"

jgsogo · 2021-05-12T15:39:53Z

recipes/mp-units/all/conanfile.py

+        tc.variables["UNITS_DOWNCAST_MODE"] = str(self.options.downcast_mode).upper()
+        tc.generate()
+        deps = CMakeDeps(self)
+        deps.generate()


Do not use CMakeToolchain and CMakeDeps yet, they are not considered stable enough. Breaking changes can happen in the following Conan releases and they can break the recipe.

Well, I must say I expected such an answer 😉. Now the question is should I "oldfashionize" (the opposite term to "modernize" 😉) and complicate my recipe or wait a bit until it will stabilize?

I know 😄 . Waiting or oldfashionizing is up to you. Stabilizing those generators might need a couple of releases at least (Conan client) and in ConanCenter we are usually one release behind the latest one... then we can start to consider it here. It is 3 months at least.

I would prefer to oldfashionize, maybe this recipe/version is useful for some other dependency. And afterward, we can come back and modernize.

conan-center-bot · 2021-05-12T17:22:08Z

All green in build 3 (ab5ddc57b06c0dc72f8a69bf4c0b2366d1a3eaae):

mp-units/0.7.0@:
All packages built successfully! (All logs)

madebr · 2021-05-12T21:08:44Z

recipes/mp-units/0.7.0/conanfile.py

+        compiler = self.settings.compiler
+        self.requires("fmt/7.1.3")
+        self.requires("gsl-lite/0.38.0")
+        if compiler == "clang" and compiler.libcxx == "libc++":


Isn't there a minimum versions of libc++ standard library that has ranges?

Not in llvm-12. Maybe llvm-13 will have a working implementation. Until then nasty hacks are needed:

https://github.com/mpusz/units/blob/43825a531fa91501277161fefd94c50a3cf0ea96/src/core/include/units/bits/external/hacks.h#L69-L214

jgsogo · 2021-05-13T06:47:54Z

recipes/mp-units/0.7.0/conanfile.py

+        else:
+            raise ConanInvalidConfiguration("Unsupported compiler")


We typically raise for known values but leave the door open to future/unknown compilers. Here we might know it doesn't work with apple-clang, but it might be possible to build using qnx, elbrus,... or some alias a company could be using to name their own build of gcc or clang.

Suggested change

else:

raise ConanInvalidConfiguration("Unsupported compiler")

elif compiler == 'apple-clang':

raise ConanInvalidConfiguration("apple-clang is not supported")

You could also copy conan-io/conan#8002

Well, for now, it is hard to tell. It was only gcc-10 a few months ago, MSVC caught up at the beginning of the year, and clang joined the party only recently with clang-12 (even though I need a lot of hacks to work around its issues). I do not know of any other compiler that is supported right now. So of course, we can get rid of the else clause here but I do not expect any other compiler to be supported in the upcoming months or even years. Well, maybe besides app-clang that probably will catch up at some point in the future. But at this time we will have 0.8.0 or more recent releases already.

prince-chrismc · 2021-05-13T12:23:18Z

recipes/mp-units/0.7.0/conanfile.py

+        compiler = self.settings.compiler
+
+        # core
+        self.cpp_info.components["core"].requires = ["gsl-lite::gsl-lite"]


The components can not have generic names since it produces can conflict with other package names.

How is that?
Won't they get namespaced with mp-units::?

From the generatored mp-unitsTargets.cmake:

if(NOT TARGET mp-units::core) add_library(mp-units::core INTERFACE IMPORTED) endif() if(NOT TARGET mp-units::isq) add_library(mp-units::isq INTERFACE IMPORTED) endif() if(NOT TARGET mp-units::si) add_library(mp-units::si INTERFACE IMPORTED) endif() # ...

if a consumer uses the pkg_config generator, then it will create core.pc, which definitely has a high conflict potential with other recipes.

Aha thanks.
So I guess it's better to pretend them with e.g. an underscore.

But those are the official names used by the library. Do I really have to provide:

self.cpp_info.components["_xxx"].names["cmake_find_package"] = "xxx" self.cpp_info.components["_xxx"].names["cmake_find_package_multi"] = "xxx"

for each and every component in the list? I personally think that sucks 😢

Also how _core is better than core if everyone will be forced to add _ prefix to their generic names? 😉

Maybe this is something for the Conan repository: how to handle file collisions in pkgconfig world due to the usage of components because of CMake consumers.

We are not going to modify all the recipes to add those changes 😄

that's probably not just a pkg-config, but any generator in general. as long as two projects define the same component such as core, files like core.pc or core-config.cmake will collide, at least if we use the same directory for all dependencies.
it's probably better to isolate them in their own directories to avoid such collisions.

it's probably better to isolate them in their own directories to avoid such collisions

How can I achieve that?

conan does not support this (currently).
This would require adding paths to the PKG_CONFIG_PATH environment variable.
I don't know whether the search order is documented.
Putting the generated .pc files in different folders or overwriting them, might result in the same behavior.

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label May 11, 2021

mpusz force-pushed the mp-units-0.7.0 branch from ebcb443 to 01f81ab Compare May 11, 2021 20:10

This comment has been minimized.

Sign in to view

ghost mentioned this pull request May 11, 2021

[service] Potentially conflicting recipes #3571

Closed

This was referenced May 12, 2021

Updatable recipes qchateau/conan-center-bot#1

Open

[request] Updatable recipes #3470

Closed

mpusz mentioned this pull request May 12, 2021

mp-units 0.6.0 switched to a 0.6.0 directory #5520

Merged

4 tasks

jgsogo suggested changes May 12, 2021

View reviewed changes

mp-units 0.7.0 added

ab5ddc5

mpusz force-pushed the mp-units-0.7.0 branch from 01f81ab to ab5ddc5 Compare May 12, 2021 17:16

conan-center-bot removed the Failed label May 12, 2021

Conan recipes "oldfashionized" + all renamed to 0.7.0

6701260

madebr reviewed May 12, 2021

View reviewed changes

madebr approved these changes May 12, 2021

View reviewed changes

github-actions bot mentioned this pull request May 12, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

jgsogo approved these changes May 13, 2021

View reviewed changes

prince-chrismc reviewed May 13, 2021

View reviewed changes

SSE4 approved these changes May 14, 2021

View reviewed changes

conan-center-bot merged commit 7d3c551 into conan-io:master May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mp-units 0.7.0 support added #5517

mp-units 0.7.0 support added #5517

mpusz commented May 11, 2021

This comment has been minimized.

This comment has been minimized.

mpusz commented May 11, 2021

ericLemanissier commented May 12, 2021

mpusz commented May 12, 2021 •

edited

Loading

ericLemanissier commented May 12, 2021

ericLemanissier commented May 12, 2021

mpusz commented May 12, 2021

jgsogo May 12, 2021

mpusz May 12, 2021

jgsogo May 12, 2021

mpusz May 12, 2021

conan-center-bot commented May 12, 2021

madebr May 12, 2021 •

edited

Loading

mpusz May 13, 2021

jgsogo May 13, 2021

prince-chrismc May 13, 2021

mpusz May 13, 2021

prince-chrismc May 13, 2021

madebr May 13, 2021

mpusz May 13, 2021

ericLemanissier May 14, 2021

madebr May 14, 2021

mpusz May 14, 2021

jgsogo May 14, 2021

SSE4 May 14, 2021

mpusz May 15, 2021

madebr May 15, 2021

		else:
		raise ConanInvalidConfiguration("Unsupported compiler")

-        else:
-            raise ConanInvalidConfiguration("Unsupported compiler")
+        elif compiler == 'apple-clang':
+            raise ConanInvalidConfiguration("apple-clang is not supported")

mp-units 0.7.0 support added #5517

mp-units 0.7.0 support added #5517

Conversation

mpusz commented May 11, 2021

This comment has been minimized.

This comment has been minimized.

mpusz commented May 11, 2021

ericLemanissier commented May 12, 2021

mpusz commented May 12, 2021 • edited Loading

ericLemanissier commented May 12, 2021

ericLemanissier commented May 12, 2021

mpusz commented May 12, 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

conan-center-bot commented May 12, 2021

madebr May 12, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

mpusz commented May 12, 2021 •

edited

Loading

madebr May 12, 2021 •

edited

Loading