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

config: upgrade dbg to err on missing option #778

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

mbolivar
Copy link
Contributor

A user has pointed out that west's behavior in the following case is confusing:

  west config foo.bar=baz

The desired behavior is "set foo.bar to baz". Instead, west exits 1 without printing anything on the console.

Let's make it clearer that the user is inadventently asking for the value of an option called "foo.bar=baz", since right now there is no rule forbidding "=" in option names, so the above confusing case is actually asking for section "foo", key "bar=baz".

With this patch, west prints:

  ERROR: foo.bar=baz is unset

to stderr along with exiting 1, making it clearer that something went wrong.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Approved but only because this is on stderr, otherwise this would break:

value_maybe=$( west config key ) || true
test -n "$value_maybe" || ...

This PR will not break but NOISE the output of such code though. To silence their existing code, people will now have to redirect stderr:

test -n "$( 2>/dev/null west config key )" || ...

Maybe warn about that in the commit message?

There may also be some thorough CI engines that fail if stderr is not empty. These will break but I think that's acceptable. Also record that in the commit message?

A user has pointed out that west's behavior in the following case is
confusing:

  west config foo.bar=baz

The desired behavior is "set foo.bar to baz". Instead, west exits 1
without printing anything on the console.

Let's make it clearer that the user is inadventently asking for the
value of an option called "foo.bar=baz", since right now there is no
rule forbidding "=" in option names, so the above confusing case is
actually asking for section "foo", key "bar=baz".

With this patch, west prints:

  ERROR: foo.bar=baz is unset

to stderr along with exiting 1, making it clearer that something went
wrong.

Note that since this doesn't print to stdout, the new output won't be
confused with the value of the option by well-behaved programs. In
general, 'west config' documents no guarantees about when it prints to
stderr, and new error messages showing up on stderr is in line with
general de facto standard expectations for command line programs.
However, users may need to redirect stderr to /dev/null (or do
similarly on Windows) now if they aren't interested in this output.

Reported-by: David Brown <[email protected]>
Signed-off-by: Martí Bolívar <[email protected]>
@mbolivar mbolivar force-pushed the config-print-to-stderr branch from 9394008 to 8afec11 Compare January 13, 2025 23:18
@mbolivar
Copy link
Contributor Author

but only because this is on stderr

right, I considered this as well and agree it's important.

I added the following to the commit message:

Note that since this doesn't print to stdout, the new output won't be
confused with the value of the option by well-behaved programs. In
general, 'west config' documents no guarantees about when it prints to
stderr, and new error messages showing up on stderr is in line with
general de facto standard expectations for command line programs.
However, users may need to redirect stderr to /dev/null (or do
similarly on Windows) now if they aren't interested in this output.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Approved but maybe wait before merge until the jury is out for related #779? Just in case #779 is "a bigger problem" that could make this #778 irrelevant.

@mbolivar
Copy link
Contributor Author

I'm not personally going to do anything about #779 so that might be a long wait. I think this current PR improves the UI and can't hurt anything so I don't see the point of waiting.

@marc-hb marc-hb merged commit 950fc32 into zephyrproject-rtos:main Jan 14, 2025
18 checks passed
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.

4 participants