Added warning if `check` kwarg of `run_command` is not given. #9304

Volker-Weissmann · 2021-09-27T10:08:38Z

codecov · 2021-09-27T10:18:10Z

Codecov Report

Merging #9304 (4401a89) into master (32b7cbd) will decrease coverage by 3.13%.
The diff coverage is n/a.

❗ Current head 4401a89 differs from pull request most recent head d76b5e1. Consider uploading reports for the commit d76b5e1 to get more accurate results

@@            Coverage Diff             @@
##           master    #9304      +/-   ##
==========================================
- Coverage   67.29%   64.15%   -3.14%     
==========================================
  Files         394      198     -196     
  Lines       85344    42662   -42682     
  Branches    17465     8729    -8736     
==========================================
- Hits        57432    27371   -30061     
+ Misses      23231    12994   -10237     
+ Partials     4681     2297    -2384

Impacted Files	Coverage Δ
wrap/wraptool.py	`40.80% <0.00%> (-0.72%)`	⬇️
msubprojects.py	`55.41% <0.00%> (-0.41%)`	⬇️
dependencies/pkgconfig.py	`58.44% <0.00%> (-0.31%)`	⬇️
mesonlib/universal.py	`75.76% <0.00%> (-0.09%)`	⬇️
backend/ninjabackend.py	`71.17% <0.00%> (-0.02%)`	⬇️
interpreter/interpreter.py	`81.58% <0.00%> (-0.01%)`	⬇️
mesonbuild/scripts/scanbuild.py
mesonbuild/templates/dlangtemplates.py
mesonbuild/interpreterbase/interpreterbase.py
mesonbuild/cmake/traceparser.py
... and 156 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32b7cbd...d76b5e1. Read the comment docs.

eli-schwartz · 2021-09-27T12:28:12Z

test cases/unit/59 introspect buildoptions/meson.build

@@ -9,7 +9,7 @@ foo = false

 executable(target_name, target_src, build_by_default : foo)

-r = run_command(find_program('c_compiler.py'))
+r = run_command(find_program('c_compiler.py'), check: false)
 if r.returncode() != 0
  error('FAILED')


This one should should probably be check: true, lol

Make sure we still have at least one place where we test with check: false and a command that actually fails.

33 run program does that several times. assert(ret.returncode() == 0, 'failed to run python3: ' + ret.stderr())

If you're looking for one that is expected to error out and raises a testsuite failure if ret.returncode() == 0, this doesn't do that either.

If you run rg -F "check: false" test\ cases/, you'll find this pattern at a couple of places:

r = run_command(..., check: false) if r.returncode() != 0 error('some error') endif

(sometimes with an if clause, sometimes with an assert). This can be removed if we set check to true, but this would change the error message.
In some places (like error('FAILED')), this error message isn't better than the default, but in some other places it is.
I think you should decide which places should keep their custom error messages and which places can use the default error message.

eli-schwartz · 2021-10-04T03:15:32Z

test cases/common/220 fs module/meson.build

@@ -15,7 +15,7 @@ if not is_windows and build_machine.system() != 'cygwin'
  # * Windows user permissions to create symlinks, and/or Windows in Developer mode
  # so at this time the symlink test is skipped for Windows.
  symlink = meson.current_build_dir() / 'a_symlink'
-  run_command('ln', '-s', meson.current_source_dir() / 'meson.build', symlink)
+  run_command('ln', '-s', meson.current_source_dir() / 'meson.build', symlink, check: true)


This is a great error right here!

The test runner configures this test case correctly, and it succeeds in generating, running, and in theory passing. But the test runner also checks that the backend will correctly force regeneration for build/test, by updating the utime of meson.build.

On this second run, the ln program fails because

--- stderr --- /usr/bin/ln: failed to create symbolic link '/tmp/b d19af95f3b/a_symlink': File exists

(after your latest push)

Note that the additional -f flag to ln would make it overwrite the existing symlink rather than erroring.

eli-schwartz · 2021-10-10T00:49:31Z

Meson PRs cannot include merge commits. Please rebase instead.

Also the "simple bug fix" commit should be squashed into the commit which it is fixing.

Volker-Weissmann · 2021-10-10T20:02:13Z

Meson PRs cannot include merge commits. Please rebase instead.

Also the "simple bug fix" commit should be squashed into the commit which it is fixing.

I fought with git for a while and ended up saving a code-diff elsewhere, deleting my local repo, and force-pushing a commit created from the code-diff.

Relevant xkcd: https://xkcd.com/1597/

eli-schwartz · 2021-10-10T20:06:14Z

That sounds complicated... I'd just git branch newbranch origin/master and apply the code diff to that, and then force push. No need to delete the repo.

Anyway, changes seem okay, let's see what CI thinks.

Volker-Weissmann · 2021-10-10T20:08:49Z

That sounds complicated... I'd just git branch newbranch origin/master and apply the code diff to that, and then force push. No need to delete the repo.

Then I would have to create a new PR, because this PR wants to merge check_false_warning and not newbranch

eli-schwartz · 2021-10-10T20:09:00Z

Oh! Almost forgot... sorry...

This should get a release notes snippet in docs/markdown/snippets/.

eli-schwartz · 2021-10-10T20:11:31Z

Nah, you can push branches like this:

git push myfork <currentbranch>:<remotebranch>

Or actually, it's a good point that you could/should also set --set-upstream-to=myfork/check_false_warning.

Volker-Weissmann · 2021-10-10T20:54:35Z

Oh! Almost forgot... sorry...

This should get a release notes snippet in docs/markdown/snippets/.

Done

eli-schwartz · 2021-10-11T01:28:09Z

meson/test cases/unit/24 compiler run_command/meson.build

Lines 4 to 10 in 437745b

    
           # This test only checks that the compiler object can be passed to 
        
           # run_command(). If the compiler has been launched, it is expected 
        
           # to output something either to stdout or to stderr. 
        
           result = run_command(cc, '--version') 
        
           if result.stdout() == '' and result.stderr() == '' 
        
             error('No output in stdout and stderr. Did the compiler run at all?') 
        
           endif

Fails on Windows with:

--- stderr ---
Microsoft (R) C/C++ Optimizing Compiler Version 19.16.27045 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line warning D9002 : ignoring unknown option '--version'
cl : Command line error D8003 : missing source filename



test cases\unit\24 compiler run_command\meson.build:7:0: ERROR: Command "C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\bin\HostX86\x86\cl.EXE --version" failed with status 2.

This case should be check: false as we don't care about the return value.

Volker-Weissmann · 2021-10-12T22:41:05Z

This case should be check: false as we don't care about the return value.

Done

eli-schwartz · 2021-10-13T23:11:37Z

if result.stdout() == '' and result.stderr() == ''
  error('No output in stdout and stderr. Did the compiler run at all?')
endif

We do care about this, please don't remove it. :)

Volker-Weissmann · 2021-10-13T23:35:53Z

if result.stdout() == '' and result.stderr() == ''
  error('No output in stdout and stderr. Did the compiler run at all?')
endif

We do care about this, please don't remove it. :)

Oops, my fault, fixed.

eli-schwartz · 2021-10-13T23:44:48Z

Alright, thanks. This seems good to me, so it should be merged as soon as the release candidate freeze ends.

eli-schwartz · 2021-10-31T02:27:25Z

Thanks!

An update to meson 0.61.1 meant that it started showing warnings due to the fact that the default for run_command's 'check' parameter is going to change. It unveiled the fact that we were even missing that parameter in some calls where we expected different outcome. To make sure the behaviour does not change specify the parameter explicitly. In places where we check for the return code the parameter should be 'false' so that meson does not fail. In all other cases the parameter should be set to 'true' to make sure possible failure also stops meson. The warning in meson was added in mesonbuild/meson#9304 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Reviewed-by: Tim Wiederhake <twiederh@redhat.com>

Volker-Weissmann requested a review from jpakkane as a code owner September 27, 2021 10:08

eli-schwartz reviewed Sep 27, 2021

View reviewed changes

eli-schwartz reviewed Oct 4, 2021

View reviewed changes

Volker-Weissmann force-pushed the check_false_warning branch 2 times, most recently from e78ef72 to 74cd845 Compare October 10, 2021 19:57

Volker-Weissmann force-pushed the check_false_warning branch from 74cd845 to bb0ee05 Compare October 10, 2021 20:53

eli-schwartz approved these changes Oct 10, 2021

View reviewed changes

Volker-Weissmann force-pushed the check_false_warning branch from bb0ee05 to 8fb78b1 Compare October 12, 2021 22:40

eli-schwartz added this to the 0.61.0 milestone Oct 13, 2021

Added warning if run_command is called without the check kwarg

d76b5e1

Volker-Weissmann force-pushed the check_false_warning branch from 8fb78b1 to d76b5e1 Compare October 13, 2021 23:35

eli-schwartz merged commit 2c079d8 into mesonbuild:master Oct 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added warning if `check` kwarg of `run_command` is not given. #9304

Added warning if `check` kwarg of `run_command` is not given. #9304

Volker-Weissmann commented Sep 27, 2021

codecov bot commented Sep 27, 2021 •

edited

eli-schwartz Sep 27, 2021

xclaesse Sep 27, 2021

eli-schwartz Sep 27, 2021

Volker-Weissmann Sep 27, 2021

eli-schwartz Oct 4, 2021

eli-schwartz Oct 10, 2021

Volker-Weissmann Oct 10, 2021

eli-schwartz commented Oct 10, 2021

Volker-Weissmann commented Oct 10, 2021

eli-schwartz commented Oct 10, 2021

Volker-Weissmann commented Oct 10, 2021

eli-schwartz commented Oct 10, 2021

eli-schwartz commented Oct 10, 2021

Volker-Weissmann commented Oct 10, 2021

eli-schwartz commented Oct 11, 2021

Volker-Weissmann commented Oct 12, 2021

eli-schwartz commented Oct 13, 2021

Volker-Weissmann commented Oct 13, 2021

eli-schwartz commented Oct 13, 2021

eli-schwartz commented Oct 31, 2021

Added warning if check kwarg of run_command is not given. #9304

Added warning if check kwarg of run_command is not given. #9304

Conversation

Volker-Weissmann commented Sep 27, 2021

codecov bot commented Sep 27, 2021 • edited

Codecov Report

eli-schwartz Sep 27, 2021

Choose a reason for hiding this comment

xclaesse Sep 27, 2021

Choose a reason for hiding this comment

eli-schwartz Sep 27, 2021

Choose a reason for hiding this comment

Volker-Weissmann Sep 27, 2021

Choose a reason for hiding this comment

eli-schwartz Oct 4, 2021

Choose a reason for hiding this comment

eli-schwartz Oct 10, 2021

Choose a reason for hiding this comment

Volker-Weissmann Oct 10, 2021

Choose a reason for hiding this comment

eli-schwartz commented Oct 10, 2021

Volker-Weissmann commented Oct 10, 2021

eli-schwartz commented Oct 10, 2021

Volker-Weissmann commented Oct 10, 2021

eli-schwartz commented Oct 10, 2021

eli-schwartz commented Oct 10, 2021

Volker-Weissmann commented Oct 10, 2021

eli-schwartz commented Oct 11, 2021

Volker-Weissmann commented Oct 12, 2021

eli-schwartz commented Oct 13, 2021

Volker-Weissmann commented Oct 13, 2021

eli-schwartz commented Oct 13, 2021

eli-schwartz commented Oct 31, 2021

Added warning if `check` kwarg of `run_command` is not given. #9304

Added warning if `check` kwarg of `run_command` is not given. #9304

codecov bot commented Sep 27, 2021 •

edited