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: Fix MinGW tools on Windows native shells #91847

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented May 11, 2024

A reimplementation of #91734 with the intent of maintaining readability & functionality. The main thing this serves to fix is an odd quirk of running MinGW Tools on Windows in a native shell: they will very frequently fail because of the Windows-style path separators being parsed as escape characters. While I tried to circumvent this through other more roundabout means, the solution that turned out to be the most reliable by far was detecting if the os separator is Window-style; if it is, prepend the MinGW bin folder to the path. Frankly, I'm not sure why this works, but… It works!

This also solves a significant flaw found in the now-reverted #85319, where the mingw_prefix was prepended to everything, leading to certain tools to fail outright (RES most notably). This circumvents that problem by making the process by which these values are checked much, much more involved. It utilizes the SCons function Detect to see if a given tool is found, and it does this check upwards of 4 times in the following order:

  • Check in mingw_prefix bin folder with arch prefix
  • Check in mingw_prefix bin folder with no prefix
  • Check on path with arch prefix
  • Check on path with no prefix

If the tool is found at any point, it's returned as a path. If the tool isn't found, it's returned as an empty string, evaluating to False in conditionals. Use of conditionals in this manner was significantly reduced, as that is largely handled on SCons' end if a given env variable either evaluates to False or doesn't exist. As a result of this change, build_res_file is no longer needed. The arch alias option is instead assigned to RCFLAGS and the detected tool for windres is assigned to RC.

@Repiteo Repiteo requested a review from a team as a code owner May 11, 2024 20:18
@Repiteo
Copy link
Contributor Author

Repiteo commented May 11, 2024

cc @TokageItLab. I wanna be 100% sure this can't cause #91710 again.

@akien-mga
Copy link
Member

Would be good to assess whether that may fix #40286 fully - or whether that specific older issue was still reproducible in the first place.

@Repiteo
Copy link
Contributor Author

Repiteo commented May 14, 2024

The order of operations changes in this PR should make it impossible to pull something from the wrong path, if it exists on the mingw_prefix bin path in the first place.

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.

None yet

2 participants