-
Notifications
You must be signed in to change notification settings - Fork 408
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
Perform AOT check on repository #2598
Conversation
.github/workflows/aot-check.yml
Outdated
|
||
strategy: | ||
matrix: | ||
language: [ 'csharp' ] |
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.
what's this for? Is it used?
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 was copied from another yml file and was unused. I have removed it.
.github/workflows/aot-check.yml
Outdated
- name: Checkout repository | ||
uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 2 |
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 2?
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.
I have changed the fetch depth to 1 so it fetches the top commit only.
.github/workflows/aot-check.yml
Outdated
|
||
on: | ||
push: | ||
branches: [ "dev", "dev6x" ] |
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.
Is the script going to be checked into dev6x
? I didn't think that version was AOT compatible. I think we did all the work in v7.
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, that's correct, good point @eerhardt
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.
I have removed the dev6x branch.
build/test-aot.ps1
Outdated
|
||
|
||
|
||
|
||
|
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.
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.
Done
build/test-aot.ps1
Outdated
Write-Host $publishOutput | ||
} | ||
|
||
#$runtime = if ($IsWindows) { "win-x64" } elseif ($IsMacOS) { "macos-x64"} else {"linux-x64"} |
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.
Can we keep the script able to run on all 3 OSes? That way devs can run the script locally without having to move to a specific OS.
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.
I have restored the checks.
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.
LGTM. Thanks!
We can also delete https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/tree/dev/test/Microsoft.IdentityModel.AotCompatibility.Tests now since this replaces those tests. We can do that in this PR or in a follow up.
Perform AOT check on repository
Summary of the changes
Added a script and yml file to perform AOT check on repository for every pull request. This script will notify us if a non AOT compatible change is added to the repo.