Skip to content
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

[Quality Week] Improve quality of Crossgen2 testing, most notably in context of library tests #74681

Closed
trylek opened this issue Aug 26, 2022 · 26 comments
Assignees
Milestone

Comments

@trylek
Copy link
Member

trylek commented Aug 26, 2022

Over the course of 2 years since we shipped the first preview of Crossgen2 we have identified several testing gaps that bit us in various phases of development. We would like to at least create an action plan how to address them and ideally implement at least part of the work in context of the Core Runtime team Quality Weeks.

The problem

  • Library testing in the runtime repo doesn't target Crossgen2 at all, neither for compiling the framework nor the individual test apps.

  • While CoreCLR does implement Crossgen2 testing, it does so using a raw freshly built Crossgen2, not the NativeAOT-compiled package we ship to customers. Ideally we should test what customers are using both in CoreCLR and in library testing.

  • Implementing support for Crossgen2 in the library tests would let us expand testing in directions that weren't previously possible - e.g. testing of the new feature of "speculative" cross-version inlining - that would have very limited coverage in many of the tiny runtime tests.

Overall approach

To address these issues, we're broadly thinking about the following:

  • Somehow restructuring the runtime build so that both CoreCLR and library testing can leverage the "final" Crossgen2 produced as part of packs.product.

  • Modifying src\tests\build.proj to publish the customer Crossgen2 to CORE_ROOT for the purpose of CoreCLR testing.

  • Modifying library build scripts to provide more flexibility regarding framework and / or test compilation using Crossgen2 and passing additional flags to Crossgen2.

Open issues

  • The proposed build restructuring goes largely against the current build pipeline (build clr + libs, then the packs). We need to figure out something that doesn't place additional burden on runtime developers (too many new switches or steps) and doesn't substantially regress local innerloop (runtime build perf)

  • At this point it's unclear to me whether library testing with Crossgenned framework should also just grab the pack-produced framework package or basically duplicate the step (perhaps by just calling into the already existing SDK target).

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 26, 2022
@trylek trylek removed the untriaged New issue has not been triaged by the area owner label Aug 26, 2022
@trylek
Copy link
Member Author

trylek commented Aug 26, 2022

/cc @dotnet/crossgen-contrib, @davidwrighton, @AntonLapounov, @jkotas, @jkoritzinsky, @ViktorHofer, @danmoseley, @stephentoub, @agocke. Just for visibility I'm also /cc-ing @SamMonoRT even though at this point I'm not aware of any particular Mono impact of this work.

@trylek
Copy link
Member Author

trylek commented Aug 26, 2022

Related to (precursor issue): #50127

@MichalStrehovsky
Copy link
Member

Cc @LakshanF since moving where we AOT compile crossgen2 is similar to AOT compiling ILC that Lakshan has been looking at.

@davidwrighton
Copy link
Member

I think we may be able to have a relatively minor variation to our test build and setup.

In particular, I think the following approaches may be viable:

  1. We can probably use the Crossgen2 outerloop job to run these new test variations instead of modifying the normal test legs.
  2. If we build the product.pack build, we'll have both the full Runtime+libraries compiled via Crossgen2 available as well as the final Crossgen2 product that we ship to customers.
  3. When constructing the Libraries helix bits, it produces a "faux" product release to run the tests. We should be able to simply drop the final product instead into the Libraries tests.
  4. Similarly, we should be able to drop a real crossgen2.exe into the CoreCLR tests, and convince our tests to use that.

I do not yet have a strategy for how to run crossgen2 on the actual libraries tests. I think we may just want to run crossgen2 directly before sending to helix.
5.

@trylek
Copy link
Member Author

trylek commented Aug 29, 2022

Thanks David, that is an interesting direction - while using the packs-produced Crossgen2 (and framework) for local CoreCLR / libraries testing may be challenging due to the build ordering issues, for lab testing it's a no-concern as we have all the dependencies readily available.

On the other hand, generally speaking we should strive to keep most lab chores reproducible locally for the purpose of bug investigation i.o.w. once we start running crossgenned library tests in the lab, we should have at least a non-trivial recipe for doing the same thing locally.

By non-trivial I mean that e.g. developers will newly need to build the packs along with clr+libs prior to running tests to make this work, otherwise in 99% of cases running the "raw" Crossgen2 should yield the same result as using the "nuget"-one and for normal developer innerloop that should be typically sufficient.

@jkotas
Copy link
Member

jkotas commented Aug 29, 2022

I agree that one needs to be able to replicate the lab testing locally. It does not need to be as efficient as dev inner loop, it is fine to require building the whole repo for that.

In general, I think we should be making stronger distinction between dev inner loop where a dev iterates on a specific component or a specific test, and lab testing where we want to run all tests as efficiently as possible in number of configuration, including configurations that are identical to installed product.

@davidwrighton
Copy link
Member

Yes. For instance, we could easily have another flag to the build script for the src\tests\build.cmd which could use the crossgen2 from the products pack. something like src\tests\build.cmd generatelayoutonly use_final_product_crossgen2 and that would simply pull the crossgen2 from the actual product build. Or possibly we could just have an environment variable which does the redirection. So the developer would need to know where the crossgen2 was produced, and then set something like set crossgen2_exec_command=c:\git\runtime\artifacts\path to crossgen2.exe

@trylek
Copy link
Member Author

trylek commented Aug 29, 2022

Adding a new option to src\tests\build.cmd/sh for using the "nuget" Crossgen2 sounds great to me and should be pretty easy to implement including some warning diagnostics in case the pack is not available ("You need to build packs.product prior to using the nuget-crossgen2 option"). The environment variable will ultimately report the same via a "file not found" exception but direct support in the test build script can presumably provide better developer-facing diagnostics.

@davidwrighton
Copy link
Member

The bigger question is about running the libraries tests with official bits, and crossgening the libraries tests themselves. @danmoseley, do you have someone who we could talk to about various options around the libraries tests?

@trylek
Copy link
Member Author

trylek commented Aug 29, 2022

While I made the speculative syntax nuget-crossgen2 up, it's actually an interesting aspect as we could use the same or similar syntax to make CoreCLR test build grab the "nuget" version of the framework (per-assembly or composite). While this may sound nitpicky, CoreCLR test build uses the very non-standard internal app R2RTest to compile the framework (if it sounds like I'm trying to troll R2RTest author let me remind you it is myself) that may differ in many aspects from the way we're compiling the nuget framework. Naturally all of this holds for runtime testing that could be used as a first testbed for exercising the nuget Crossgen2 and framework, for library testing we'll need to massage the scripts much more.

@trylek
Copy link
Member Author

trylek commented Aug 29, 2022

In a way it's an interesting question whether, if we decide to add support for using the nuget framework for runtime testing, it makes any sense to keep the pre-existing logic of custom crossgenning of the framework using R2RTest - my gut feeling is that two different versions of R2R framework would do more harm than good and we should just use the one we ultimately ship.

@trylek
Copy link
Member Author

trylek commented Aug 29, 2022

And David's question for Dan made me realize that we can already start actually building up an ordering of tasks:

  1. Add options to src/tests/build.cmd/sh/proj to use nuget Crossgen2 / nuget framework instead of using raw Crossgen2 and manually compiled framework. Switch over all Crossgen2 pipelines to use this new logic. This seems well understood, I would estimate it at 1/2-1 workweek and in my opinion @ivdiazsa would be a great person to implement that based on his past experience but I certainly don't insist.

  2. Add options to library tests to use the nuget framework instead of the raw one (per-assembly or composite). Switch [at least some] library test runs to use this option. This is less understood in terms of actual script changes but still relatively well defined. I would expect this to take 1/2-1 workweek for someone familiar with library test build.

  3. Add options to library tests to use the nuget Crossgen2 to compile the test apps themselves. Switch [at least some] library test runs to use this option. This is hand-wavy but hopefully doable in something like a workweek by someone familiar with library test build assisted by a Core Runtime team member familiar with Crossgen2.

@danmoseley
Copy link
Member

@ViktorHofer is a great person to include for discussion of libraries test execution.

@MichalStrehovsky
Copy link
Member

The bigger question is about running the libraries tests with official bits, and crossgening the libraries tests themselves. @danmoseley, do you have someone who we could talk to about various options around the libraries tests?

When single file testing was added for the libraries, the approach that was chosen was to do the end user flow: inject <PublishSingleFile> into the project and publish the test. We do the same thing for <PublishAot>.

Of course one needs to mess with thing a little bit to make it use the live built bits, but going through the E2E workflow provides additional testing for the build targets, constructing the command line to invoke the compiler, etc.

@davidwrighton
Copy link
Member

@MichalStrehovsky that sounds good. Can you point me at the logic for doing that, or the PR which introduced that testing?

@mangod9 mangod9 added this to the 8.0.0 milestone Sep 2, 2022
@MichalStrehovsky
Copy link
Member

@MichalStrehovsky that sounds good. Can you point me at the logic for doing that, or the PR which introduced that testing?

The meat is in the tests.singlefile.targets file I linked, but the PRs are #42972 for single file testing and #63781 for NativeAOT testing.

@ViktorHofer
Copy link
Member

@MichalStrehovsky is talking about self-contained testing which is the more prominent mode these days. Back in the 2.0 days, it was mostly framework dependent testing as features that made self-contained deployment more useful link the linker integration, single-file, crossgen2, nativeAOT, etc. didn't exist. Our testhost folder is a fake representation to make testing framework dependent apps possible.

With the rising popularity of self-contained deployment modes, I think that we should prefer self-contained over framework-dependent for new workloads like this one.

Some thoughts from my side:

  • As Michal already expressed, try to inject this new workload into the common path that we expose to our customers (i.e. flipping a property when building/testing the test projects). While it's easy to add new infrastructure, it takes a lot of time to replace it in favor of common, already publicly exposed paths.
  • Keep incremental builds in mind and make sure that inputs and outputs are defined for custom targets. Try to avoid binclashes by not writing to the same location twice (i.e. once with the crossgen2 flag enabled and once without it). You could mutate the test app's output path if crossgen2 is enabled.

If you need any further help from my side please let me know. Ideally this is just about flipping the already exposed crossgen2 switch and making sure that tests are submitted in self-contained deployment mode to helix.

@davidwrighton
Copy link
Member

#75230 contributes the basic ability to run the library tests under R2R, we'll need some follow on to enable testing in the lab. We may also need to implement a selection of various crossgen2 modes (composite, cross-module inlining enabled, etc)

@ivdiazsa
Copy link
Member

ivdiazsa commented Aug 7, 2023

This is a work item that will certainly improve our Crossgen2 testing coverage. It's been almost a year though since this issue in particular was touched and commented on. So, I would like to ask for an overall rundown of the current status of things, so I can get an idea on where I need to start :)

@trylek trylek modified the milestones: 8.0.0, 9.0.0 Aug 8, 2023
@trylek
Copy link
Member Author

trylek commented Aug 8, 2023

Moving the milestone to .NET 9 so that it doesn't appear to be blocking RC1 fork, infra work is generally orthogonal to shipping schedule.

@ivdiazsa
Copy link
Member

ivdiazsa commented Aug 17, 2023

I just reread through all the conversation in this thread and I think we're still mostly following the design proposed initially. Here's the plan of action I've devised and would like to share and hear everyone's thoughts.

  • Work Item 1 (Get the Libraries R2R Test Runs Working in the Lab): Currently tracked in [Corrected] - Enable Libraries R2R Testing in CI #89405. Still investigating how to get the libraries test build (when passing the -p:TestReadyToRun=true flag) to use the Crossgen2 dll with the repo's downloaded dotnet, rather than relying on the executable that needs .NET installed on the machines.

  • Work Item 2 (Add option to use customer Crossgen2 build): Add the flag option to tell src/tests/build.sh to use the Crossgen2 live product version, rather than the artifacts/bin built one. This will also require some adjustments in the test build MSBuild files to actually pick said live Crossgen2 when said option is specified, as well as the appropriate error handling mechanism when said subset hasn't been built.

  • Work Item 3 (Add the functionality to use customer Crossgen2 for Libraries R2R Test Runs): Mirror version of Work Item 1 to use the live Crossgen2 build instead. We'd probably need to add a new MSBuild flag for this purpose (similarly to how TestReadyToRun was added originally). Hopefully, by leveraging the results of Work Item 1, this functionality will be less complicated to implement than it currently sounds.

@trylek @mangod9

@jkotas
Copy link
Member

jkotas commented Aug 18, 2023

use the Crossgen2 dll with the repo's downloaded dotnet

We publish crossgen2 as a self-contained app during the build that is native AOT compiled where supported. Why can't you use that? It should be faster and it would have better fidelity with how customers run crossgen.

@ivdiazsa
Copy link
Member

use the Crossgen2 dll with the repo's downloaded dotnet

We publish crossgen2 as a self-contained app during the build that is native AOT compiled where supported. Why can't you use that? It should be faster and it would have better fidelity with how customers run crossgen.

Can you explain where this self-contained crossgen2 is and how do we build it?

@jkotas
Copy link
Member

jkotas commented Aug 19, 2023

It is built by publish target in src/coreclr/tools/aot/crossgen2/crossgen2.csproj project. It gets built to artifacts\bin\coreclr\windows.x64.Release\crossgen2\win-x64\publish. In regular build, the publish target is invoked from under src/installer.

@trylek
Copy link
Member Author

trylek commented Jan 22, 2024

@ivdiazsa - am I right to believe that your PR introducing library testing using Crossgen2 has been merged in and this issue can be closed? Please correct me if I'm wrong. As Crossgen2 is moving under your new team led by Steve Pfister, I'm trying to slightly simplify matters for him by going over the Crossgen2 backlog and closing obsolete issues.

@ivdiazsa
Copy link
Member

@trylek Yes I think we can close this issue now. All related stuff has been either merged or is being tracked elsewhere in more highlighted places

@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants