Update assembly version from hardcoded to MajorVersion and update NetCoreAppCurrent to net8.0 #78354

carlossanlop · 2022-11-15T01:03:05Z

Contributes to #72112
Fixes #58109

Co-authored with @radical and @ViktorHofer

Original PR was #74157 but we had to revert it #77899

Reason why it had to be reverted:

Changing assembly versions without changing the TargetFramework represented by those assemblies is causing any consuming assembly in packages that target net7.0 to have references to 8.0 versioned assemblies. This will break the consumption of those packages.

For this change to work, it cannot change referenced versions that are exposed as net7.0. The TFM targeted by packages can likely change before AssemblyVersion, but not vice-versa.

ghost · 2022-11-15T01:03:23Z

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Co-authored with @radical

Original PR was #74157 but we had to revert it #77899

Reason why it had to be reverted:

Changing assembly versions without changing the TargetFramework represented by those assemblies is causing any consuming assembly in packages that target net7.0 to have references to 8.0 versioned assemblies. This will break the consumption of those packages.

For this change to work, it cannot change referenced versions that are exposed as net7.0. The TFM targeted by packages can likely change before AssemblyVersion, but not vice-versa.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-Infrastructure`
Milestone:	-

…bits are in a LocalEchoServer.helix.targets, instead of being special-cased in sendtohelix-wasm.targets. And this is setup and used by the test projects by importing one file.

- The RemoteLoopServer, and NetCoreServer are projects used as aspnetcore middleware, and loaded by xharness. - These are built against the live artifacts, same as other projects. - But this can be a problem when the libraries in `runtime` are on a newer assembly version (say `8.0.0`), but xharness is still built with `7.0.0` libraries. - In that case, xharness fails to load the middleware: ``` Application startup exception: System.Reflection.ReflectionTypeLoadException: Unable to load one or more of the requested types. ... Could not load file or assembly 'System.Runtime, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified. at System.Reflection.RuntimeModule.GetTypes(RuntimeModule module) at System.Reflection.Assembly.GetTypes() at Microsoft.DotNet.XHarness.CLI.CommandArguments.TypeFromAssemblyArgument`1.GetLoadedTypes()+MoveNext() in /_/src/Microsoft.DotNet.XHarness.CLI/CommandArguments/Arguments/TypeFromAssemblyArgument.cs:line 29 at Microsoft.DotNet.XHarness.CLI.Commands.WebServer.<>c__DisplayClass0_0.<Start>b__9(TestWebServerOptions options) in /_/src/Microsoft.DotNet.XHarness.CLI/Commands/WebServer.cs:line 60 at Microsoft.Extensions.Options.OptionsFactory`1.Create(String name) at Microsoft.Extensions.Options.OptionsCache`1.<>c__3`1.<GetOrAdd>b__3_0(String name, ValueTuple`2 arg) at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd[TArg](TKey key, Func`3 valueFactory, TArg factoryArgument) at Microsoft.Extensions.Options.OptionsCache`1.GetOrAdd[TArg](String name, Func`3 createOptions, TArg factoryArgument) at Microsoft.DotNet.XHarness.CLI.Commands.WebServer.TestWebServerStartup.Configure(IApplicationBuilder app, IOptionsMonitor`1 optionsAccessor) in /_/src/Microsoft.DotNet.XHarness.CLI/Commands/WebServer.cs:line 126 at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor) at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr) at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) at Microsoft.AspNetCore.Hosting.ConfigureBuilder.Invoke(Object instance, IApplicationBuilder builder) at Microsoft.AspNetCore.Hosting.ConventionBasedStartup.Configure(IApplicationBuilder app) at Microsoft.AspNetCore.Hosting.WebHost.BuildApplication() ``` - Build the project in isolation from rest of the repo, so that it is built with references only from the SDK. The built assembly can then be deployed for use with xharness, just like before.

… runtime tests since the paths are set explicitly in the project

Directory.Build.props

src/libraries/System.Data.Odbc/src/System.Data.Odbc.csproj

Directory.Build.props

carlossanlop · 2022-11-17T19:14:06Z

@ViktorHofer do we need any other changes to prevent breaking the other repos, or are we good now?

I can't approve this PR because I submitted it, and I assume @ViktorHofer can't either since he contributed to it too.

@agocke / @ericstj / @smasher164 / @radical can we get a sign-off from at least one of you?

ViktorHofer

Approving others and my own changes.

AaronRobinsonMSFT · 2022-12-04T20:10:08Z

@ViktorHofer Can I do anything in DNNE to make this better?

agocke · 2022-12-05T06:44:34Z

A note about the workaround: I tried to work around the missing KnownILCompilerPack by instead adding it in targetingpacks.targets, and then removing items from PackageDownload. This wasn't working because the nativeaot tests aren't restored in a standard way:

Thanks for providing the workaround @sbomer. I just tried your original approach out and it worked for me. I removed your workaround and added a KnownILCompilerPack entry instead. Related, now that the libraries linker and aot tests use the live apphost, I had to add the clr.runtime+host.native subset to the nativeaot build lanes. I just wanted to build the apphost via the host.native subset but that didn't work without building the singlefilehost first. I opened #79144 for that.

I don't want to block this PR, but consider just redefining clr.aot to be whatever you had to write to get this to work. The clr.aot partition is meant to be "whatever's needed for AOT in common cases", not a specific set of stuff.

ViktorHofer · 2022-12-05T08:15:16Z

I don't want to block this PR, but consider just redefining clr.aot to be whatever you had to write to get this to work. The clr.aot partition is meant to be "whatever's needed for AOT in common cases", not a specific set of stuff.

That sounds like a good strategy until we have strongly defined dependencies in the repo. But before adding the clr.runtime subset to clr.aot we should first resolve the discussion in #79144 as I don't think that the clr.runtime subset is necessary at all and makes the AOT build significantly slower.

ViktorHofer · 2022-12-05T08:21:29Z

@ViktorHofer Can I do anything in DNNE to make this better?

DNNE's msbuild integration is well written and allows to customize any of the used properties. I doubt that we would need improvements there to simplify our build. The problem is dotnet/runtime itself which makes it hard to depend on live built artifacts (like the host) because of the way it currently builds in CI (runtime and libraries in parallel on separate machines).

MichalStrehovsky · 2022-12-05T09:06:17Z

That sounds like a good strategy until we have #43110. But before adding the clr.runtime subset to clr.aot we should first resolve the discussion in #79144 as I don't think that the clr.runtime subset is necessary at all and makes the AOT build significantly slower.

Agreed, especially if the host is only needed to run the RunNativeAotTestApps mode introduced in #76109. This is not a set of tests a runtime developer would run - they guard libraries invariants and are not interesting for runtime devs. I really wish we did something so that the host is not looked for in that mode (not in this PR I guess). I don't know why it's looked for.

MichalStrehovsky · 2022-12-05T09:08:53Z

docs/workflow/building/coreclr/nativeaot.md

@@ -101,7 +101,7 @@ For more advanced scenarios, look for at [Building the Tests](/docs/workflow/tes

 ### Running library tests

-Build library tests by passing the `libs.tests` subset together with the `/p:TestNativeAot=true` to build the libraries, i.e. `clr.aot+libs+libs.tests /p:TestNativeAot=true` together with the full arguments as specified [above](#building). Then, to run a specific library, go to the tests directory of the library and run the usual command to run tests for the library (see [Running tests for a single library](/docs/workflow/testing/libraries/testing.md#running-tests-for-a-single-library)) but add the `/p:TestNativeAot=true` and the build configuration that was used, i.e. `dotnet.cmd build /t:Test /p:TestNativeAot=true -c Release`.
+Build library tests by passing the `libs.tests` subset together with the `/p:TestNativeAot=true` to build the libraries, i.e. `clr.runtime+clr.aot+host.native+libs+libs.tests /p:TestNativeAot=true` together with the full arguments as specified [above](#building). Then, to run a specific library, go to the tests directory of the library and run the usual command to run tests for the library (see [Running tests for a single library](/docs/workflow/testing/libraries/testing.md#running-tests-for-a-single-library)) but add the `/p:TestNativeAot=true` and the build configuration that was used, i.e. `dotnet.cmd build /t:Test /p:TestNativeAot=true -c Release`.


Is this really necessary?

I thought the failures we saw were related to the RunNativeAotTestApps step that was added in #76109. We don't run that in this doc.

Thanks, you are right. I mixed up RunNativeAotTestApps and TestNativeAot. Will send a follow-up to revert this change to not block this PR.

As mentioned in #78354 (comment), this documentation refers to libraries tests and not `RunNativeAotTestApps` testing. Reverting the documentation change.

…SecondAttempt Update assembly version from hardcoded to MajorVersion and update NetCoreAppCurrent to net8.0 Commit migrated from dotnet/runtime@315f4ab

As mentioned in #78354 (comment), this documentation refers to libraries tests and not `RunNativeAotTestApps` testing. Reverting the documentation change.

carlossanlop added the area-Infrastructure label Nov 15, 2022

carlossanlop requested review from agocke, ViktorHofer and ericstj November 15, 2022 01:03

carlossanlop self-assigned this Nov 15, 2022

build-analysis bot mentioned this pull request Nov 15, 2022

System.IO.Pipes.Tests.NamedPipeTest_Specific.ClientConnectAsync_Cancel_With_InfiniteTimeout failing on Libraries Test Run release coreclr windows x86 Release #69101

Open

carlossanlop and others added 12 commits November 16, 2022 18:31

Update assembly version from hardcoded to MajorVersion

17764b9

Adjust Microsoft.VisualBasic.Core

04be22b

Adjust System.Diagnostics.EventLog

e1ab7c4

Adjust System.Resources.Extensions

b74da6d

Adjust System.Runtime.Serialization.Xml

498421e

Extract the LocalEchoServer bits from the projects so that the helix …

7b33ff9

…bits are in a LocalEchoServer.helix.targets, instead of being special-cased in sendtohelix-wasm.targets. And this is setup and used by the test projects by importing one file.

Add missing files

2a40f98

Use eng/targetingpacks.targets instead of wasm specific targets

39e41c8

Avoid duplicate imports, and don't use the targetingpacks.targets for…

bda1ada

… runtime tests since the paths are set explicitly in the project

Update NetCoreAppCurrent to net8.0 and NetCoreAppPrevious

406dc14

Add NetCoreAppPrevious tfm to out-of-band projects

057c3f8

ViktorHofer force-pushed the Net8ChangesSecondAttempt branch from c5dc79a to 057c3f8 Compare November 16, 2022 19:13

carlossanlop marked this pull request as ready for review November 16, 2022 20:33

carlossanlop requested a review from radical as a code owner November 16, 2022 20:33

carlossanlop commented Nov 16, 2022

View reviewed changes

Directory.Build.props Show resolved Hide resolved

carlossanlop commented Nov 16, 2022

View reviewed changes

src/libraries/System.Data.Odbc/src/System.Data.Odbc.csproj Show resolved Hide resolved

carlossanlop commented Nov 16, 2022

View reviewed changes

Directory.Build.props Show resolved Hide resolved

carlossanlop requested a review from smasher164 November 17, 2022 19:14

carlossanlop assigned ViktorHofer Nov 17, 2022

ViktorHofer marked this pull request as draft November 17, 2022 19:20

ViktorHofer added the NO-REVIEW Experimental/testing PR, do NOT review it label Nov 17, 2022

Allow NativeExports to use a prebuilt apphost

83279ca

ViktorHofer marked this pull request as ready for review December 4, 2022 19:38

ViktorHofer requested review from lewing, pavelsavara, thaystg, marek-safar and MichalStrehovsky as code owners December 4, 2022 19:38

ViktorHofer removed the NO-REVIEW Experimental/testing PR, do NOT review it label Dec 4, 2022

ViktorHofer approved these changes Dec 4, 2022

View reviewed changes

UseLocalAppHostPack property

adcc6c8

agocke approved these changes Dec 5, 2022

View reviewed changes

Fix apphostsourcepath in DNNE project

8907d92

ViktorHofer changed the title ~~Update assembly version from hardcoded to MajorVersion~~ Update assembly version from hardcoded to MajorVersion and change net7.0 to net8.0 Dec 5, 2022

ViktorHofer mentioned this pull request Dec 5, 2022

Brand dotnet/runtime to NET 8 #72112

Closed

7 tasks

ViktorHofer changed the title ~~Update assembly version from hardcoded to MajorVersion and change net7.0 to net8.0~~ Update assembly version from hardcoded to MajorVersion and update NetCoreAppCurrent to net8.0 Dec 5, 2022

MichalStrehovsky reviewed Dec 5, 2022

View reviewed changes

ViktorHofer merged commit 315f4ab into dotnet:main Dec 5, 2022

ViktorHofer added a commit that referenced this pull request Dec 5, 2022

Follow-up from NetCoreAppCurrent tfm update

0e9ac69

As mentioned in #78354 (comment), this documentation refers to libraries tests and not `RunNativeAotTestApps` testing. Reverting the documentation change.

ViktorHofer mentioned this pull request Dec 5, 2022

Follow-up from NetCoreAppCurrent tfm update #79218

Merged

This was referenced Dec 5, 2022

Infra improvements for Helix #68176

Closed

Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040

Closed

stephentoub pushed a commit that referenced this pull request Dec 5, 2022

Follow-up from NetCoreAppCurrent tfm update (#79218)

9166ee4

As mentioned in #78354 (comment), this documentation refers to libraries tests and not `RunNativeAotTestApps` testing. Reverting the documentation change.

ghost locked as resolved and limited conversation to collaborators Jan 4, 2023

carlossanlop deleted the Net8ChangesSecondAttempt branch July 28, 2023 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update assembly version from hardcoded to MajorVersion and update NetCoreAppCurrent to net8.0 #78354

Update assembly version from hardcoded to MajorVersion and update NetCoreAppCurrent to net8.0 #78354

carlossanlop commented Nov 15, 2022 •

edited by ViktorHofer

Loading

ghost commented Nov 15, 2022

carlossanlop commented Nov 17, 2022

ViktorHofer left a comment

AaronRobinsonMSFT commented Dec 4, 2022

agocke commented Dec 5, 2022

ViktorHofer commented Dec 5, 2022 •

edited

Loading

ViktorHofer commented Dec 5, 2022

MichalStrehovsky commented Dec 5, 2022

MichalStrehovsky Dec 5, 2022

ViktorHofer Dec 5, 2022

Update assembly version from hardcoded to MajorVersion and update NetCoreAppCurrent to net8.0 #78354

Update assembly version from hardcoded to MajorVersion and update NetCoreAppCurrent to net8.0 #78354

Conversation

carlossanlop commented Nov 15, 2022 • edited by ViktorHofer Loading

ghost commented Nov 15, 2022

carlossanlop commented Nov 17, 2022

ViktorHofer left a comment

Choose a reason for hiding this comment

AaronRobinsonMSFT commented Dec 4, 2022

agocke commented Dec 5, 2022

ViktorHofer commented Dec 5, 2022 • edited Loading

ViktorHofer commented Dec 5, 2022

MichalStrehovsky commented Dec 5, 2022

MichalStrehovsky Dec 5, 2022

Choose a reason for hiding this comment

ViktorHofer Dec 5, 2022

Choose a reason for hiding this comment

carlossanlop commented Nov 15, 2022 •

edited by ViktorHofer

Loading

ViktorHofer commented Dec 5, 2022 •

edited

Loading