-
Notifications
You must be signed in to change notification settings - Fork 5.1k
draft update AOT pipeline to no longer require ci parameters #53752
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -1,15 +1,49 @@ | |||
| param( | |||
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.
Do you intend to keep this as a script as opposed to moving it into the azsdk cli?
If we do keep it as a script we should consider changing the args to take a package path instead of a service directory and package name.
| } | ||
| Write-Host "Running Check-AOT-Compatibility.ps1 for Package: $($package.ArtifactName) Service $($package.ServiceDirectory)" | ||
|
|
||
| $scriptPath = Join-Path $PSScriptRoot "Check-AOT-Compatibility.ps1" |
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.
This is a place we should consider switching to call the azsdk cli command instead of calling the script directly.
|
|
||
| dotnet msbuild ` | ||
| /nologo ` | ||
| /t:GetAotCompatOptOut ` |
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.
We don't need a custom target to get properties any longer. See https://learn.microsoft.com/en-us/visualstudio/msbuild/evaluate-items-and-properties?view=vs-2022
dotnet msbuild -getProperty:AotCompatOptOut
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 can see an example we did in the mcp repo at https://github.com/microsoft/mcp/blob/main/eng/scripts/Get-ProjectProperties.ps1#L60C38-L60C49
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.
ah thanks so much, I was trying to figure that out but wasn't finding what I needed
| <EnableSourceLink Condition="'$(EnableSourceLink)' == ''">true</EnableSourceLink> | ||
| <DefineConstants Condition="'$(BuildSnippets)' == 'true'">$(DefineConstants);SNIPPET</DefineConstants> | ||
| <AotCompatOptOut>false</AotCompatOptOut> | ||
| <AotAnalyzersOptOut>false</AotAnalyzersOptOut> |
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.
Should we condition these with IsShippingLibrary?
| <WriteLinesToFile File="$(OutputProjectFilePath)" Lines="@(_WriteToLines)" /> | ||
| </Target> | ||
|
|
||
| <Target Name="GetAotCompatOptOut"> |
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.
See my other comment but this shouldn't be needed. We probably should clean-up the other similar cases now as well.
| exit 0 | ||
| } | ||
|
|
||
| $filteredPackages = Get-ChildItem -Path $PackageInfoFolder -Filter "*.json" -File ` |
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.
My hope is that we can eliminate the need for this "PR" script completely. I would like to see us compute the list of changed projects in one place (which we have I believe already) and just do a simple loop over inline and call the azsdk cli for each.
No description provided.