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 msvc stdout issues with cl/link #4513

Closed
wants to merge 1 commit into from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Apr 12, 2024

There's been a long-standing issue with msvc when using the cl/link tools: their stdout choices are abysmal. They will always have a line of bloat with absolutely no arguments to circumvent it & will frequently put error messages on stdout instead of stderr. This PR addresses both issues at once with a modified implementation of a SPAWN fix used in the Godot repo1. The main change is this is directly built into the spawn function itself, branching off if cl or link is the leading argument.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

Footnotes

  1. https://github.com/godotengine/godot/pull/90551

@jcbrill
Copy link
Contributor

jcbrill commented May 12, 2024

Link to Godot issue for details concerning implementation issues and notes:
godotengine/godot#91883

@bdbaddog
Copy link
Contributor

I don't think we can merge your implementation.

  1. We'd have to do a deprecation cycle as this changes default behavoir
  2. You're adding tool specific logic in the core of the tool. We could probably merge an implementation where the output filtering was provided by the tool on a tool by tool basis.

If you're interested in taking a shot at #2, please come discuss on discord server and/or open a new PR with your initial implementation.

@bdbaddog bdbaddog closed this May 12, 2024
@Repiteo Repiteo deleted the msvc-stdout-fix branch May 12, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants