-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: master
Are you sure you want to change the base?
Conversation
303f0bf
to
8db3644
Compare
continue | ||
tool = potential_bin.get_command() |
There was a problem hiding this comment.
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.
65eaed2
to
69c4e5c
Compare
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 |
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:
And these feel like they fall into the category of "handle issues for users upstream, not downstream"?
|
This allows users to use custom dependencies that may not be popular enough to merit special handling by meson directly.
69c4e5c
to
73ac85d
Compare
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. |
Yes, through a properly written implementation in the python, that everyone gets for free by just writing Allowing people to just write I also share @dnicolodi's concern about the proliferation of On the implementation side I'm 100% against overloading |
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 |
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 |
There was a problem hiding this comment.
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.
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. |
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.
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. |
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 |
The meson FAQ specifically calls out dependencies as an area where it's better for support to be embedded in meson:
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