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

Update (remove most) most of CLRTestKind, GenerateRunScript, RestorePackages #89696

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

markples
Copy link
Member

@markples markples commented Jul 30, 2023

I came across these while looking at the set of properties in order to generate some error checking for merged groups. As best as I can tell, most of these are very old uses that likely have had the test system changed from under them. They cause confusion when encountered.

  • RestorePackages - I think has been replaced by the logic that bulk restores.
  • CLRTestKind - A (somewhat) more recent change was Switch over Loader/classloader/TypeGeneratorTests to use merged wrappers #61942, which changed the default for CLRTestKind from "BuildAndRun" to "BuildAndRun if exe, else BuildOnly". However, BuildOnly appears to be intended for exes - build the exe but not test infrastructure to execute it as another project will handle that. I think SharedLibrary is what was intended here. Many OutputType=library tests specify BuildOnly or SharedLibrary, which seem incorrect and (now) unnecessary.
  • GenerateRunScript - This is unconditionally set (overriding individual project files) in src/tests/Directory.Build.targets.

@ghost
Copy link

ghost commented Jul 30, 2023

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

Issue Details

null

Author: markples
Assignees: markples
Labels:

area-System.Reflection.Metadata

Milestone: -

@markples markples changed the title redo props Update (remove most) most of CLRTestKind, GenerateRunScript, RestorePackages Jul 31, 2023
@markples
Copy link
Member Author

@trylek @jkoritzinsky This is fallout from staring at msbuild properties too long for merged groups.

cc @dotnet/jit-contrib @Maoni0 @cshung @mrsharm

I'm a bit concerned that the checkin gate doesn't provide complete coverage so would welcome additional tests to run.

@markples markples marked this pull request as ready for review July 31, 2023 07:27
@markples
Copy link
Member Author

/azp run runtime-coreclr outerloop, runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@markples
Copy link
Member Author

One little static check on the contents of the diff:

C:\r\runtime4\src\tests>git diff dotnet/main prop_update | findstr "^+ ^-" | findstr /v "^+++ ^---" | sort | uniq
-
-    <CLRTestKind Condition="'$(CLRTestKind)' == ''">BuildOnly</CLRTestKind>
-    <CLRTestKind>BuildAndRun</CLRTestKind>
-    <CLRTestKind>BuildOnly</CLRTestKind>
-    <CLRTestKind>SharedLibrary</CLRTestKind>
-    <GenerateRunScript>false</GenerateRunScript>
-    <RestorePackages>false</RestorePackages>
-    <RestorePackages>true</RestorePackages>
-  </PropertyGroup>
-  <PropertyGroup>
+    <CLRTestKind Condition="'$(CLRTestKind)' == ''">SharedLibrary</CLRTestKind>

The conditional updates are the change to the default in Directory.Build.targets. The PropertyGroup removals are intended (not unchecked) removals of (now) empty groups.

@ghost
Copy link

ghost commented Jul 31, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

I came across these while looking at the set of properties in order to generate some error checking for merged groups. As best as I can tell, most of these are very old uses that likely have had the test system changed from under them. They cause confusion when encountered.

  • RestorePackages - I think has been replaced by the logic that bulk restores.
  • CLRTestKind - A (somewhat) more recent change was Switch over Loader/classloader/TypeGeneratorTests to use merged wrappers #61942, which changed the default for CLRTestKind from "BuildAndRun" to "BuildAndRun if exe, else BuildOnly". However, BuildOnly appears to be intended for exes - build the exe but not test infrastructure to execute it as another project will handle that. I think SharedLibrary is what was intended here. Many OutputType=library tests specify BuildOnly or SharedLibrary, which seem incorrect and (now) unnecessary.
  • GenerateRunScript - This is unconditionally set (overriding individual project files) in src/tests/Directory.Build.targets.
Author: markples
Assignees: markples
Labels:

area-CodeGen-coreclr

Milestone: -

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mark for cleaning this up!

@trylek
Copy link
Member

trylek commented Aug 21, 2023

Based on offline discussion with Mark I have rebased this change against latest main and I plan to retest it and merge it in once it passes as the start of my quality week activities as Mark has fully switched over to GC work.

@trylek
Copy link
Member

trylek commented Aug 21, 2023

/azp run runtime-extra-platforms

@trylek
Copy link
Member

trylek commented Aug 21, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@trylek
Copy link
Member

trylek commented Aug 22, 2023

Runtime and outerloop runs are clean and for the runtime-extra-platforms I haven't found any issues different from a previous run, merging in.

@MichalStrehovsky
Copy link
Member

We stopped running the SharedLibrary test after this change.

I'm putting back the ClrTestKind for this test in #91356 but I do wonder if there's any other test we stopped running by dropping ClrTestKind.

@markples
Copy link
Member Author

This one happened because the test was marked OutputType==library and ClrTestKind=BuildAndRun. However, it looks like it depends on a native entry point (and CLRTestBatchPreCommands, etc.), so the managed part of the test is "library" but the overall project is an executable. I just did a scan of all proj files in that commit that removed BuildAndRun, and SharedLibrary is the only one of the ~30 that doesn't have OutputType==exe.

@markples markples deleted the prop_update branch September 28, 2023 23:34
@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants