-
Notifications
You must be signed in to change notification settings - Fork 280
Parse version > 9 in dotnet.py #4470
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
Conversation
cc @ilonatommy |
Apparently, printf wasn't showing up in CI logs, so I moved the diag to exception message. Please retry. |
Ah, so it is cloning the wrong repo :(
|
Thanks, sorry I was in my just before calling it a weekend meeting with my manager 🤣 |
That's fine. From some reason we're ignoring net10 that we parsed and we fall back to net9, see performance/scripts/performance/common.py Line 214 in f6d0cff
that logs
|
noticed this one as well: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-106599-merge-9f744b5760d349c987/x64.micro.net10.0.Partition0/1/console.a0f73dde.log?helixlogtype=result
|
I think this is understandable, considering that sdk 10 is expected to be used. |
Don't we need another case here: Line 81 in f6d0cff
We're using this method to set performance/scripts/ci_setup.py Line 511 in f6d0cff
This it not the full fix for sure, though. We have to figure out how args.channel became nativeaot9.0
Edit: aha, like this: performance/azure-pipelines.yml Line 149 in f6d0cff
|
I re-run before the final touches were done, so in case it won't pass, we'll make another round. |
Waiting for the 2nd run to finish: |
BenchmarkDotNet apparently also has a lot of hardcoded monikers https://github.com/search?q=repo%3Adotnet%2FBenchmarkDotNet%20net9&type=code Maybe we need to update BDN as well? |
Maybe this will help? At least for |
Looks like regex required non-greedy when there is more than one digit in first match. Some sort of python gotcha, but this works (tested on godbolt with net9.0 and net10.0) |
Next up:
so it is correctly passing 10.0, but 10.0 SDK is only supporting .NET 9.. see dotnet/runtime#106599 (comment) |
We might need to get the BDN change to support targeting .NET 10 out and in this as well, similar to when we made this change last year: |
scripts/channel_map.py
Outdated
'tfm': 'net9.0', | ||
'branch': '9.0', | ||
'tfm': 'net10.0', | ||
'branch': '10.0', |
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 should do this in a separate change as this shouldn't be a dependency to get dotnet/runtime#106599 working. This also means we can spend some time to get the BDN changes in as well to support wasm on .NET 10.
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.
@am11 Can you remove the change to channel_map.py then we can merge this in?
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
@caaavik-msft thanks. Just curious, are those unsupported TMFs like (netcoreapp3) and desktop CLR ones (net4XX) channels still relevant / working? If not, maybe we should clean that up and keep it to lowest supported LTS. |
Perhaps a good idea, I suppose there isn't much need for the older target frameworks. @DrewScoggins not sure if you had any other thoughts on if that might be something worth cleaning up. |
Judging by performance/src/benchmarks/gc/GC.Infrastructure/GC.Infrastructure.Core/Configurations/Common.cs Line 52 in d384536
|
Yeah, I was thinking about this. I am trying to come up with a realistic scenario that would involve us needing to go back and do performance testing on anything that old, and can't really get there. If the need did somehow arise, we can always just add it back in. I think removing the obsolete TFMs seems like a good idea. |
That is for the GC perf tests and infrastructure, which is different than the existing microbenchmarks. However, I get your point. |
A job run in PR dotnet/runtime#106599 is failing like https://dev.azure.com/dnceng-public/public/_build/results?buildId=813428&view=logs&j=3ca38e2b-df6d-5512-8718-3c103788a0f9&t=0d6eb74e-f21d-5060-4e66-099a24a67e5e