-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 build errors on Windows when project path contains spaces #196
Conversation
@dotnet-bot test Windows_NT Release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tincann for the help, it's much appreciated.
I just have a couple minor comments.
src/Native/build.cmd
Outdated
@@ -37,7 +36,8 @@ if exist %_VSWHERE% ( | |||
if not exist "%_VSCOMNTOOLS%" set _VSCOMNTOOLS=%VS140COMNTOOLS% | |||
if not exist "%_VSCOMNTOOLS%" goto :MissingVersion | |||
|
|||
set VSCMD_START_DIR="%__currentScriptDir%" | |||
|
|||
set VSCMD_START_DIR=%__currentScriptDir% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this set different than the 2 below? For example, the two below have the format set "name=%__currentScriptDir%"
where this one doesn't have the surrounding double-quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I've added the quotes again (also around the variable for consistency).
@@ -44,7 +44,7 @@ | |||
|
|||
<!-- Run script that invokes Cmake to create VS files, and then calls msbuild to compile them --> | |||
<Message Text="$(MSBuildProjectDirectory)\build.cmd $(BuildArgs)" Importance="High"/> | |||
<Exec Command="$(MSBuildProjectDirectory)\build.cmd $(BuildArgs)" /> | |||
<Exec Command=""$(MSBuildProjectDirectory)\build.cmd" $(BuildArgs)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears the unix build has the same issue. Would it be possible to fix the Unix build as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly, I have some time in the end of the week. Shall I put this in a new pull request, so the Windows fix can already be put to use now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is totally fine with me to create another PR for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #191
The problem seemed to be that
%~dp0
was used in the batch scripts without proper quoting.