8000 shell: make -bail work for more errors by mlafeldt · Pull Request #16594 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

shell: make -bail work for more errors #16594

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

Jump to bottom
Merged
merged 1 commit into from
Mar 20, 2025
Merged

shell: make -bail work for more errors #16594

merged 1 commit into from
Mar 20, 2025

Conversation

mlafeldt
Copy link
Contributor
@mlafeldt mlafeldt commented Mar 11, 2025 •
edited
Loading

I noticed that -bail doesn't have any effect in several cases, most notably when passing init commands, so I tried to fix it.

  • -cmd and -init with -bail will exit on error without starting the shell
  • -cmd and -init without -bail will continue to start the shell as before
  • -f / -s / -c will now always exit with 1 on error
❯ build/release/duckdb -c boom; echo $?
Parser Error:
syntax error at or near "boom"

LINE 1: boom
        ^
1

❯ build/release/duckdb -s boom; echo $?
Parser Error:
syntax error at or near "boom"

LINE 1: boom
        ^
1

❯ build/release/duckdb -cmd boom; echo $?
-- Loading resources from /Users/mathias/.duckdbrc
Parser Error:
syntax error at or near "boom"

LINE 1: boom
        ^
v1.3.0-dev1569 9d11d953e6
Enter ".help" for usage hints.
🟡◗

0

❯ build/release/duckdb -cmd boom -bail; echo $?
-- Loading resources from /Users/mathias/.duckdbrc
Parser Error:
syntax error at or near "boom"

LINE 1: boom
        ^
1

❯ build/release/duckdb -init init.sql; echo $?
-- Loading resources from init.sql
Parser Error:
syntax error at or near "boom"

LINE 1: boom;
        ^
v1.3.0-dev1569 9d11d953e6
Enter ".help" for usage hints.
D

0

❯ build/release/duckdb -init init.sql -bail; echo $?
-- Loading resources from init.sql
Parser Error:
syntax error at or near "boom"

LINE 1: boom;
        ^
1

❯ build/release/duckdb -f init.sql; echo $?
Parser Error:
syntax error at or near "boom"

LINE 1: boom;
        ^
1

@mlafeldt
Copy link
Contributor Author
mlafeldt commented Mar 11, 2025 •
edited
Loading
  • -bail with -s / -c will exit with 1 on error

Actually, I think this should also always return 1, even without -bail.

Or should I modify -f to only return 1 with -bail to not change the default behavior?

mlafeldt
mlafeldt commented Mar 11, 2025
View reviewed changes
tools/shell/shell.cpp
@@ -4656,11 +4659,11 @@ void ShellState::ProcessDuckDBRC(const char *file) {
// could not find home directory - return
raw_printf(stderr, "-- warning: cannot find home directory;"
" cannot read ~/.duckdbrc\n");
return;
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Treated as a warning and not an error (according to the log message)

@mlafeldt
Copy link
Contributor Author

Actually, I think this should also always return 1, even without -bail.

Or should I modify -f to only return 1 with -bail to not change the default behavior?

@Mytherin Let me know what you think. 🙏

@Mytherin
Copy link
Collaborator

I think -f, -c, etc returning exit code 1 if an error is encountered makes a lot of sense. I would also say perhaps -f should always have -bail on by default and not execute the remainder of the file.

@Okabintaro Okabintaro mentioned this pull request Mar 16, 2025
Open
2 tasks
@mlafeldt mlafeldt force-pushed the shell-bail-more branch 3 times, most recently from 877e71a to 9d11d95 Compare March 19, 2025 15:08
@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 19, 2025 15:09
@mlafeldt
Copy link
Contributor Author
mlafeldt commented Mar 19, 2025 •
edited
Loading

@Mytherin I took another pass. The new version makes more sense because only the init options -cmd and -init will now fail with -bail. Non-init options will fail in any case. Also see my updated PR description incl. examples.

@mlafeldt mlafeldt marked this pull request as ready for review March 19, 2025 15:17
@mlafeldt
shell: make -bail work for more errors
ad11731 
- -cmd and -init with -bail will exit on error without starting the shell
- -cmd and -init without -bail will continue to start the shell as before
- -f / -s / -c will now always exit with 1 on error
Mytherin
Mytherin approved these changes Mar 19, 2025
View reviewed changes
Copy link
Collaborator
@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! LGTM

@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 19, 2025 15:33
@Mytherin Mytherin marked this pull request as ready for review March 20, 2025 06:24
@Mytherin Mytherin merged commit 0b3d9e7 into duckdb:main Mar 20, 2025
31 checks passed
@mlafeldt mlafeldt deleted the shell-bail-more branch March 20, 2025 09:54
@Tishj
Copy link
Contributor
Tishj commented Mar 20, 2025 
/Users/thijs/DuckDBLabs/duckdb/tools/shell/shell.cpp:4644:13: warning: variable 'rc' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
 4644 |         } else if (!is_duckdb_rc) {
      |                    ^~~~~~~~~~~~~
/Users/thijs/DuckDBLabs/duckdb/tools/shell/shell.cpp:4650:9: note: uninitialized use occurs here
 4650 |         return rc == 0;
      |                ^~
/Users/thijs/DuckDBLabs/duckdb/tools/shell/shell.cpp:4644:9: note: remove the 'if' if its condition is always true
 4644 |         } else if (!is_duckdb_rc) {
      |                ^~~~~~~~~~~~~~~~~~
/Users/thijs/DuckDBLabs/duckdb/tools/shell/shell.cpp:4635:8: note: initialize the variable 'rc' to silence this warning
 4635 |         int rc;
      |               ^
      |                = 0

There's an uninitialized value issue in the use of rc, that was introduced here

@mlafeldt
Copy link
Contributor Author

@Tishj Good catch! I will create a follow-up PR.

Two questions though:

  1. Where did you see this? Even make TREAT_WARNINGS_AS_ERRORS=1 won't reveal it for me.
  2. Why didn't CI catch this?

@mlafeldt
Copy link
Contributor Author

Ok, found it with make CXXFLAGS=-Wall. Interestingly, there are more "sometimes-uninitialized" errors in sqlite3_api_wrapper when using CFLAGS=-Wall as well.

@mlafeldt mlafeldt mentioned this pull request Mar 20, 2025
Merged
Mytherin added a commit that referenced this pull request Mar 21, 2025
@Mytherin
shell: fix sometimes-uninitialized error (#16761)
baf27fa 
This is a follow-up to #16594, which introduced the problem.

Thanks to @Tishj for bringing it up
[here](#16594 (comment)).

(I only encountered the error with `make CXXFLAGS=-Wall`, which makes me
wonder a) why that's not the default and b) why CI didn't catch this
bug?)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 15, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@df40825
28cf529 
JSON bugfixes (duckdb/duckdb#16729)
Call Notify External Repositories from Invoke CI (duckdb/duckdb#16747)
shell: make -bail work for more errors (duckdb/duckdb#16594)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 15, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@df40825
c758c67 
JSON bugfixes (duckdb/duckdb#16729)
Call Notify External Repositories from Invoke CI (duckdb/duckdb#16747)
shell: make -bail work for more errors (duckdb/duckdb#16594)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 16, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@df40825
d0d73ab 
JSON bugfixes (duckdb/duckdb#16729)
Call Notify External Repositories from Invoke CI (duckdb/duckdb#16747)
shell: make -bail work for more errors (duckdb/duckdb#16594)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 16, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@df40825
923be61 
JSON bugfixes (duckdb/duckdb#16729)
Call Notify External Repositories from Invoke CI (duckdb/duckdb#16747)
shell: make -bail work for more errors (duckdb/duckdb#16594)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 17, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@df40825
07357f7 
JSON bugfixes (duckdb/duckdb#16729)
Call Notify External Repositories from Invoke CI (duckdb/duckdb#16747)
shell: make -bail work for more errors (duckdb/duckdb#16594)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin approved these changes

Assignees
No one assigned
Labels
Ready To Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mlafeldt @Mytherin @Tishj
0