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

Fix --generate-mono-glue printing an unknown CLI argument warning #99294

Closed

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Nov 15, 2024


  • Abort with an error when using --generate-mono-glue on builds without C# support (similar to --test on builds with tests disabled).
  • Fix unknown CLI argument warning messages not being visible on Windows.
  • Fix --test error message not being visible on Windows on builds with tests disabled.

The issue about prints not being visible is due to how ERR_PRINT() and WARN_PRINT() work. We don't need their stack trace to be visible here anyway. We can reinstate the coloring later in a future PR (which will apply to all CLI arguments by adding a new method for it).

@Calinou Calinou requested a review from a team as a code owner November 15, 2024 18:36
@Calinou Calinou added this to the 4.4 milestone Nov 15, 2024
@Calinou Calinou force-pushed the mono-handle-cli-arguments branch 2 times, most recently from 238917d to 21d6cc2 Compare November 15, 2024 18:44
- Fix unknown CLI argument warning messages not being visible on Windows.
- Fix `--test` error message not being visible on Windows on builds
  with tests disabled.
@Calinou Calinou force-pushed the mono-handle-cli-arguments branch from 21d6cc2 to 83e62aa Compare November 15, 2024 19:00
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks! I can confirm this fixes the --generate-mono-glue issue.

Comment on lines +1804 to +1813
// Handle arguments specific to C#-enabled builds.
const bool using_mono_args = arg == "--generate-mono-glue";
#ifndef MODULE_MONO_ENABLED
if (using_mono_args) {
OS::get_singleton()->print(
"`--generate-mono-glue` was specified on the command line, but this Godot binary was compiled without .NET support. Aborting.\n"
"To be able to generate C# glue code, use the `module_mono_enabled=yes` SCons option when compiling Godot.\n");
goto error;
}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

We're starting to accumulate a lot of hacks in main.cpp for the Mono module... I'm not fond of increasing the tech debt further here.

There's also --class-db-json in the Mono module that's not being handled here.

Copy link
Member

Choose a reason for hiding this comment

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

And basically any module that would want to "handle" a command line argument unique to itself by parsing OS::get_cmdline_args would have the same issue.

There are more:

modules/gdscript/tests/gdscript_test_runner_suite.h
44:             bool print_filenames = OS::get_singleton()->get_cmdline_args().find("--print-filenames") != nullptr;
45:             bool use_binary_tokens = OS::get_singleton()->get_cmdline_args().find("--use-binary-tokens") != nullptr;

I think it's better to go back to the drawing board for how to detect invalid arguments. We can't add special cases in main.cpp for all arguments parsed by first-party and third-party modules.

Copy link
Member

Choose a reason for hiding this comment

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

See also #99224.

Copy link
Member

Choose a reason for hiding this comment

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

There's also --class-db-json in the Mono module that's not being handled here.

I don't think anybody's using this, it's an undocumented option and could probably be replaced with --dump-extension-api.

@akien-mga
Copy link
Member

Superseded by #99300.

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.

Fail to build mono glue
3 participants