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.

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

[feature] Add properties to MakeDeps generator #16572

Closed
1 task done
vajdaz opened this issue Jun 29, 2024 · 9 comments · Fixed by #16613
Closed
1 task done

[feature] Add properties to MakeDeps generator #16572

vajdaz opened this issue Jun 29, 2024 · 9 comments · Fixed by #16613

Comments

@vajdaz
Copy link
Contributor

vajdaz commented Jun 29, 2024

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: 19daca6

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:

  • Would such kind of a merge request have any chance to be merged? (Of course with docs and tests etc.)
  • Any ideas how to cope with non-string property values if they are allowed?
  • Where should such a feature be documented?
  • What kind of tests would make sense here?

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@vajdaz
Copy link
Contributor Author

vajdaz commented Jun 30, 2024

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

@memsharded
Copy link
Member

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
Copy link
Contributor Author

vajdaz commented Jul 1, 2024

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
Copy link
Member

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 #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
Copy link
Contributor Author

vajdaz commented Jul 1, 2024

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
Copy link
Member

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.

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 CONAN_PROPERTY_ variables, if you want to propose a PR, we will try to help to move it forward.

@vajdaz
Copy link
Contributor Author

vajdaz commented Jul 4, 2024

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.

@memsharded
Copy link
Member

PR looks good, tests new checks looks good too, thanks!

The documentation, adding a few lines explaining it in https://docs.conan.io/2/reference/tools/gnu/makedeps.html would be enough.

@vajdaz
Copy link
Contributor Author

vajdaz commented Jul 5, 2024

Created PR for docs: conan-io/docs#3794

Kept them draft because the unclarified question about escaping spaces in property values. If we agree on the right solution, I will remove the draft flags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants