Skip to content

Improve VS Code build current project support#43682

Merged
2 commits merged intodotnet:masterfrom
jaredpar:build
Apr 26, 2020
Merged

Improve VS Code build current project support#43682
2 commits merged intodotnet:masterfrom
jaredpar:build

Conversation

@jaredpar
Copy link
Member

This makes the following changes to our ability to build the current
project from VSCode (essentially implementing the rough equivalent of
build current project in VS).

  • Ability to choose msbuild in addition to dotnet msbuild. The full
    framework msbuild is still necessary for building parts of the IDE
    code base.
  • Support for VB projects
  • Fixes the execution of the task on Windows. The hard dependency on
    /.dotnet existing doesn't hold on Windows. Falling back to normal
    powershell in that environment

This makes the following changes to our ability to build the current
project from VSCode (essentially implementing the rough equivalent of
build current project in VS).

- Ability to choose `msbuild` in addition to `dotnet msbuild`. The full
framework `msbuild` is still necessary for building parts of the IDE
code base.
- Support for VB projects
- Fixes the execution of the task on Windows. The hard dependency on
`/.dotnet` existing doesn't hold on Windows. Falling back to normal
powershell in that environment
@jaredpar jaredpar requested a review from a team as a code owner April 25, 2020 15:02
@jaredpar
Copy link
Member Author

@333fred @RikkiGibson PTAL. I'm fairly confident I maintained the non-windows behavior for these tasks but would appreciate if you all could validate this. I have validated these changes work correctly on windows for both the dotnet msbuild and msbuild cases.

}

function Run-MSBuild([string]$projectFilePath, [string]$buildArgs = "", [string]$logFileName = "", [switch]$parallel = $true, [switch]$summary = $true, [switch]$warnAsError = $true, [string]$configuration = $script:configuration) {
function Run-MSBuild([string]$projectFilePath, [string]$buildArgs = "", [string]$logFileName = "", [switch]$parallel = $true, [switch]$summary = $true, [switch]$warnAsError = $true, [string]$configuration = $script:configuration, [switch]$skipAnalyzers = $false) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This function was relying on $skipAnalyzers being implicitly defined. That seemed dangerous so I had it be passed as an explicit argument and fixed up the callers to use this.

param (
[Parameter(Mandatory = $true)][string]$filePath
[Parameter(Mandatory = $true)][string]$filePath,
[string]$msbuildEngine = "vs"
Copy link
Member Author

Choose a reason for hiding this comment

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

This parameter effectively acts like a global variable here. The implementation of InitializeBuildTool below relies on reading this variable to pick the appropriate build engine.

This pattern is how our core build.ps1 script works and I'm essentially replicating it here. That should make this script more resilient over time as it should evolve in sync with our core build logic.

Copy link
Member

Choose a reason for hiding this comment

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

And InitializeBuildTool is in arcade and hard coded to dotnet.exe :(

Co-Authored-By: Rikki Gibson <rikkigibson@gmail.com>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit 5e765f4 into dotnet:master Apr 26, 2020
$invocation = "$dotnetPath msbuild $projectDir -p:UseRoslynAnalyzers=false -p:GenerateFullPaths=true"
Write-Output "> $invocation"
Invoke-Expression $invocation
$buildTool = InitializeBuildTool
Copy link
Member

Choose a reason for hiding this comment

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

Remember when I approved this? Yeah, that was a mistake.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants