Skip to content

interpreter: add depend_files kwarg to run_command() #15016

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

interpreter: add depend_files kwarg to run_command() #15016

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mesonbuild/interpreter/interpreter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@ def validate_arguments(self, args, argcount, arg_types):
varargs=(build.Executable, ExternalProgram, compilers.Compiler, mesonlib.File, str))
@typed_kwargs(
'run_command',
DEPEND_FILES_KW.evolve(since='1.X.0'),
KwargInfo('check', (bool, NoneType), since='0.47.0'),
KwargInfo('capture', bool, default=True, since='0.47.0'),
ENV_KW.evolve(since='0.50.0'),
Expand Down Expand Up @@ -833,6 +834,9 @@ def run_command_impl(self,
else:
raise InterpreterException(overridden_msg.format(a.name, cmd.description()))

for f in kwargs['depend_files']:
self.add_build_def_file(f)
Copy link
Member

Choose a reason for hiding this comment

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

Given the kwarg is part of func_run_command I don't know why it's being read+used in the totally different run_command_impl, but it is the cause of the CI failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

func_run_command() just forwards to run_command_impl(), and the latter shares the type-definition of the kwargs argument.

I think the CI failure is because this should read:

for f in kwargs.get('depend_files', []):
            self.add_build_def_file(f)

I wonder, though, why this didn't trigger errors in my local tests. I used:

./run_project_tests.py --only "common/33 run program"

And this worked fine. Hmm. But maybe I am missing something here. How exactly would you adjust func_run_command() to support the option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I got this right, then func_run_command() properly fills in the default value in kwargs, and as such I get an empty depend_files when called via run_command() in meson build files. However, there are 2 other users of the actual implementation of func_run_command(), which is run_command_impl(), and those do not fill in defaults but pass kwargs manually (and explicitly specify all kwargs).

Do you prefer to update explicit users to specify all options, or should I use the kwargs.get(..., []) style?

Copy link
Member

Choose a reason for hiding this comment

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

I already told you what I want, but I can explain the distinction between the two and why your analysis doesn't apply in this case. Sorry for not elucidating earlier.

We have two functions:

  • a DSL entry-point with functionality relevant to the DSL
  • a backend refactoring function so that we can share the logic for "spawn a command" in various places where this is relevant

Most of the logic for the former (technically before this PR, all of it) needs to be forwarded regardless, because it tunes the details of the command spawning itself. It isn't work that occurs independently beforehand. Even though other uses don't necessarily need to control these details, run_command_impl must somehow accept knobs for e.g. env, and it could be done via a dict kwarg or via separate arguments but we happened to use this.

depend_files is unnecessary to expose to run_command_impl, it can live wholly inside of func_run_command which means avoiding unnecessary complexity inside other internal callers of run_command_impl.


# If any file that was used as an argument to the command
# changes, we must re-run the configuration step.
self.add_build_def_file(cmd.get_path())
Expand Down
Loading