-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fixes #63: __dirname must be quoted to support paths with spaces. #64
base: master
Are you sure you want to change the base?
Conversation
Oops, looks like a PAT might have expired?
|
Thanks for this. I'll have a look what's happening with the 401, probably is an expired PAT so should be easy enough to fix. |
Fails if no image is specified as ubuntu16 isn't a valid label (presumably a default somewhere in the background).
Looks like non-Windows don't like this approach, I suspect it's the |
…ashes dont work on non-windows.
Looks like the agent isn't updating with the latest version of the task. I'll give it an hour or so to think about things and then rerun it hoping it gets the right version of the task. |
Looks like the change to the slash hasn't fixed the issue on non-windows. It might be worth building up the path before we use it and wrap it in a conditional that checks for windows and wraps it in quotes then. |
I'm probably just dumb, but how come the Mac/Linux failures still show a backslash?
|
I reran it manually and it uses a newer version of the task where it's got a forward slash: https://dev.azure.com/halbaradkenafin/Github/_build/results?buildId=1288&view=logs&j=9688f48a-2893-52c3-8abd-64e9f735f0f5&t=70de6170-bc55-5eba-f7fd-18f49e0a4630 |
Ah, gotcha, thank you. Okay, I'll see about making the quotes conditional on the OS. |
Actually, one other thought: would it make sense to make the quotes conditional on a space in the path? How would Linux behave if it were running in a |
Sorry for the spam -- just realized I could try that my damn self! Quotes work just fine for me in Ubuntu.
I bet it's the non-Windows Did we have a good reason for I'll assume yes and go with your original idea of a platform switch. It'd be cleaner to just remove that flag if you think it's safe, though. Lemme know! |
I'm not sure the reasoning for adding it, I didn't write the initial version of the javascript file. It might be worth testing it without that set and see what happens with the current tests. |
I'm surprised these tests are failing. I reverted the slash/quote change entirely and just removed an option that according to the docs is, "Ignored on Unix." Is it possible the build is out of date again? |
It looks like that's what's happening, it's still pulling down the last version of the task. In theory the verifaction step should stop that but it doesn't seem to work 100% of the time. I'll give it a little while and run the stage again to see if it works this time. |
What problem does this PR address?
This addresses #63, when an agent is installed in a directory with spaces. (Eg.
C:\Program Files
.)Is there a related Issue?
This is intended to fix #63.
Other notes
It looks like
spawn()
would normally take care of this itself except that we havewindowsVerbatimArguments: true
, which keeps spawn() from quoting its arguments.