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

[Windows] Fix handling of paths in console wrapper #99429

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

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Nov 19, 2024

Wrapper does not handle paths with spaces

There might be an alternative way to add this or format this, but this solves the problem, I'm surprised this hasn't popped up before (it only did for me now because my new laptop was configured in the store and had "First-name Last-name" as my account, triggering this)

Additionally we might want to make the error message a bit more descriptive (I had to hack in printing the command line string to discover what was even wrong for this), but not touching that right now

Edit: It seems to be in very specific paths, see the report

@AThousandShips AThousandShips added bug platform:windows topic:export cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 19, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 19, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner November 19, 2024 15:47
@akien-mga akien-mga requested a review from bruvzg December 4, 2024 23:52
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

What's exactly no working? Paths with spaces seems to be already handled fine without any changes, I can't reproduce any issues.

@AThousandShips
Copy link
Member Author

I'll send the error message later today, it fails to run if the path to the main executable contains spaces

@bruvzg
Copy link
Member

bruvzg commented Dec 5, 2024

it fails to run if the path to the main executable contains spaces

I have tried path with spaces, and executable name with spaces in various combinations and all work with issues.

@AThousandShips
Copy link
Member Author

The error is CreateProcess failed, error 193, with a path with spaces in the path, not necessarily in the executable

No error with this fix

Wrapper does not handle paths with spaces
@bruvzg
Copy link
Member

bruvzg commented Jan 9, 2025

Tried it again and still can't reproduce the issue, but it seems to work the same with this change, so I guess it's OK.

@AThousandShips
Copy link
Member Author

With a user name with a space in it?

@bruvzg
Copy link
Member

bruvzg commented Jan 9, 2025

With a user name with a space in it?

With multiple spaces in the folder names and the executable names in different combinations, not username specifically, but there's nothing special about user folder so it should not matter.

@AThousandShips
Copy link
Member Author

There is, see the issue, it fails specifically with that case

@bruvzg
Copy link
Member

bruvzg commented Jan 9, 2025

Will check it with username in a moment.

CreateProcess failed, error 193

The error is also strange, it's X is not a valid Win32 app. Usually it should indicate the executable is not current (like trying to run ARM64 exe on x86), not a path issue.

@bruvzg
Copy link
Member

bruvzg commented Jan 9, 2025

Will check it with username in a moment.

No, it's still not reproducible.

@AThousandShips
Copy link
Member Author

That is very strange, no clue why it's happening to me specifically

@bruvzg
Copy link
Member

bruvzg commented Jan 9, 2025

Is it a local account or Microsoft account with "Documents" stored in OneDrive? I have tried both fully local and Microsoft accounts with local "Documents", but if it's not in a normal folder, it might have different behavior. Or it's some sort of external interference (like antivirus tempering with disk access, this might also explain strange error 193).

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 9, 2025

Will check when I'm at my laptop, I don't think it's on OneDrive, it should be a local account

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release platform:windows topic:export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Windows] Godot console wrapper fails to launch in certain paths
2 participants