Output improvements #391
Output improvements #391
Conversation
|
@mbolivar please rebase |
This is like the internal routine _use_colors(), but it never warns. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar
force-pushed
the
output-improvements
branch
from
e08bf70
to
b991f83
Compare
done |
| except subprocess.CalledProcessError: | ||
| cp = project.git(['diff', f'--src-prefix={project.path}/', | ||
| f'--dst-prefix={project.path}/', | ||
| '--exit-code'] + color, |
There was a problem hiding this comment.
BTW, I tried to do the same thing for 'git status', but ran into a potential problem. The --porcelain=2 option is not available on Ubuntu 16.04;s git, and I believe that was required to get machine-readable output from git status which can be colorized. I think we need to either:
- cache the git version in the WestApp and pass it to the WestCommand, which can check for features with something like
if self.git_version >= SOME_MIN_VERSION: # use git status --porcelain=2, to try to keep supporting older versions of git, OR - start tracking a minimum version of git that west's core supports and testing against it and only use features that are always available in the minimum version
This gets tricky and is a general problem, IMO.
| print(f"WARNING: invalid color.ui value: {e}.", | ||
| file=sys.stderr) | ||
| print(' To fix: "west config color.ui <true|false>"', |
There was a problem hiding this comment.
didn't knew west had this feature, and the changes themselves looks good to me.
But as we are using: config.ui, and git is also having a config.ui setting, then maybe we should align allowed values.
That is, auto, always, false
From https://www.git-scm.com/book/en/v2/Customizing-Git-Git-Configuration
color.ui
Git automatically colors most of its output, but there’s a master switch if you don’t like this behavior. To turn off all Git’s colored terminal output, do this:
$ git config --global color.ui falseThe default setting is auto, which colors output when it’s going straight to a terminal, but omits the color-control codes when the output is redirected to a pipe or a file.
You can also set it to always to ignore the difference between terminals and pipes. You’ll rarely want this; in most scenarios, if you want color codes in your redirected output, you can instead pass a --color flag to the Git command to force it to use color codes. The default setting is almost always what you’ll want.
There was a problem hiding this comment.
While I don't disagree with the suggestion, that feels like scope creep.
There was a problem hiding this comment.
While I don't disagree with the suggestion, that feels like scope creep.
That was not my intention.
I was more thinking that it would be just passing on the setting, as well as not reinventing the wheel.
But it is in principle not something which needs discussion in this PR.
Out of curiosity, why is this needed as a west config also, when user can already do:
git config --global color.ui auto
and we could just honor that setting ?
There was a problem hiding this comment.
The west option has the same name as the git one for familiarity, but it controls west's color output, e.g. for log.wrn().
There was a problem hiding this comment.
... and therefore subprocesses which are capable of colored output (like git) should be encouraged to print in color when the west user has requested west colors.
| if cp.returncode == 1 or log.VERBOSE > log.VERBOSE_NONE: | ||
| log.banner(f'diff for {project.name_and_path}:') | ||
| log.inf(cp.stdout.decode('utf-8')) |
There was a problem hiding this comment.
are we sure users want this ?
If I run west diff today, I might see:
=== diff for zephyr (zephyr):
=== diff for nffs (modules/fs/nffs):
=== diff for segger (modules/debug/segger):
=== diff for mbedtls (modules/crypto/mbedtls):
....
but if we remove those lines per default, users might be unsure if the command was actually executed in all repos.
At minimum inform something like:
=== no diff for zephyr (zephyr):
=== no diff for nffs (modules/fs/nffs):
=== no diff for segger (modules/debug/segger):
=== no diff for mbedtls (modules/crypto/mbedtls):
....
But maybe even better could be to collect repos with no diff, and then print a final message, such as:
=== No diff in projects: zephyr, nffs, segger, mbedtls, ....
I don't think the path is important for projects without changes.
There was a problem hiding this comment.
Good idea. Updated. It now prints something like this:
$ west diff
=== diff for manifest (zephyr):
diff --git zephyr/scripts/dts/gen_defines.py zephyr/scripts/dts/gen_defines.py
index 4270eb7b4f..2d3d33a309 100755
--- zephyr/scripts/dts/gen_defines.py
+++ zephyr/scripts/dts/gen_defines.py
@@ -431,7 +431,7 @@ def phandle_macros(prop, macro):
out_comment(f"No label macro emitted for {prop.node.name} "
f"property {prop.name}:\n"
f"{prop.val.path} has no label\n",
- blank_before=False)
+ blank_before=False) # hello
else:
for i, node in enumerate(prop.val):
ret[f"{macro}_IDX_{i}_VAL_LABEL"] = quote_str(node.label)
Empty diff in 31 projects.
The output is kind of cluttered when many projects are available. Let's silence output for projects with no diff when not in verbose mode. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar
force-pushed
the
output-improvements
branch
from
b991f83
to
c8aecab
Compare
| if failed: | ||
| self._handle_failed(args, failed) | ||
| elif log.VERBOSE <= log.VERBOSE_NONE: | ||
| log.inf(f"Empty diff in {no_diff} projects.") |
depends on #390