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

Add --log-file command line argument to write output log to a file #87373

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jan 19, 2024

This allows using a custom output path for the log file, with log rotation disabled. This works even if file logging is disabled in the project settings, or for the editor/project manager.

--log-file's value can be an absolute path or relative to the project directory (similar to existing arguments like --write-movie).

Note that flushing stdout on print isn't forced in release builds (it still depends on the project setting's value). We may need a way to enforce this, but we already have a lot of CLI arguments exposed.

@Calinou Calinou requested review from a team as code owners January 19, 2024 13:40
@Calinou Calinou added this to the 4.x milestone Jan 19, 2024
main/main.cpp Outdated
Comment on lines 1706 to 1709
if (FileAccess::get_create_func(FileAccess::ACCESS_USERDATA) &&
(!log_file.is_empty() || (!project_manager && !editor && GLOBAL_GET("debug/file_logging/enable_file_logging")))) {
Copy link
Member

Choose a reason for hiding this comment

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

--log-file typically wouldn't be used to write to user://, but to a pass relative to the working directory, so I'm not sure the check for ACCESS_USERDATA makes sense / is enough.

Copy link
Member Author

@Calinou Calinou Jan 19, 2024

Choose a reason for hiding this comment

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

I've tested an absolute path located outside user:// and it worked fine, but I guess we can check for ACCESS_FILESYSTEM when the log path isn't empty.
Edit: Done.

Also, the logger currently fails with a crash if the file can't be written by the current user (e.g. when trying to write /log.txt as non-root). A check needs to be added so that it prints an error and exits gracefully instead (or even keep going).

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.

Sounds like a good option to have, especially for when users struggling getting actual console output to debug issues on Windows. Would be worth checking to confirm that it works well for both the .console.exe wrapper and the normal .exe.

doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
@Calinou Calinou force-pushed the add-log-file-cli-argument branch 3 times, most recently from b41b473 to 70305d2 Compare January 19, 2024 19:29
This works even if file logging is disabled in the project settings,
or for the editor/project manager.

`--log-file`'s value can be an absolute path or relative to the project
directory (similar to existing arguments like `--write-movie`).
@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Jan 22, 2024
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Seems to work fine on Windows with either console.exe or regular exe.

@YuriSizov YuriSizov merged commit e89c9b5 into godotengine:master Jan 22, 2024
16 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@theromis
Copy link
Contributor

@Calinou thank you for quick development, is it good idea to create different proposal for optional running godot in background, reopening log file on signal and having ability to have a PID file to run it as a service. Or better just leave it as is?

@Calinou
Copy link
Member Author

Calinou commented Jan 24, 2024

@Calinou thank you for quick development, is it good idea to create different proposal for optional running godot in background, reopening log file on signal and having ability to have a PID file to run it as a service. Or better just leave it as is?

Yes, each of these items should be tracked in its own proposal. Please don't cram several unrelated items into a single proposal, as this makes it difficult to track them individually.

@Calinou Calinou deleted the add-log-file-cli-argument branch January 24, 2024 19:19
@theromis
Copy link
Contributor

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.

Ability to write godot console output to log file
5 participants