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

Update razzle to use vswhere (#13) #606

Merged
merged 5 commits into from
May 10, 2019
Merged

Update razzle to use vswhere (#13) #606

merged 5 commits into from
May 10, 2019

Conversation

amweiss
Copy link
Contributor

@amweiss amweiss commented May 9, 2019

Instead of listing out all the locations for MSBuild, use the vswhere tool to find the location and add that directory to that path.

@msftclas
Copy link

msftclas commented May 9, 2019

CLA assistant check
All CLA requirements met.

@DHowett-MSFT
Copy link
Contributor

I can't figure out where in the nuget docs it specifies that .nuget is a good and sane location, but I'm glad it works.

tools/razzle.cmd Outdated
)

rem Add path to MSBuild Binaries
for /f "usebackq tokens=*" %%i in (`%VSWHERE% -latest -requires Microsoft.Component.MSBuild -find MSBuild\**\Bin\MSBuild.exe`) do set MSBUILD=%%i
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add -products * if you want to also support Visual Studio Build Tools. By default, vswhere only supports finding Community, Professional, and Enterprise - our traditional VS SKUs.

@amweiss amweiss marked this pull request as ready for review May 9, 2019 11:02
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple questions, but I'm not gonna block this over them

tools/razzle.cmd Show resolved Hide resolved
tools/razzle.cmd Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@amweiss
Copy link
Contributor Author

amweiss commented May 9, 2019

Couple questions, but I'm not gonna block this over them

I can tackle these after work today, or on another PR later if you want this in sooner.

@zadjii-msft
Copy link
Member

@amweiss I'm in no rush - my unread inbox is expanding at just about the rate I reply to things :P

@amweiss
Copy link
Contributor Author

amweiss commented May 9, 2019

Ha, I expected it would be worse than that by now 😁. Anyway, changes made. I did expand the scope a bit when I changed the readme to use bcz since it failed when the MSBUILD path had a space, so I fixed that by wrapping it in quotes in bcz.

@miniksa
Copy link
Member

miniksa commented May 10, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@miniksa miniksa merged commit cafe59d into microsoft:master May 10, 2019
@amweiss amweiss deleted the vswhere-based-razzle branch May 10, 2019 18:47
ReneeGA2020 pushed a commit to ReneeGA2020/Terminal that referenced this pull request May 11, 2019
* Update razzle to use vswhere

* Make vswhere pickup build tools

* Make razzle handle errors better

* Make bcz handle MSBUILD with spaces

* Update readmes to use bcz and fix typo

rem Find vswhere
rem from https://github.com/microsoft/vs-setup-samples/blob/master/tools/vswhere.cmd
for /f "usebackq delims=" %%I in (`dir /b /aD /o-N /s "%~dp0..\packages\vswhere*" 2^>nul`) do (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this needs to be a batch file rather than using PowerShell? I imagine this could be clearer than the obscure dir command if it used PowerShell.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just following what was already there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because not everyone likes Powershell. Our dev team is split between users of Powershell and cmd, so we have scripts for both.

metathinker added a commit to metathinker/console that referenced this pull request May 29, 2019
Since version 5.0.2, NuGet has used the PATH environment variable
to find MSBuild.exe before looking in other file paths.
See NuGet change
NuGet/NuGet.Client@21f2b07
(NuGet/NuGet.Client#2687 ).

Unfortunately, in PR
microsoft#606 ,
`tools\razzle.cmd` was changed to add the MSBuild.exe folder path
in _quotes_ to the PATH environment variable.
Windows itself is fine with this (you can type `msbuild` and
MSBuild runs), but some tools are not, including NuGet itself,
so you would get errors like this:

```
D:\GitHub\metathinker\console> where nuget
C:\ProgramData\chocolatey\bin\nuget.exe
D:\GitHub\metathinker\console\dep\nuget\nuget.exe

D:\GitHub\metathinker\console> nuget restore OpenConsole.sln
Illegal characters in path.
```

`razzle.cmd` runs NuGet itself, but does so before adding
the MSBuild folder to the PATH, so it was not affected by this
problem.

This change fixes the issue by dequotifying the PATH,
so that if you already had a newer version of NuGet on your PATH
before running `tools\razzle.cmd`, that version will continue
to work should you need to run `nuget restore` again
(such as after a `git clean -dx`).
DHowett-MSFT pushed a commit that referenced this pull request May 29, 2019
…1046)

Since version 5.0.2, NuGet has used the PATH environment variable
to find MSBuild.exe before looking in other file paths.
See NuGet change
NuGet/NuGet.Client@21f2b07
(NuGet/NuGet.Client#2687 ).

Unfortunately, in PR
#606 ,
`tools\razzle.cmd` was changed to add the MSBuild.exe folder path
in _quotes_ to the PATH environment variable.
Windows itself is fine with this (you can type `msbuild` and
MSBuild runs), but some tools are not, including NuGet itself,
so you would get errors like this:

```
D:\GitHub\metathinker\console> where nuget
C:\ProgramData\chocolatey\bin\nuget.exe
D:\GitHub\metathinker\console\dep\nuget\nuget.exe

D:\GitHub\metathinker\console> nuget restore OpenConsole.sln
Illegal characters in path.
```

`razzle.cmd` runs NuGet itself, but does so before adding
the MSBuild folder to the PATH, so it was not affected by this
problem.

This change fixes the issue by dequotifying the PATH,
so that if you already had a newer version of NuGet on your PATH
before running `tools\razzle.cmd`, that version will continue
to work should you need to run `nuget restore` again
(such as after a `git clean -dx`).
donno2048 added a commit to donno2048/terminal that referenced this pull request Sep 28, 2020
…#1046)

Since version 5.0.2, NuGet has used the PATH environment variable
to find MSBuild.exe before looking in other file paths.
See NuGet change
NuGet/NuGet.Client@21f2b07
(NuGet/NuGet.Client#2687 ).

Unfortunately, in PR
microsoft/terminal#606 ,
`tools\razzle.cmd` was changed to add the MSBuild.exe folder path
in _quotes_ to the PATH environment variable.
Windows itself is fine with this (you can type `msbuild` and
MSBuild runs), but some tools are not, including NuGet itself,
so you would get errors like this:

```
D:\GitHub\metathinker\console> where nuget
C:\ProgramData\chocolatey\bin\nuget.exe
D:\GitHub\metathinker\console\dep\nuget\nuget.exe

D:\GitHub\metathinker\console> nuget restore OpenConsole.sln
Illegal characters in path.
```

`razzle.cmd` runs NuGet itself, but does so before adding
the MSBuild folder to the PATH, so it was not affected by this
problem.

This change fixes the issue by dequotifying the PATH,
so that if you already had a newer version of NuGet on your PATH
before running `tools\razzle.cmd`, that version will continue
to work should you need to run `nuget restore` again
(such as after a `git clean -dx`).
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.

7 participants