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: Support working directories containing whitespace #646

Merged
merged 6 commits into from
Oct 9, 2024

Conversation

osteensco
Copy link
Contributor

addresses #546 #472, probably more

I believe this solution is OS agnostic but happy to adjust if it doesn't work somewhere.

@osteensco osteensco changed the title encapsulate bin path with quotes to account for whitespace FIX: Support working directories containing whitespace Sep 21, 2024
@osteensco
Copy link
Contributor Author

osteensco commented Sep 23, 2024

sorry for the messy merges... I was having issues with some of the tests that seemed unrelated to my changes, and the build process on github kept failing so I tried to catchup since I saw there was a linter problem fixed.

I had the same issues with the tests trying to run them on the main branch of my fork as well, even when my fork was caught up.

I ended up just testing my PR manually on a local project and it works on my machine 🤷‍♂️ . Is there a known issue with some of the tests in their current state? I couldn't find any open issues about it.

@cosmtrek cosmtrek merged commit df13da5 into air-verse:master Oct 9, 2024
5 of 7 checks passed
@omarattia3143
Copy link

this is causing issue #659, I am not sure why does space in path an cause issues, test paths with spaces using windows 11 it works fine, so I am not sure how to test this but could you use something like this?
if strings.Contains(c.Build.Bin, " ") && !strings.HasPrefix(c.Build.Bin, "\"") && !strings.HasSuffix(c.Build.Bin, "\"") { c.Build.Bin = "\"" + c.Build.Bin + "\"" }

@cosmtrek
Copy link
Collaborator

@omarattia3143 Sorry for the bug, I don't have windows for testing, PR is welcome~

@omarattia3143
Copy link

@osteensco can you confirm that this PR still works with this code? I have only windows and I can not test your changes

this is causing issue #659, I am not sure why does space in path an cause issues, test paths with spaces using windows 11 it works fine, so I am not sure how to test this but could you use something like this? if strings.Contains(c.Build.Bin, " ") && !strings.HasPrefix(c.Build.Bin, "\"") && !strings.HasSuffix(c.Build.Bin, "\"") { c.Build.Bin = "\"" + c.Build.Bin + "\"" }

@@ -327,6 +329,9 @@ func (c *Config) preprocess() error {
// CMD will not recognize relative path like ./tmp/server
c.Build.Bin, err = filepath.Abs(c.Build.Bin)

// Account for spaces in filepath
c.Build.Bin = fmt.Sprintf("%q", c.Build.Bin)

Choose a reason for hiding this comment

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

I think this is causing an error on Windows, the exe can't be found and the path includes escape characters like this:

'\"c:\\go\\hello\\tmp\\hello.exe\"' is not recognized as an internal or external command,
operable program or batch file.

When if it was actually missing the error would be: this when the exe is missing and I run it from a command prompt

'c:\go\hello\tmp\hello.exe' is not recognized as an internal or external command,
operable program or batch file.

@moboqe
Copy link

moboqe commented Oct 15, 2024

this is causing issue #659, I am not sure why does space in path an cause issues, test paths with spaces using windows 11 it works fine, so I am not sure how to test this but could you use something like this? if strings.Contains(c.Build.Bin, " ") && !strings.HasPrefix(c.Build.Bin, "\"") && !strings.HasSuffix(c.Build.Bin, "\"") { c.Build.Bin = "\"" + c.Build.Bin + "\"" }

This seems not working either

@omarattia3143
Copy link

@moboqe this is not merged, It works if you dont have space in your path, I made it so it can bypass the issue created by this PR.
I am not sure if this PR works for other platforms.
@cosmtrek I think the best is to revert this PR since the author is not responding and it is affecting all windows users

@moboqe
Copy link

moboqe commented Oct 15, 2024

@moboqe this is not merged, It works if you dont have space in your path, I made it so it can bypass the issue created by this PR. I am not sure if this PR works for other platforms. @cosmtrek I think the best is to revert this PR since the author is not responding and it is affecting all windows users

I mean this doesn't work for me related for this issue.
Yep, I also fixed those issue by adding check for space in path

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.

5 participants