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

ninjabackend: do not use --internal exe for capturing custom target on Unix #5573

Closed
wants to merge 3 commits into from

Conversation

bonzini
Copy link
Contributor

@bonzini bonzini commented Jun 28, 2019

Using capture: true with custom_target makes "ninja -v" less useful since the command line is completely hidden from the user. It would be nice if meson --internal exe knew to print the command line itself; unfortunately support for a NINJAFLAGS command line variable, which would pass -v down to meson --internal exe, was rejected (ninja-build/ninja#1543).

However, for Unix systems ninja sends commands to the shell, which can take care of capturing output from stdout. Do not go through the serialized executables in that case. While it would be possible to use execute_wrapper, for simplicity this patch only "fixes" POSIX hosts.

…on Unix

For Unix systems, Ninja sends commands to the shell, which can take care of
capturing output from stdout.  Do not go through the serialized executables
in that case, in order to make "ninja -v" output more useful.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
For Unix systems, Ninja sends commands to the shell, which can take care of
capturing output from stdout.  Do not go through the serialized executables
in that case, in order to make "ninja -v" output more useful.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
@bonzini
Copy link
Contributor Author

bonzini commented Jun 28, 2019

Another possibility to solve this bug is to allow --internal exe to take options from the command line instead of a serialized file in the simple case, so that ninja -v produces good-looking output.

Then this could fix another issue: right now capture is not taking advantage of Ninja's restat feature, but we could keep the --internal exe wrapper and adds more smarts there to check if the output has changed; if not, avoid touching the output file.

@jpakkane
Copy link
Member

jpakkane commented Jul 5, 2019

We on purpose don't use shell metacharacters in Ninja files because they are a) not portable to Windows and b) a nightmare for string quoting. This is why the wrapper commands exist, they provide the same behaviour on all platforms thanks to Python.

If failure is the only interesting case, then printing the actual command line from the invoker script in case the command execution fails seems like the simplest solution.

@bonzini
Copy link
Contributor Author

bonzini commented Jul 7, 2019

It is not only failure, the command could do the wrong thing but still succeed; that is also very hard to debug.

I agree that Windows quoting is madness, but on POSIX systems you still have to deal with quoting.

Should I look into the alternative of invoking a wrapper script to do the capture without serializing the command to a binary file?

(The right solution would have been NINJAFLAGS but there's nothing that we can do about it).

@eli-schwartz
Copy link
Member

(The right solution would have been NINJAFLAGS but there's nothing that we can do about it).

samurai in git master (and the upcoming 0.7 release) supports SAMUFLAGS and I can confirm that it works beautifully.

@bonzini
Copy link
Contributor Author

bonzini commented Jul 10, 2019

SAMUFLAGS takes options from the environment in addition to the command line. Here we would need samurai to publish -v in the environment even if the user didn't define a SAMUFLAGS variable h{im,er}self.

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.

None yet

4 participants