Adding linker correctness tests to test library annotations for the linker

[joperezr]

Fixes #37258

cc: @safern @ericstj @ViktorHofer @eerhardt @layomia

This will add the infrastructure as well as the first test that will make sure that annotations added to the framework for the linker will work as expected and we won't get things trimmed if we needed them.

Here are some notes on these tests:

• Each test is a *.cs file located under /tests/LinkerTests/ folder and will contain a minimal Program class with a main method.
• Each .cs file will be taken by the infrastructure and converted into a console application that will then get published and trimmed using the live-built runtime that was produced by the build output.
• The infrastructure will then execute the trimmed app, and expect to get back a 100 return code. If so, it will mark the test as successful, and an error will be logged otherwise.

Things need to get done before this PR can get merged:

• Need to add capability to work with different RIDs, as initially only win-x64 is supported.
• Need to add Helix support so that we are able to submit these tests to Helix and get results back.
• Need to add a separate pipeline that runs each PR which will handle executing these tests.
• For now we are only using live-built runtime but not live-built ref which will be handy when adding annotations to new API. For that, we depend on the work going into Create Microsoft.NetCore.App.Ref targeting pack for NetCoreAppCurrent in libraries #37600 which adds live-built ref packs cc: @ViktorHofer

[ghost]

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

[danmoseley]

Do I understand right that each linker test will need to be in its own test assembly (so they can be trimmed independently) and these will be built during the dotnet/runtime build -- could we imagine having 50 of these tests -- would this discernably slow down the build? I'm thinking of the JIT tests which are in 100's of separate assemblies and those take a huge time to build.

[joperezr]

Good question @danmosemsft let me try to explain. Yes each test will be it's own console app because as you pointed out, we want to make sure we have a minimal rooting of assemblies in order to actually test the linker is keeping what you need when the application is minimal. That said, a) we don't expect there to be 1000s of tests but instead maybe around 200 at most, given that presumably you will add a test per attribute/annotation we add in the framework which maps to the current existing IlLinkTrim.xml entries and we don't have many of those. Also b) This shouldn't slow down the regular build at all because the apps will only get build/restored/tested when running the actual linker tests (given the actual console projects get generated during test execution) which will run in a separate pipeline on CI so current runtime pipeline won't be affected. Further more, c) the matrix for these tests is very limited and we will really only require about 5 jobs for the pipeline, and tests are very fast to run, which means that this new pipeline will by no means be the long pole in our build.

[joperezr]

All that said, we do have one more concern with this which is the number of workitems in Helix will be increasing significantly. Today we have one workitem per Test assembly, meaning that for a specific vertical, we h ave around 200 or so individual work items (then of course we have to multiply by the number of verticals and number of queues) so this would be adding one workitem per console app initially. I'm trying to instead think of a model where the publish happens on the Helix machine so that we can use the same workitem to run all linker tests for a given platform

[eerhardt]

This can simply be -c Release.

[joperezr]

will fix.

[safern]

It seems like you missed to address this?

[marek-safar]

Does this setup mean that once there is a test for RID scenario it will be run with every RID we have in our build matrix?

[joperezr]

For now, we are only running the tests in windows-x64, linux-x64, and osx-x64. The plan is that as we start getting some tests that need to be covered in different RID/platform, then we will just add builds for those platforms.

[marek-safar]

None of these really matter for .NET5 so there should perhaps be different prioritization (@danmosemsft)

[joperezr]

For these tests we don't really care much about the actual runtime that the end user will have for .NET 5 as much, what we really care about is that the attributes are placed correctly such that the linker will not strip methods that you would use during execution. For this, we want to to cover places where the libraries code might use one branch of code or the other, which for our runtime libraries most of those branches are dependent on whether you are running on Windows, Linux or OSX (hence the initial set of runtimes we are testing). Once we start adding code branches that would only ever be touched in wasm, then we would definitely care about adding that to our runtimes set as well as we for sure want to validate that the attributes are placed on the right places in there as well. I'm happy to chat more about this if you want, but the high level explanation is that these tests are not as much to test the end product experience or scenario, but more of tests for our annotations correctness.

[marek-safar]

Once we start adding code branches that would only ever be touched in wasm

It's not once but that's the setup today. We build a very different version of libraries for browser today than what is CoreCLR version on desktop (starting with System.Private.CoreLib to libraries like System.Net.Http)

[joperezr]

Right, for wasm in particular we are simply not adding that with this PR because that is not ready yet, there is an ongoing PR that is adding support for running tests on wasm and once that is in, then we will add that to these sets of tests.

[joperezr]

@eerhardt @marek-safar @safern this should be now ready to go in can you take one last look? If everything looks fine I'll merge as is, and then I'll put up a quick follow up which will add instructions and documentation on how to add new, run, and maintain these tests.

[safern]

Other than some comments, LGTM.

[ViktorHofer]

Aren't you running the linker on Windows? Don't you need .exe there? At least when you quote the path it should be required.

[ViktorHofer]

Why not use DotNetTool instead? https://github.com/dotnet/arcade/blob/dcc5ca2cf397c53575d376e80ecd30f63160037f/src/Microsoft.DotNet.Arcade.Sdk/tools/RepoLayout.props#L35-L36

[joperezr]

you don't need it, in Windows when you call an application inside a folder without using the .exe, the default probing mechanism will find dotnet.exe for you.

[ViktorHofer]

nit: we prefer the attribute syntax instead of extra elements but this is a really minor nit.

[ViktorHofer]

nit: TestConsoleApp, items use the singular form.

[joperezr]

I have actually seen us use both and don't really think we have a standard, and for this case it will always be more than one, so I think I'll keep plural for now unless there is a big pushback.

[ViktorHofer]

nit: TestConsoleAppSourceFile, items use the singular form.

[ViktorHofer]

same question here, why aren't you using the msbuild task instead of an out-of-proc dotnet invocation?

[joperezr]

Here we do out of proc because we don't want any of the properties that are on the current msbuild session to propagate to the test projects, we do the same for packaging projects for the same reason. I did however took your advise and moved the Restore out-of-proc and moved it to happen in-proc on the PublishTrimmed target.

[ViktorHofer]

You can specify the props that you don't want to be propagated in the RemoveProperties attribute, also you can specify the props that you explicitly want to pass into the target via the Properties attribute. Are you sure the limitation applies?

[joperezr]

While it is true that you could RemoveProperties, global properties that we use to build runtime changes over time, so that means that even if we get this right today, it is possible in the future we set a different one that will flow down to these tests and for this ones we really want to be in as close to an isolated environment as possible. Similar to the package tests, we want to emulate users projects which won't flow down any of the state or infra that this repo uses, so I still feel that running out of proc is what we want here.

[ViktorHofer]

Can you please move the linker test files from eng/linkerTests into eng/testing/linker?

[ViktorHofer]

curious, why are you removing coreclr's S.P.Corelib?

[joperezr]

Because it was being added in both managed and native folders. I do believe that there was an underlying issue here when constructing our packs for non-mono, but that can be addressed in a separate PR.

[ViktorHofer]

Should we condition the mono binplaceitems above on RuntimeFlavor == Mono?

[joperezr]

We don't need to, as they would simply not be includded if you don't build for mono

[ViktorHofer]

Can we instead rely on EnableDefaultItems and the Compile item? Makes more sense to me instead of manually globbing the source files. cc @ercistj for opinion.

[joperezr]

The actual projects are not compiling these *.cs files, so they are not added to the project so Compile item is empty. These source files are copied into a new generated project and that is where we compile them.

[ViktorHofer]

I think this project should not just be the entry point to drive the generation, restore and execution but also define the source files. Consider using Microsoft.Build.NoTargets instead and use EnableDefaultItems to access the Compile items.

[joperezr]

That is interesting point I didn't think of that. I will leave this as is for now so that we can merge and unblock other folks from adding tests which are waiting for this, but I will explore that and change this model.

[ViktorHofer]

Why do we also run the application when we build them? I assumed dotnet build generates the projects and dotnet test or dotnet msbuild /t:Test executes them.

[joperezr]

We will in the future change this once we want to start sending tests to helix, by having the ExecuteApplications target conditioned on whether or not you use ArchiveTests=true and then instead of executing we will zip and send them over to helix. This will be addressed in a subsequent PR once we need a test runtime that doesn't work on the build machines.

[joperezr]

Merging to unblock the test work that needs to happen for #37677.