conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
7.96k stars 952 forks source link

[feature] Add properties to MakeDeps generator #16572

Open vajdaz opened 5 days ago

vajdaz commented 5 days ago

What is your suggestion?

I would like to access custom properties from GNU Make files generated by the MakeDeps generator. If the receipt defines custom properties in the package_info() method then those properties should appear as make variables, too. The pattern of the make variable would be

CONAN_PROPERTY_{dependency_name}_{property_name}

where both, dependency_name and property_name would be uppercase. Dots in property_name would be replaced by underscores.

Here is my first take: https://github.com/conan-io/conan/commit/19daca6aaebb3a4d4242e77686d1c2105ef04216

I am aware that in this implementation has the assumption that custom properties are always strings. Is there such a constraint. or can properties contain data of any type?

My questions:

Have you read the CONTRIBUTING guide?

vajdaz commented 4 days ago

On second thought I would drop the replacement of dots with underscores. Dots are valid charactes in GNU Make variable names.

memsharded commented 4 days ago

Hi @vajdaz

Thanks for your suggestion

What would be exactly the use case? What kind of information are you propagating from dependencies to the consumers via properties?

The properties are intended to be mapped to somehow "native" things in the consumer build system. When a property like cmake_target_name is defined is because the CMake build system understand that thing.

I think this could be a valid request and probably can be moved forward, as it would be very low risk

On second thought I would drop the replacement of dots with underscores. Dots are valid charactes in GNU Make variable names.

If there are other Make systems that this generator could be valid beyond GNU Make, then it might be worth using underscores.

vajdaz commented 3 days ago

Hi @memsharded ,

I have packages that contain more than one library files which have circular dependencies (let's imagine liba.a and libb.a). When you link such libraries, you must put them between appropriate linker options that make the linker iterate over them several times when the symbols are resolved. So instead of having the linking options

-L/my/lib/path -la -lb

you will have to use following linking options

-L/my/lib/path -Wl,--start-group -la -lb -Wl,--end-group

I signal this situation by means of a custom property. Currently I have a custom makefile generator that creates a makefile similar to what MakeDeps creates but additionally it creates variables for the custom properties. My build system integration receives the info about circular dependencies via this mechanism and can prepend/append the mentioned linker options.

In my current implementation I have a property for the linker options that should be prepended and appended. So in the receipt I have

    def package_info(self):
        # has circular dependencies
        self.cpp_info.set_property("ldflags_prepend", "-Wl,--start-group")
        self.cpp_info.set_property("ldflags_append", "-Wl,--end-group")

My generator creates the make variables CONAN_PROPERTY_LDFLAGS_PREPEND_MYLIBNAME and CONAN_PROPERTY_LDFLAGS_APPEND_MYLIBNAME. In the build system integration I use these variables to create the linker command (together with the well known other make variables, like CONAN_LIB_DIRS_, CONAN_LIBS_, etc.).

I am also aware that this is not very portable. I could abstract the linker options by having an abstract property

    def package_info(self):
        self.cpp_info.set_property("has_circular_dependencies", "True")

and let the build system decide what to do with this information.

But this details do not matter. The point is, I want to access custom property values set in the receipt in my GNU Make based build system to control linking behavior in special cases.

memsharded commented 3 days ago

I am also aware that this is not very portable. I could abstract the linker options by having an abstract property

Indeed, this is something that we have wanted to improve for a long time, improving the cpp_info model or adding new "built-in" properties that other build systems as CMake coudl also use.

If you have some other cases besides this one, then it might be possible to move this feature forward now, otherwise we are trying to improve and address this model in https://github.com/conan-io/conan/pull/15866 (very early stage) so maybe it would be better to wait until the definition of a built-in way for link groups, if you have your own generator at the moment and is good by now, then it wouldn't be very painful?

vajdaz commented 3 days ago

My comment on the lack of portabiliy of my example implementation for my current use case refers only to my the implementation itslef. The feautre (being able to access properties by make variables) would be a generic feature in my opinion.

I don't really get the connection between #15866 and this feature. I wouldn't see such a property (like "this package has cicrular dependencies") as a built-in property, because t is too platform specific. Solaris linker have no problems linking static libs with circular dependencies. How Microsoft linker behave, I don't know.

Currently I have no other use cases.

About the painfulness of not having this feature. MakeDeps is much better than my generator. I can also copy MakeDeps, rename it and patch it. Then I have a nice generator with my feature. But you know how it is. Conan will change, then my "fork" will break, I will have to patch it again... Everything would be so simple if this nice little feature would flow back upstream, and I could use the built-in MakeDeps without having an own patched copy. But I also can understand your wariness. New features, new problems... You decide. I would also invest some time to complete this feature if you would give me some directions (for example, I discovered _makefiy() and adjusted the code to use it to transform the property name to a make variable name segment).

memsharded commented 3 days ago

I don't really get the connection between https://github.com/conan-io/conan/pull/15866 and this feature. I wouldn't see such a property (like "this package has cicrular dependencies") as a built-in property, because t is too platform specific. Solaris linker have no problems linking static libs with circular dependencies. How Microsoft linker behave, I don't know.

The fact that libraries have circular dependencies isn't really platform specific, the libraries have that "topology". It is the linkers of different platforms that might have issues or not with that, and might need extra inputs or not. But defining that some libraries form a cycle can be part of the cpp_info model, as it is intrinsic to those libraries.

Ok, I think it would be no risk to have MakeDeps translate properties to CONANPROPERTY variables, if you want to propose a PR, we will try to help to move it forward.

vajdaz commented 5 hours ago

Above a draft PR.

I added some tests by adding them to existion ones. Is this ok, or should I create new test cases? What should be with documentation? Is this something that has to be documented? Where?

Let me know what you think about the PR.