-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Move to building with the 9.0 SDK and move our ToolCurrent TFM to 9.0 #95980
Conversation
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue Detailsnull
|
Looks like public static class PathUtilities
{
-#if NET8_0
- public const string TFMDirectoryName = "net8.0";
-#elif NET7_0
- public const string TFMDirectoryName = "net7.0";
-#elif NET6_0
- public const string TFMDirectoryName = "net6.0";
-#elif NET5_0
- public const string TFMDirectoryName = "net5.0";
-#elif NETCOREAPP3_0
- public const string TFMDirectoryName = "netcoreapp3.0";
-#elif NET471
+#if NET471
public const string TFMDirectoryName = "net471";
#else
-#error "Unknown TFM"
+ public static readonly string TFMDirectoryName = $"net{Environment.Version.Major}.{Environment.Version.Minor}";
#endif (dropping netcoreapp3.0) |
@sbomer @vitek-karas can you please help with the linker test failures? |
@mikem8361 can you please take a look at the failing |
I haven't work on ENC/hot reload/loader in 2 years. I assume some .NET 9 loader change broke this test. @mikelle-rogers might have an idea of what broken. |
I see @akoeplinger is updating dotnet/hotreload-utils#348. Maybe he's already onto what fix is needed here? My best guess was that the hot reload diff somehow broke custom attribute type lookup. Tried to debug, but these tests don't like that If @akoeplinger's fix to the hot reload tools doesn't resolve this, I recommend we just file an issue for the hot-reload owners to fix. Edit: after closer inspection - probably that change is unrelated. Still not sure what's going on that would cause this hot reload test to start failing. |
I'm afraid I cannot add much more to what Eric has already discovered. Binary diffing of the |
Updating to latest hotreload-utils and newer SDK fixed this locally, let's see what CI says. |
I simplified the path expectations logic in the linker tests but had to push individual commits (and manually) as my codespace doesn't have permissions to push to this fork. |
@radical please take a look at all the wasm/wasi failures |
addresses review feedback from @ viktorhofer .
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
RuntimePackLabels="Mono" | ||
Condition="'$(UseLocalTargetingRuntimePack)' == 'true' and ('@(KnownRuntimePack)' == '' or @(KnownRuntimePack->WithMetadataValue('Identity', 'Microsoft.NETCore.App')->WithMetadataValue('RuntimePackLabels', 'Mono')->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)')) == '')" /> | ||
<!-- always add wasi-wasm as it is never added by the sdk --> | ||
<KnownRuntimePack Update="@(KnownRuntimePack->WithMetadataValue('Identity', '$(LocalFrameworkOverrideName)')->WithMetadataValue('RuntimePackLabels', 'Mono')->WithMetadataValue('TargetFramework', '$(NetCoreAppCurrent)'))" | ||
RuntimePackRuntimeIdentifiers="%(RuntimePackRuntimeIdentifiers);wasi-wasm" |
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 this RID addition be up-streamed into dotnet/sdk?
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.
wasi-wasm is only supported with the workload right now, so we add that as part of the workload. And adding here for in-tree builds. So, upstreaming it is not needed right now.
I have been trying to debug the last remaining failures on WBT, and it seems to be down to
.. and we wait for 3mins but the process never seems to exit. |
I can reproduce this only on CI. And it does not happen for the first few times/tests. |
.. instead of an old custom implementation.
`Process.Kill` returns immediately, and WaitOnExit needs to be used to wait for the actual exit.
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
I don't think that any of those issues block merging the PR. Given that we need to get this in ASAP for being able to ship .NET 9 Preview 1, I'm merging this in now. |
Unblocks #95904