-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Upstream changes from Vertical Build PoC #71666
Changes from 14 commits
5a028bb
8a40d54
6c9c891
d50da23
e6093d0
6abad35
670c398
273422b
fc0e653
cdada0f
01f12f2
2d1d923
c1b1501
b1335d2
0e038a5
da05c1a
7a2ec71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -403,8 +403,11 @@ stages: | |
arguments: -test -configuration Release | ||
condition: or(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['compilerChange'], 'true')) | ||
|
||
- script: $(Build.SourcesDirectory)\.dotnet\dotnet.exe tool run dotnet-format whitespace $(Build.SourcesDirectory)\src --folder --include-generated --include $(Build.SourcesDirectory)\src\Compilers\CSharp\Portable\Generated\ $(Build.SourcesDirectory)\src\Compilers\VisualBasic\Portable\Generated\ $(Build.SourcesDirectory)\src\ExpressionEvaluator\VisualBasic\Source\ResultProvider\Generated\ --verify-no-changes | ||
- task: PowerShell@2 | ||
displayName: Validate Generated Syntax Files | ||
inputs: | ||
filePath: eng/validate-code-formatting.ps1 | ||
arguments: -rootDirectory $(Build.SourcesDirectory)\src -includeDirectories Compilers\CSharp\Portable\Generated\, Compilers\VisualBasic\Portable\Generated\, ExpressionEvaluator\VisualBasic\Source\ResultProvider\Generated\ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jaredpar I tweaked this so it doesn't assume that the SDK is installed at the root of the repo (instead, uses Ensure-DotNetSDK). I set the validation tooling to diag for this PR iteration to ensure that things are operating as expected. I'm pretty sure that the current format command is incorrect. It appears that the include directories are expected to be paths relative to the root dir ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking good now. 11 generated files from those directories checked. I changed the verbosity back down to detailed. It prints some details (number of files) that are useful. |
||
condition: or(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['compilerChange'], 'true')) | ||
|
||
- template: eng/pipelines/publish-logs.yml | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ function Run-LanguageCore($language, $languageSuffix, $languageDir, $syntaxProje | |
function Test-GeneratedContent($generatedDir, $scratchDir) { | ||
$algo = "MD5" | ||
foreach ($fileName in (Get-ChildItem $scratchDir)) { | ||
$fileName = $fileName.Name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this change needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On my local powershell installation (powershell core 7), Get-ChildItem is returning full file objects. So when you do Join-Path below you actually get: C:\r\roslyn\src\generatedfoobar\C:\r\roslyn\src\generatedfoobar\Foo.cs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can definitely revert this if you want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tweaked this so that it selects |
||
Write-Host "Checking $fileName" | ||
$realFilePath = Join-Path $generatedDir $fileName | ||
$scratchFilePath = Join-Path $scratchDir $fileName | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
param ( | ||
[string]$rootDirectory, | ||
[string[]]$includeDirectories | ||
) | ||
|
||
mmitche marked this conversation as resolved.
Show resolved
Hide resolved
|
||
try { | ||
. (Join-Path $PSScriptRoot "build-utils.ps1") | ||
Push-Location $RepoRoot | ||
|
||
$dotnet = Ensure-DotnetSdk | ||
Exec-Console $dotnet "tool run dotnet-format -v detailed whitespace $rootDirectory --folder --include-generated --include $includeDirectories --verify-no-changes" | ||
|
||
exit 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was following the generate-compiler.code.ps1 patterns https://github.com/dotnet/roslyn/pull/71666/files#diff-64ff4bf5168c08ddeb40d75a8889a9ad53f4f2bf3ff1b320a320cdf3567227c0R139 |
||
} | ||
catch { | ||
Write-Host $_ | ||
exit 1 | ||
} | ||
finally { | ||
Pop-Location | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,10 @@ | |
"The assembly '...' is not inside the 'lib' folder and hence it won't be added as a reference when the package is installed into a project." | ||
--> | ||
<NoWarn>$(NoWarn);NU5100</NoWarn> | ||
|
||
<!-- Work around missing project dependencies. VS features not needed at this time: | ||
https://github.com/dotnet/source-build/issues/3981. --> | ||
<ExcludeFromVerticalBuild>true</ExcludeFromVerticalBuild> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious: if we set something as excluded from vertical build is it automatically excluded from source build? That would save us a lot of duplicate properties if true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will with the new unified build controls. "ExcludeFromVerticalBuild" is the PoC switch. It becomes a general |
||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,5 +2,8 @@ | |
<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" /> | ||
<PropertyGroup> | ||
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild> | ||
<!-- TODO: https://github.com/dotnet/source-build/issues/3981 | ||
Remove if necessary. --> | ||
<ExcludeFromVerticalBuild>true</ExcludeFromVerticalBuild> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having both properties seems unnecessary. What code is excuded from the vertical build but included in source build? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expect this to go away when the new Unified build controls come to this repo. That will happen after the implementation is complete (very soon) and when Roslyn moves to .NET 9 Arcade (late). Whenever you see both of these conditions, they would be replaced by |
||
</PropertyGroup> | ||
</Project> |
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.
Curious: why did you pick the PowerShell task here vs using
pwsh
? That is installed in our repo as a local tool. Generally I try and use that when possible.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.
Will do.