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

SCons: Colorize warnings/errors during generation #91220

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Apr 26, 2024

This change adds the ability to substitute print with methods.print_warning or methods.print_error, prepending the messages with WARNING: /ERROR: and colorizing them as yellow/red respectively. The messages will also be routed to stderr instead of stdout, allowing users to silence stdout without missing on any crucial information if a build fails or otherwise stops. non-atty environments won't benefit from colorization, but will still enjoy prefix & stderr benefits.
24-04-26 13-22-39 Code

EDIT: Supersedes #88074

Bugsquad edit:

@Repiteo Repiteo requested review from a team as code owners April 26, 2024 18:30
@AThousandShips AThousandShips added this to the 4.x milestone Apr 26, 2024
@Repiteo Repiteo force-pushed the scons/colorize-warn-error branch 3 times, most recently from a60846a to 0d38176 Compare April 26, 2024 19:18
Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Holy moly you rock!

Took a look and the diff looks fine!

@Calinou
Copy link
Member

Calinou commented Apr 26, 2024

Does this supersede #88074? It includes some not changes not made in this PR.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 26, 2024
@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 26, 2024

I don't think it supercedes that PR entirety; though, it probably would need a different title if left as-is

@Riteo
Copy link
Contributor

Riteo commented Apr 26, 2024

Oops, I forgot about the older PR, I'm sorry @Calinou.

This is a bit of an unfortunate situation. How should we handle it?

They're both great PRs, with the same main change but different details.

@Repiteo Repiteo requested a review from a team as a code owner April 27, 2024 15:08
@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 27, 2024

Did some cleanup work; now it supersedes #88074! The only bits I didn't bring over were print_info because it felt a little redundant & the "Enable ANSI escape code support on Windows 10 and later" section as that issue appears to have been fixed in Python 3.7 which is good enough for me.

@Calinou
Copy link
Member

Calinou commented Apr 28, 2024

Sounds good to me. We should make sure to update the Compiling on Windows documentation to require Python 3.7 though, if we start requiring it de facto for proper display of errors/warnings on Windows.

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 28, 2024

Ehhh, if we gotta go through that trouble I'll just add that bit of code after all; I'll throw in a note that it should be removed when the minimum version is >=3.7

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 28, 2024

Hmm, I might've misread the original post; I don't think it's necessarily limited to 3.7 after all. And yet, I've never had any issues with displaying colored text & I use Windows 11, so I honestly have no clue what circumstances cause that bug to occur. Either way, no real harm done in including it.

@tiloc
Copy link

tiloc commented Apr 28, 2024

Sounds good to me. We should make sure to update the Compiling on Windows documentation to require Python 3.7 though, if we start requiring it de facto for proper display of errors/warnings on Windows.

Python 3.7 reached end-of-life 10 months ago. 3.8 and later still get security fixes.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (Windows 11 23H2, MSVC 2022, Python 3.12.3), it works as expected. Code looks good to me.

@Calinou
Copy link
Member

Calinou commented Apr 28, 2024

Compilation output on CI looks strange with lots of [Initial build], which I don't see on other PRs' CI logs: https://github.com/godotengine/godot/actions/runs/8868984410/job/24349194236?pr=91220

Perhaps we should use progress=no on CI either way, as I don't think logging progress on CI is that useful.

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 28, 2024

I double-checked the PR this supersedes, and that error was happening there too. Seems like enabling progress reporting for CI is what caused it, as reverting that fixed the issue.

@akien-mga akien-mga merged commit dee1231 into godotengine:master Apr 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Anutrix
Copy link
Contributor

Anutrix commented Apr 29, 2024

This MR broke use_mingw flag on windows:

PS D:\Repo\godot> scons use_mingw=yes
scons: Reading SConscript files ...
Automatically detected platform: windows
Auto-detected 24 CPU cores available for build parallelism. Using 23 cores by default. You can override it with the -j argument.
Using MinGW, arch x86_64
Building for platform "windows", architecture "x86_64", target "editor".
AttributeError: 'str' object has no attribute 'insert':
  File "D:\Repo\godot\SConstruct", line 971:
    methods.no_verbose(env)
  File "D:\Repo\godot\methods.py", line 671:
    env.Append(SHLINKCOMSTR=link_shared_library_message)
  File "C:\Users\<username>\AppData\Roaming\Python\Python312\site-packages\SCons\Environment.py", line 1458:
    val.insert(0, orig)

Note: Compilation proceeds when verbose flag is given.

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.

Make warnings/notes in build scripts colored
7 participants