AzureDevOpsExtension icon indicating copy to clipboard operation
AzureDevOpsExtension copied to clipboard

Fixes #63: __dirname must be quoted to support paths with spaces.

Open ladenedge opened this issue 3 years ago • 13 comments

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 have windowsVerbatimArguments: true, which keeps spawn() from quoting its arguments.

ladenedge avatar Feb 02 '22 15:02 ladenedge

Oops, looks like a PAT might have expired?

Received response 401 (Not Authorized). Check that your personal access token is correct and hasn't expired.

ladenedge avatar Feb 02 '22 16:02 ladenedge

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.

ChrisLGardner avatar Feb 02 '22 16:02 ChrisLGardner

Looks like non-Windows don't like this approach, I suspect it's the \ in the path for the script.

ChrisLGardner avatar Feb 02 '22 19:02 ChrisLGardner

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.

ChrisLGardner avatar Feb 02 '22 20:02 ChrisLGardner

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.

ChrisLGardner avatar Feb 03 '22 17:02 ChrisLGardner

I'm probably just dumb, but how come the Mac/Linux failures still show a backslash?

The argument '"/Users/runner/work/_tasks/Pester_cca5462b-887d-4617-bf3f-dcf0d3c622e9/10.3.12\Pester.ps1"' is not recognized

ladenedge avatar Feb 04 '22 13:02 ladenedge

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

ChrisLGardner avatar Feb 04 '22 13:02 ChrisLGardner

Ah, gotcha, thank you. Okay, I'll see about making the quotes conditional on the OS.

ladenedge avatar Feb 04 '22 14:02 ladenedge

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 Program Files directory?

ladenedge avatar Feb 04 '22 14:02 ladenedge

Sorry for the spam -- just realized I could try that my damn self! Quotes work just fine for me in Ubuntu.

laden@CAST:~$ pwsh "Program Files/hello.ps1"
Hello, world!

I bet it's the non-Windows spawn() that's actually escaping our quotes -- but not on Windows due to windowsVerbatimArguments: true. It would probably also work to remove that flag so spawn() can handle the quoting on all platforms.

Did we have a good reason for windowsVerbatimArguments: true?

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!

ladenedge avatar Feb 04 '22 14:02 ladenedge

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.

ChrisLGardner avatar Feb 04 '22 14:02 ChrisLGardner

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?

ladenedge avatar Feb 04 '22 15:02 ladenedge

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.

ChrisLGardner avatar Feb 04 '22 15:02 ChrisLGardner