-
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
Allow override_find_program to use an executable. #4055
Conversation
With this it is now possible to do foobar = executable('foobar', ...) meson.override_find_program('foobar', foobar) Which is convenient for a project like protobuf which produces both a dependency and a tool. If protobuf is updated to use override_find_program, it can be used as protobuf_dep = dependency('protobuf', version : '>=3.3.1', fallback : ['protobuf', 'protobuf_dep']) protoc_prog = find_program('protoc')
Thanks for working on this. I have a project that will at some point need this. But it needs checking against uses of that program directly in the meson file (see #3218 (comment) in the original PR for what i got from some related irc discussions). Also does this at the moment take care of adding dependencies to the backends? |
mesonbuild/interpreter.py
Outdated
'(overridden: %s)' % ' '.join(exe.command)) | ||
return ExternalProgramHolder(exe) | ||
'(overridden: %s)' % exe) | ||
return self.holderify(exe) |
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.
I don't think we should start returning ExecutableHolder
in find_program()
. That's unexpected and breaks the abstraction. For instance, users will expect to be able to call all the methods that exist on ExternalProgramHolder
, and then we need to either implement all those methods on ExecutableHolder
and find a way to not allow those methods to be called on objects that are returned from executable()
, which is impossible.
A better way would be to allow ExternalProgramHolder
to "hold" Executable
objects.
mesonbuild/interpreter.py
Outdated
@@ -1753,7 +1756,7 @@ def override_find_program_method(self, args, kwargs): | |||
if not os.path.exists(abspath): | |||
raise InterpreterException('Tried to override %s with a file that does not exist.' % name) | |||
exe = dependencies.ExternalProgram(abspath) | |||
if not isinstance(exe, dependencies.ExternalProgram): | |||
if not isinstance(exe, (dependencies.ExternalProgram, build.Executable)): | |||
# FIXME, make this work if the exe is an Executable target. | |||
raise InterpreterException('Second argument must be an external program.') |
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.
Need to remove FIXME and update the exception message.
mesonbuild/dependencies/base.py
Outdated
@@ -953,6 +953,9 @@ def __repr__(self): | |||
r = '<{} {!r} -> {!r}>' | |||
return r.format(self.__class__.__name__, self.name, self.command) | |||
|
|||
def __str__(self): | |||
return ' '.join(self.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.
Why was this implemented? We do not allow implicit str()
conversions for any objects.
mesonbuild/build.py
Outdated
@@ -1304,6 +1304,9 @@ def __init__(self, name, subdir, subproject, is_cross, sources, objects, environ | |||
# Only linkwithable if using export_dynamic | |||
self.is_linkwithable = self.export_dynamic | |||
|
|||
def __str__(self): | |||
return self.name |
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.
Why was this implemented? We do not allow implicit str()
conversions for any objects.
mesonbuild/interpreter.py
Outdated
@@ -803,6 +803,9 @@ class ExecutableHolder(BuildTargetHolder): | |||
def __init__(self, target, interp): | |||
super().__init__(target, interp) | |||
|
|||
def found(self): | |||
return True |
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.
Don't implement this method on ExecutableHolder
, see below.
Besides the changes in the review comments, as @textshell said:
An easy way to solve (2) is to error out when this happens. However, is that what people expect when the program was overriden but is also available on the system? Probably not. Subproject setups would be prone to breakage too. A better way to fix this is to make this feature an opt-in. i.e., find_program('foo', allow_built_executable_overrides: true) The name is a placeholder, bikeshedding welcome. Without that kwarg set, |
I don't think having overrides that only work with a special parameter is the way to go. I think having a override that is ignored will very likely just break in strange and hard to diagnose ways, let's try not to go there. So i prefer always returning these and erroring out when they are used synchronously. Also keep in mind, that one important use case is for these overridden programs to be used by meson modules. For example a protobuf module in the future or the qt modules (when it's adopted to look there). I think the most important synchronous use of this kind of program is checking version requirements. That is something we need to handle in the long run. For overrides i think the only way i can imagine working is to attach metadata with the override. Yes, this means projects that want to support this use case need special code, but that's still better than what i can imagine as alternatives. e.g.
and later
Of course the metadata bit can be implemented in a different PR. But i believe it's part of the process of how we want to design this. |
Why do you believe that? We already do caching and detection based on kwargs for
Let's take the following situation. subproject/foo does: meson.override_find_program('foobar', ...)` subproject/bar does: foobar = find_program('foobar')
ct = custom_target(..., command : [foobar, ...]) subproject/baz does: foobar = find_program('foobar')
f = configure_file(..., command : [foobar, ...])
This is a pretty shitty situation. If we don't want to make it opt-in, we should at least make it opt-out.
That's #1609, and yes we should implement it. |
mesonbuild/interpreter.py
Outdated
@@ -1753,7 +1756,7 @@ def override_find_program_method(self, args, kwargs): | |||
if not os.path.exists(abspath): | |||
raise InterpreterException('Tried to override %s with a file that does not exist.' % name) | |||
exe = dependencies.ExternalProgram(abspath) | |||
if not isinstance(exe, dependencies.ExternalProgram): | |||
if not isinstance(exe, (dependencies.ExternalProgram, build.Executable)): |
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.
Need to handle the case where Executable
is actually cross-built. In that case we'll want to run it with the exe wrapper, or error out if an exe wrapper is not found.
It's not about the complexity of implementing it. It's about keeping things understandable. I've often heard the mantra that goes along the lines if "if you have two different things with the same name they will get confused and leave a big mess". I believe mixing a "system install" and a subproject provided version will lead down to such a mess.
Maybe. But it's clear what happens. What if the program from the system path produces a file that is incompatible to the generated files in the other part of the build or the library also produced by the subproject. That is very hard to diagnose and is at least as bad. Or if you add a subproject with new version of a library to test if that library works and "random" parts of your build don't use that override, causing you to release a broken version? I prefer breaking the build in obvious ways to breaking it in subtle ways.
I don't think a subproject can ever have the needed situation awareness to make a sensible decision here. That might be more reasonable in a global override infrastructure.
I don't think we should tie this into a 80% percent solution without escape hatch. But maybe a |
* Don't use __str__. * Use ExternalProgramHolder for holding an executable. * Add tests of the dependencies. * Mark executable in test as native. * Error if an executable is used with run_command. I went with an error on the last item because I think that there is no non error behavior that is always correct. If a program is not available on the system, we would need an error one way or the other. The interesting case then is when the program is available on the system but is overridden anyway. I would assume that this would normally happens because the one available on the system is too old. Should we use one version during configuration and another one during the build? That sounds dangerous as there is no guarantee that the two versions are compatible. It also seems inconsistent with the fact that we error if a program is overridden after a find_program (see test cases/failing/73 override used). BTW, is there any way to test which error caused a test in the failing subdirectory to fail?
I think commit 4303e4e addresses all the requested changes. |
mesonbuild/interpreter.py
Outdated
@@ -2152,6 +2151,8 @@ def run_command_impl(self, node, args, kwargs, in_builddir=False): | |||
'or configure_file(), or a compiler object; not {!r}' | |||
if isinstance(cmd, ExternalProgramHolder): | |||
cmd = cmd.held_object | |||
if isinstance(cmd, build.Executable): | |||
raise InterpreterException('Cannot use an executable during configuration') |
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.
We need to help our users more here. The error message at least needs to say that the executable was overriden and that as it is built it is not available for execution in the build file.
mesonbuild/build.py
Outdated
@@ -1304,6 +1304,9 @@ def __init__(self, name, subdir, subproject, is_cross, sources, objects, environ | |||
# Only linkwithable if using export_dynamic | |||
self.is_linkwithable = self.export_dynamic | |||
|
|||
def desc(self): |
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.
I think this name is a bit on the cryptic side. We should try to come up with a more descriptive name here.
* Improve function name and add docstring. * Improve error message to say which program was overridden by which executable.
bafd632 should address the last round of requests. |
ping? |
As this is a behavior change this needs to be acked by @jpakkane I don't know these parts of meson well enough to say if this is technically all good now. Assuming this gets acked, what is missing is an changelog snippet and documentation. |
The common approach we do is that when something is unclear we detect it and make it a hard error and only add something later based on real world use cases. This needs the documentation fixes mentioned above but once they are done, this is ok to merge. |
|
||
This is particularly useful for fallback dependencies like protobuf | ||
that also provide a tool like protoc. | ||
|
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.
This should not be in this file. It should be in the snippets
directory as described in the chapter directly above this one.
Merged manually. |
Thanks! Sorry for the slow response. |
With this it is now possible to do
foobar = executable('foobar', ...)
meson.override_find_program('foobar', foobar)
Which is convenient for a project like protobuf which produces both a
dependency and a tool. If protobuf is updated to use
override_find_program, it can be used as
protobuf_dep = dependency('protobuf', version : '>=3.3.1',
fallback : ['protobuf', 'protobuf_dep'])
protoc_prog = find_program('protoc')