-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Add command line arguments to print licenses, authors and donors #51028
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
8cbbbc8
to
3abdcd5
Compare
FYI, the Unicode part was fixed separately in #53773. |
main/main.cpp
Outdated
@@ -640,6 +642,130 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph | |||
print_line(get_full_version_string()); | |||
goto error; | |||
|
|||
} else if (I->get() == "--about") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could move this to another file like main/show_license.h/cpp
or similar to avoid adding even more bloat to that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can look into this. Should this be done with some static methods in a main/show_license.cpp
file, or is something more elaborate needed?
3abdcd5
to
547108c
Compare
I'm kinda surprised that this wasn't available already. Are reduz' changes the only blocker here? |
Rebased and tested again, it works as expected. Regarding splitting to a separate file, I'm not sure if there are enough lines of code to warrant this. There would be a lot of boilerplate compared to actual code. I've started working on a built-in licenses GUI which is a complement to this CLI, but it'll likely need different code as the output will be formatted differently. Edit: Pull request opened: #79599 |
547108c
to
f95498e
Compare
Yeah it looks like a fairly small and single-use part of code. I'm aware that |
This should comply for desktop, Also I would argue a message on startup like "use flag --about for licensing information" is necessary, assuming it isn't already added. Edit 1: Crossed out ones that might not support passing command line arguments Edit 2: Message can be added to existing line "Godot engine, copyright. Pass --about for license details." Not perfect since the message might be different on unsupported platforms. |
This PR only aims to provide automatic compliance on desktop platforms. It's also the only way to provide automatic compliance when distributing a headless binary 🙂 For mobile/web platforms, a GUI is needed as I commented above. It'll also be useful on desktop platforms in situations where you can't use a command prompt or don't know how to. This is why I think we need both a CLI and GUI for this – they're complementary to each other.
This could be added, but I've seen complaints about the two-line startup banner already being intrusive. Making it three lines would worsen this, especially since it would also be printed in the Output panel in the editor every time you run the project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like this for 4.3 but @akien-mga let us know if it is a blocker to move the change to a separate file.
f95498e
to
56e23a0
Compare
56e23a0
to
ba92d70
Compare
Question is there a current feature to add licenses from godot extensions? |
Not that I know of, but this should be discussed in its own proposal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall pretty good.
@@ -539,6 +540,7 @@ void Main::print_help(const char *p_binary) { | |||
print_help_title("General options"); | |||
print_help_option("-h, --help", "Display this help message.\n"); | |||
print_help_option("--version", "Display the version string.\n"); | |||
print_help_option("--about", "Display engine information (\"license\", \"thirdparty-copyrights\", \"thirdparty-licenses\", \"authors\", \"donors\").\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print_help_option("--about", "Display engine information (\"license\", \"thirdparty-copyrights\", \"thirdparty-licenses\", \"authors\", \"donors\").\n"); | |
print_help_option("--about-engine", "Display engine information (\"license\", \"thirdparty-copyrights\", \"thirdparty-licenses\", \"authors\", \"donors\").\n"); |
The engine is actually a third-party component in a game project, although I think it can be listed separately. Be more explicit to differentiate between game and engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, we have --help
/--version
instead of --help-engine
/--version-engine
. While there's some ambiguity, I think we should favor consistency here.
Game arguments should already be placed after a --
and ++
separator since Godot 4.0. (If this is found to be too tedious for some use cases, we could support an alternative +syntax
that would allow you to pass game arguments at any position, without needing --
or ++
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine with me. I just worry that sensitive game developers will mind game users misunderstanding (mainly the --about license
option) and protest against this behavior.
@@ -1097,6 +1099,138 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph | |||
exit_err = ERR_HELP; // Hack to force an early exit in `main()` with a success code. | |||
goto error; | |||
|
|||
} else if (arg == "--about") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if (arg == "--about") { | |
} else if (arg == "--about-engine") { |
See above.
if (N->get() == "license") { | ||
print_line_rich("[b][u]MIT license[/u][/b]"); | ||
// Replace reference to the `AUTHORS.md` file by the equivalent command line argument. | ||
print_line_rich(String::utf8(GODOT_LICENSE_TEXT).replace("AUTHORS.md", "[code]--about authors[/code] command line argument")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print_line_rich(String::utf8(GODOT_LICENSE_TEXT).replace("AUTHORS.md", "[code]--about authors[/code] command line argument")); | |
print_line_rich(String::utf8(GODOT_LICENSE_TEXT).replace("AUTHORS.md", "[code]--about-engine authors[/code] command line argument")); |
See above.
This is required for complying with third-party licenses when distributing headless/server binaries (including Docker containers). Since the license text is already included within the binary, this doesn't increase the binary size.
ba92d70
to
933fb39
Compare
This is required for complying with third-party licenses when distributing headless/server binaries (including Docker containers).
Since the license text is already included within the binary, this doesn't increase the binary size.
This adds the following command line arguments:
--about license
- Prints Godot's MIT license--about thirdparty-copyrights
- Prints third-party copyright notices with SPDX license names--about thirdparty-licenses
- Prints the full text of third-party licenses--about authors
- Print the engine contributors' names--about donors
- Print the engine donors' namesPreview
Note: The Unicode parsing error demonstrated in this preview has since been fixed.
October 2022 edit: I tweaked the output to use
print_line_rich()
for prettier output (not shown on the output below).--about license
--about thirdparty-copyrights
--about thirdparty-licenses
--about authors
--about donors
TODO
--about authors
and--about donors
misbehaving on names containing Unicode characters. I can never figure out how to useString::utf8()
on a Variant 🙁