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

Test issues when running from a folder containing a space character in its name #629

Merged
merged 6 commits into from
Aug 18, 2021
Merged

Conversation

hrumhurum
Copy link
Contributor

When the DotNet.Script solution resides in a folder containing a space character in its name, an avalanche of test failures occurs due to the absence of escaping of command line arguments.

The enclosed pull request fixes all those issues.

Besides tests, there was a similar issue in DotNetRestorer class that was fixed as well by employing a batte-tested string escaping algorithm from Gapotchenko.FX.Diagnostics.CommandLine package.

An intricate detail of NuGet's -s argument is that it may end with \ character (e.g. the last separator in a folder path), so such a naive escaping approach as $"-s \"{s}\"" would not work here, as it will cause an unanticipated escape of the escape character ". Hence the usage of a proper escape function here to isolate the customer-supplied data.

@filipw
Copy link
Member

filipw commented Aug 17, 2021

Thanks for catching this!

Besides tests, there was a similar issue in DotNetRestorer class that was fixed as well by employing a batte-tested string escaping algorithm from Gapotchenko.FX.Diagnostics.CommandLine package.

One note here, I would prefer to inline the escaping logic because the dependency model package ships with OmniSharp and we intentionally do not want to create extra dependencies for OmniSharp.

@hrumhurum
Copy link
Contributor Author

I would prefer to inline the escaping logic because the dependency model package ships with OmniSharp and we intentionally do not want to create extra dependencies for OmniSharp.

Done. The corresponding code is now inlined.

@filipw filipw merged commit bf52486 into dotnet-script:master Aug 18, 2021
@filipw
Copy link
Member

filipw commented Aug 18, 2021

thanks!

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.

None yet

3 participants