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: Improve colored output #93479

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jun 22, 2024

Expanded the ANSI color code support already implemented in SCons by consolidating the logic in methods.py. The overall changes were:

  • Windows-specific ANSI fix now integrated into methods.py, so anything that calls that logic will have their terminal automatically setup to handle color output.
  • Added dim support to color/font output. Not used currently.
  • High-intensity versions of colors added; adds LIGHT_ prefix to existing colors.
  • Converted Ansi.GRAY from 256 color to a conditional that checks if continuous integration is active. This works around a previous issue with GitHub Actions causing Ansi.BLACK to output as black instead of gray; now CI specifically will use Ansi.LIGHT_BLACK instead.
  • Removed other 256-style colors.
  • Implementing print_info from Add colors to SCons error/warning messages #88074. Not widely used atm, mostly replacing print("Note: <...>") instances.
  • doc_status.py, make_rst.py, install_d3d12_sdk_windows.py and the GDExtension methods.py files got their color logic overhauled to utilize the root methods.py.

@Repiteo Repiteo added this to the 4.3 milestone Jun 22, 2024
@Repiteo Repiteo requested review from a team as code owners June 22, 2024 17:48
@Repiteo Repiteo force-pushed the scons/better-colored-output branch from 0bc1a15 to adb6aa2 Compare June 22, 2024 17:57
@Repiteo Repiteo removed request for a team June 22, 2024 17:57
@Repiteo Repiteo modified the milestones: 4.3, 4.x Jul 7, 2024
@Repiteo Repiteo force-pushed the scons/better-colored-output branch 2 times, most recently from 9d02815 to b519f2b Compare October 18, 2024 19:38
@adamscott
Copy link
Member

Is there a way to add color in the output of dev_mode=yes?

Capture d’écran, le 2024-10-22 à 13 09 24

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 22, 2024

Hmm, potentially. I know that's just verbose=yes, and SCons will print out the command called when no other command string is provided, so it could be as simple as getting that string and repurposing it somehow. I'll look into it later today

Copy link
Contributor

@dustdfg dustdfg left a comment

Choose a reason for hiding this comment

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

LGTM. Rebased on master and it looks like it still works

@dustdfg
Copy link
Contributor

dustdfg commented Oct 23, 2024

If it is easy to recolor verbose="yes" so maybe recoloring following is also as easy?
screen-1729692754

@Riteo
Copy link
Contributor

Riteo commented Oct 23, 2024

@dustdfg I think that's my fault, as I added the string manually instead of doing like the other generators. If all other generated files just say "Generating" I argue that we could get rid of the custom message.

@Repiteo Repiteo force-pushed the scons/better-colored-output branch 2 times, most recently from 0cfe8e3 to 7f2c701 Compare November 18, 2024 15:14
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, it works as expected on Linux (also when running doc_status.py).

Code looks good to me.

@Repiteo Repiteo force-pushed the scons/better-colored-output branch from 7f2c701 to d8761f2 Compare December 10, 2024 17:49
@Repiteo
Copy link
Contributor Author

Repiteo commented Dec 10, 2024

Opted to simply handle all the logic in methods.py instead of making a dedicated module. That file could be split into individual modules eventually, but for now this sticks with the existing style

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Dec 11, 2024
@Repiteo Repiteo requested a review from akien-mga December 12, 2024 21:49
@@ -6,16 +6,9 @@
import sys
import urllib.request

Copy link
Member

Choose a reason for hiding this comment

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

This script was executable on purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dagnabbit, Windows source control strikes again

def __str__(self) -> str:
global _colorize
return str(self.value) if _colorize else ""
from methods import Ansi
Copy link
Member

Choose a reason for hiding this comment

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

Importing methods in methods.py... does that really work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After prepending the root path, yeah! Granted, the whole implementation of the local GDExtension-specific methods.py is a bit suspect, but that's outside the scope of this PR

import re
import sys
import xml.etree.ElementTree as ET
from typing import Dict, List, Set

sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), "../../"))
Copy link
Member

@akien-mga akien-mga Dec 12, 2024

Choose a reason for hiding this comment

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

This is so messy :/

I know this was already used in make_rst.py but methods.py is a collection of methods for use by SCons to build the engine, it's not meant to be used by generic standalone scripts.

Those scripts shouldn't break if we move them around ideally.

@akien-mga
Copy link
Member

I like the reduction in code duplication, but I don't love the added dependency on methods.py for Python scripts which are unrelated to the buildsystem.

I'd question whether just getting some pretty colors justifies this complexity (both before this PR and after).

@Repiteo
Copy link
Contributor Author

Repiteo commented Dec 12, 2024

This PR originally kept the color logic in a separate file, misc/utility/colorize.py, so I could go back to that implementation if desired

However, the nature of the repo unfortunately makes some degree of dependencies inevitable. It's normally automated with SCons passing scope up automatically, but the isolated scripts have no choice in the matter. Unless we have some "master" script/hook in the repo root that calls to other scripts, I see no way of working around that fundamental limitation

@akien-mga akien-mga merged commit 182b474 into godotengine:master Dec 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@github-MaxCE
Copy link

github-MaxCE commented Dec 18, 2024

PS C:\Users\ \Godot> scons use_mingw=yes use_llvm=yes platform=windows module_mono_enabled=yes d3d12=yes
scons: Reading SConscript files ...
Auto-detected 8 CPU cores available for build parallelism. Using 7 cores by default. You can override it with the `-j` or `num_jobs` arguments.
Using MinGW, arch x86_64
AttributeError: module 'methods' has no attribute '_colorize':
  File "C:\Users\ \Godot\SConstruct", line 616:
    detect.configure(env)
  File "C:\Users\ \Godot\platform/windows\detect.py", line 929:
    configure_mingw(env)
  File "C:\Users\ \Godot\platform/windows\detect.py", line 810:
    if env["use_llvm"] and os.name == "nt" and methods._colorize:

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.

7 participants