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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ladenedge
Copy link

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
Copy link
Author

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.

@ChrisLGardner
Copy link
Collaborator

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).
@ChrisLGardner
Copy link
Collaborator

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

@ChrisLGardner
Copy link
Collaborator

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
Copy link
Collaborator

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.

@ladenedge
Copy link
Author

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

@ChrisLGardner
Copy link
Collaborator

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

@ladenedge
Copy link
Author

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

@ladenedge
Copy link
Author

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
Copy link
Author

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!

@ChrisLGardner
Copy link
Collaborator

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.

@ladenedge
Copy link
Author

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?

@ChrisLGardner
Copy link
Collaborator

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.

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.

The term 'C:\Program' is not recognized as the name of a cmdlet
2 participants