-
Notifications
You must be signed in to change notification settings - Fork 268
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
upgrade real-world to net9 #3511
Conversation
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 if the test run failures match main or there are no failures.
src/benchmarks/real-world/ILLink/SampleProject/HelloWorld.template.csproj
Show resolved
Hide resolved
@kunalspathak Do you want me to merge it? |
sure, once CI is green. |
Do we want to hold off the PR or merge and investigate then? I'll check eurotomorrow either way. Rest is known. |
I think we wait until we have had a chance to look into it a little bit, making a decision to merge with the break or not then. |
It was failing because of the change suggested in #3511 (comment). I reverted it and things work locally. |
Sorry, wrong GitHub button. |
The change seems to be caused by the change from TargetFramework to TargetFrameworks in the Illink Sample csproj here: https://github.com/dotnet/performance/pull/3511/files#diff-359fe5c6a7aea7d51cba906e3885f24452af66155f3d0568a4b7d13297278b68L4-R5. Something like this update to the publish args in the Utilities.cs -> PublishSampleProject (https://github.com/dotnet/performance/blob/main/src/benchmarks/real-world/ILLink/Utilities.cs#L64) fixed the test locally for me.
|
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Are the CI failures known and can we merge the PR? |
Yes, these failures are known. I'm waiting for @LoopedBard3's response, whether he'll include his fix here or create separate PR. |
I will go ahead and make another PR once this one goes in 👍. @cincuranet I think we are good to merge this. |
No description provided.