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

Support user-specified config-tool programs #13660

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

virtuald
Copy link

@virtuald virtuald commented Sep 10, 2024

The meson FAQ specifically calls out dependencies as an area where it's better for support to be embedded in meson:

First, Meson aims to provide a rich set of tools that solve specific problems
simply out of the box. This is similar to the "batteries included" mentality of
Python. By providing tools that solve common problems in the simplest way
possible in Meson we are solving that problem for everyone instead of forcing
everyone to solve that problem for themselves over and over again, often
badly. One example of this are Meson's dependency wrappers around various
config-tool executables (sdl-config, llvm-config, etc). In other build
systems each user of that dependency writes a wrapper and deals with the
corner cases (or doesn't, as is often the case), in Meson we handle them
internally, everyone gets fixes and the corner cases are ironed out for
everyone. Providing user defined functions or macros goes directly against
this design goal.

While this is great for common programs, it's probably unreasonable to expect meson to support every weird framework used by a dozen users or distributed by a closed proprietary vendor.

This feature allows users to use custom dependencies that may not be popular enough to merit special handling by meson directly. It implements the bare minimum to allow that to work -- if a user had a dependency that did something weird, they would need to write a wrapper config-tool to deal with that (which once again, the FAQ calls out as something bad, but I don't think it's avoidable for obscure dependencies).

I've executed the test project directly via meson/ninja on macOS, but haven't ran the test suite directly.

Discussion at #13650

continue
tool = potential_bin.get_command()
Copy link
Author

Choose a reason for hiding this comment

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

There's a lot of churn here, but it's just refactoring to the _check_config function. Open to ideas to make this churn less.

@virtuald virtuald force-pushed the generic-config-tool branch 2 times, most recently from 65eaed2 to 69c4e5c Compare September 10, 2024 06:36
@dcbaker
Copy link
Member

dcbaker commented Sep 10, 2024

I apologize for being negative, but I'm very skeptical of this, but maybe I can be convinced. it's about 10 lines of code to open code a config tool if it's proprietary or so niche that no one else would ever want it pretty easily:

config_tool = find_program('config-tool')
if run_command(config_tool, '--version').version_compare('< 1.0')
  error('config dependency needs at least 1.0')
endif
dep = declare_depenency(
  compile_args = run_command(config_tool, '--cflags').split(),
  link_args = run_command(config_tool, '--libs').split()
)

Our philosophy has been "handle issues for users upstream, not downstream", and Jussi is very fond of "make the right way the easy way", this feels like it violates both of those principles

@virtuald
Copy link
Author

virtuald commented Sep 11, 2024

Well, compared to two lines for a simple and well-behaved config tool your version seems a lot more verbose?

config_tool = find_program('config-tool')
dep = dependency('some-dep', method: config_tool)

There are some advantages to using the built in config-tool as well:

  • ConfigToolDependency.report_config prints out nice dependency/version information for free
  • dependency.get_variable can query the custom config-tool, whereas in your version you would have to manually set up a dictionary and setup the variables -- which maybe your custom config-tool doesn't support them, but it's less to think about

And these feel like they fall into the category of "handle issues for users upstream, not downstream"?

  • Your naive implementation of run_command().split() will work in many cases, but not in all -- the builtin config-tool uses meson's internal split_args function which does platform-specific magic and likely works in more cases
    • One way to make your naive version use this is to let declare_dependency take compile/linker args as a single string and do the splitting there; or add split_args method to string
    • Amusingly, some of my test failures for this PR are because of improper platform-specific quoting
  • The version comparison mechanism sanitizes the version so that comparisons always work

This allows users to use custom dependencies that may not be popular enough to
merit special handling by meson directly.
@dnicolodi
Copy link
Member

Do we have any example of project that ships a tool that would work as a config tool as required by the support added in this PR but that does not ship a pkg-config file? Adding support for hypothetical config tools does not seem a good strategy to me: it incentivizes creating more of those, rather than consolidating on one solution. IIRC a while back there even was the idea of adding a pkg-config pure Python implementation to Meson.

@dcbaker
Copy link
Member

dcbaker commented Sep 11, 2024

There are some advantages to using the built in config-tool as well:

Yes, through a properly written implementation in the python, that everyone gets for free by just writing dependency('foo').

Allowing people to just write dependency('foo', method : find_program('foo-config')) discourages this ever going upstream, and complicates builds if foo ever decides to change/add discovery methods.

I also share @dnicolodi's concern about the proliferation of config-tools, I'd much rather see the proliferation of pkg-config, and in the future cps files (a joint venture between developers from Meson, CMake, Boost, Conan, etc), which are standardized.

On the implementation side I'm 100% against overloading method with the output of find_program, if we decide to go for this idea, it needs to be some kind of new keyword, like config_tool_program (or whatever color to paint the bikeshed). But, I'm still not convinced this is a good idea.

@virtuald
Copy link
Author

Yes, through a properly written implementation in the python, that everyone gets for free by just writing dependency('foo').

Allowing people to just write dependency('foo', method : find_program('foo-config')) discourages this ever going upstream, and complicates builds if foo ever decides to change/add discovery methods.

Yes, users of the next meson release (could be waiting anywhere from days to months) will benefit from the builtin support for whatever library there is. But if that user wants to use it Right Now, then they have to write up their own hacky solution like your original naive solution.

If the config-tool works, then it doesn't have to go upstream, and that doesn't seem to be a particularly negative thing to me.


To be clear though, my actual goal isn't really to use some libraries config-tool, it's to write my own so I can easily insert dependencies into meson build files. Specifically, I'm exploring refactoring robotpy-build to leverage meson-python (currently is a giant setuptools mess).

The TL;DR for robotpy-build is that it autogenerates pybind11 wrappers around C++ artifacts, and also supports linking/loading libraries that are contained in other wheels -- while this last part is currently generally discouraged, I wrote a lot more about that topic on the python discussion forum.

This isn't something that pkg-config is going to be able to help with, and unless my solution to this inter-wheel dependency problem (which admittedly, doesn't come up particularly often) becomes really popular, I don't expect meson to support it directly. But it does seem reasonable to ask meson to support some sort of custom dependency lookup mechanism that isn't entirely a bunch of hacks, especially when it mostly already does.

Comment on lines +112 to +125
out = self._sanitize_version(out.strip())
# Some tools, like pcap-config don't supply a version, but also
# don't fail with --version, in that case just assume that there is
# only one version and return it.
if not out:
return (tool, None)
if versions:
is_found = version_compare_many(out, versions)[0]
# This allows returning a found version without a config tool,
# which is useful to inform the user that you found version x,
# but y was required.
if not is_found:
tool = None
return tool, out
Copy link

Choose a reason for hiding this comment

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

If this is going to be done, I think it'd be great to take the opportunity to stop calling the version variable out, because that is a terrible name.

@dcbaker
Copy link
Member

dcbaker commented Sep 12, 2024

Honestly, I'd much rather just add your config-tool to Meson than a generic mechanism. You don't seem to have a problem with needing to use "the version of Meson shipped by my distro in 1994", so telling people to use 1.6.0 doesn't seem like a big deal. We just added support for objfw as a config-tool, and even the llvm wrapper I wrote (and there's a ton of code in the config-tool setup for llvm-config), is only used by a couple of projects.

On the other hand, I tend to agree with the people on the Python discussion telling you to either rely on system provided versions or to vendor your own. I predict you're taking on a lot of headache trying to re-use libraries from those other wheels, especially when the providers of those wheels don't expect you to do that.

@virtuald
Copy link
Author

For the moment, given the pushback, I'm going to table this PR until I make more progress on my tool rewrite so I have better understanding for what I really need. My goal is to get it done by the end of September... but we'll see how it goes.

FWIW, I use meson at work, and meson's simplicity and clarity are things that I really appreciate about it, and even though I'm fighting to add more dynamic stuff to it, the struggle has also clarified how I can better go about doing things in a way that is simpler.


On the other hand, I tend to agree with the people on the Python discussion telling you to either rely on system provided versions or to vendor your own. I predict you're taking on a lot of headache trying to re-use libraries from those other wheels, especially when the providers of those wheels don't expect you to do that.

Yes, there's a lot of headache, but after the details were figured out it isn't that bad -- and it has been working since 2020. At the moment, all of the robotpy projects depend on other robotpy projects, so it's not that big of a headache (and as far as I know, robotpy [which is my project] is the only project using robotpy-build).

The way it's set up explicitly requires the other wheels to participate. I agree that trying to use libraries from wheels that aren't expecting it would be a terrible idea.

@ferdnyc
Copy link

ferdnyc commented Sep 13, 2024

The way it's set up explicitly requires the other wheels to participate. I agree that trying to use libraries from wheels that aren't expecting it would be a terrible idea.

I mean, it doesn't have to be. I can certainly envision a future, which admittedly might not come quickly, where enough norms and reasonable expectations have been defined for wheel-packaged shared libraries, that it's possible for them to be created in a reusable form. IOW, for the authors of those wheels to "pre-expect" that they'll be reused by other projects, and to publish a standardized form conducive to that.

Kind of like CMake projects figuring out how to properly package up EXPORTED targets into Foo-config.cmake files for the benefit of other CMake (and Meson!) projects to consume. It took a pretty long time for the majority of CMake projects to take that extra step, and many still don't.

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

Successfully merging this pull request may close these issues.

4 participants