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

Command line argument '--version' returns exit code 0 instead of 255 #62550

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

Bromeon
Copy link
Contributor

@Bromeon Bromeon commented Jun 30, 2022

Allows to detect whether printing the version was successful and makes integration with shell scripts straightforward.

Minor thing: --version is usually read by tools (CI, shell scripts, bindings) and at the moment, an extra newline is being printed, which requires trimming. On the other hand, it makes things a bit more readable for humans. Let me know if the current behavior is good as it is.

@Bromeon Bromeon requested a review from a team as a code owner June 30, 2022 10:06
main/main.cpp Outdated Show resolved Hide resolved
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Make sense, should also be added to --help. Other goto error; cases seem to all be proper invalid parameter errors so they're fine.

@akien-mga
Copy link
Member

Minor thing: --version is usually read by tools (CI, shell scripts, bindings) and at the moment, an extra newline is being printed, which requires trimming. On the other hand, it makes things a bit more readable for humans. Let me know if the current behavior is good as it is.

I think the current behavior is fine, most software seems to have human readable --version strings AFAICT:

$ gcc --version
gcc (Mageia 12.1.1-0.20220625.1.mga9) 12.1.1 20220625
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ clang --version
clang version 14.0.5 (Mageia 14.0.5-1.mga9)
Target: x86_64-mageia-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ kate --version
kate 22.04.0
$ code --version
1.68.0
4af164ea3a06f701fe3e89a2bcbb421d2026b68f
x64
$ firefox-nightly --version
Mozilla Firefox 104.0a1
$ chromium-browser --version
Chromium 103.0.5060.53 Mageia.Org 9
$ godot --version
3.4.4.stable.mageia.419e713a2

@akien-mga akien-mga added this to the 4.0 milestone Jun 30, 2022
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 30, 2022
…stead of 255

Allows to detect whether those commands executed successfully, which makes integration with shell scripts/CI/bindings straightforward.
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good!

@akien-mga akien-mga merged commit e7197cb into godotengine:master Jun 30, 2022
@akien-mga
Copy link
Member

Thanks!

@Bromeon
Copy link
Contributor Author

Bromeon commented Jun 30, 2022

Thanks for the quick feedback! 🙂

@Bromeon Bromeon deleted the bugfix/cli-version-ok branch June 30, 2022 12:42
Bromeon added a commit to Bromeon/godot4-nightly that referenced this pull request Jun 30, 2022
@akien-mga
Copy link
Member

akien-mga commented Jun 30, 2022

Cherry-picked for 3.5.

Edit: Reverted from 3.x for now due to the bug below. Still in master.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 30, 2022
@akien-mga
Copy link
Member

This introduced a bug:

$ godot-git --version
4.0.alpha.custom_build.11a978b1b
ERROR: Condition "!_start_success" is true. Returning: false
   at: start (main/main.cpp:2096)
ERROR: Condition "!_start_success" is true.
   at: cleanup (main/main.cpp:2943)
$ echo $?
1

And doesn't actually achieve what was intended, since it still returns a failure code but further down.

I didn't pay close attention but this PR actually makes setup() return OK, which means that it will then proceed to start() instead of doing an early exit.

akien-mga added a commit that referenced this pull request Jul 1, 2022
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 2, 2022
@akien-mga
Copy link
Member

akien-mga commented Jul 2, 2022

Cherry-picked for 3.5.

Edit: Seems like I messed up locally and forgot to push this back then. Fixed on 2022-07-08 with da78e92.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 2, 2022
Riordan-DC pushed a commit to Riordan-DC/godot that referenced this pull request Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants