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

Fixes for BuildAsStandalone in tests #84374

Merged
merged 5 commits into from
Apr 6, 2023
Merged

Conversation

markples
Copy link
Member

@markples markples commented Apr 5, 2023

This should fix test builds with set BuildAsStandalone=true. Viewing the individual commits will help because the first one touches a lot of files.

  • Remove unconditional <BuildAsStandalone>false</BuildAsStandalone> from various HardwareIntrinsics tests. This is already the default via Directory.Build.props but interferes with set BuildAsStandalone=true. (nit - also remove an empty PropertyGroup from GitHub_43569.csproj)
  • Remove Main methods. We might be able to pursue a strategy where the test author provides a Main(args) method for local custom execution. The test would either need a separate parameterless [Fact] method or a [Theory] plus data for Main for merged group to execute it. However, a RequiresProcessIsolation/BuildAsStandalone build would ignore that metadata, so I'd like to avoid that complexity. The Main methods here set a bool or parse an int, which can be added locally to any test as needed.
  • Add comment to Directory.Merged.props (addressing feedback on [release/7.0] JIT: fix bug in cloning conditions for jagged array (#83414) #83462)
  • Add the Methodical test back into the superpmi test using RequiresProcessIsolation

Admittedly, I still do not understand fully how RequiresProcessIsolation and BuildAsStandalone work. Both effectively do the same thing (use the .cmd route for a test), but they are implemented differently. RequiresProcessIsolation is valid in test project files and isn't used elsewhere. BuildAsStandalone is set in some test infrastructure projects but does not work in specific test project files; it does work in an environment variable for tests. A lot of the complexity is that we don't want boilerplate in individual test project files, but Directory.Build.props runs very early (before per-project settings) and Directory.Build.targets runs very late (after SDK .targets files). Therefore we don't have a good place for default logic that consumes test-specific values.

I'm definitely open to ideas. One would be to add an Import at the end of every test project file. At least it would only be one line (and could replace OutputType=Library in the small set that does that), and perhaps it could simplify some of the logic in the D.B.props and .targets files but I'm not sure.

Fixes #76421
Fixes #81984

@ghost
Copy link

ghost commented Apr 5, 2023

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

Issue Details

This should fix test builds with set BuildAsStandalone=true. Viewing the individual commits will help because the first one touches a lot of files.

  • Remove unconditional <BuildAsStandalone>false</BuildAsStandalone> from various HardwareIntrinsics tests. This is already the default via Directory.Build.props but interferes with set BuildAsStandalone=true. (nit - also remove an empty PropertyGroup from GitHub_43569.csproj)
  • Remove Main methods. We might be able to pursue a strategy where the test author provides a Main(args) method for local custom execution. The test would either need a separate parameterless [Fact] method or a [Theory] plus data for Main for merged group to execute it. However, a RequiresProcessIsolation/BuildAsStandalone build would ignore that metadata, so I'd like to avoid that complexity. The Main methods here set a bool or parse an int, which can be added locally to any test as needed.
  • Add comment to Directory.Merged.props (addressing feedback on [release/7.0] JIT: fix bug in cloning conditions for jagged array (#83414) #83462)
  • Add the Methodical test back into the superpmi test using RequiresProcessIsolation

Admittedly, I still do not understand fully how RequiresProcessIsolation and BuildAsStandalone work. Both effectively do the same thing (use the .cmd route for a test), but they are implemented differently. RequiresProcessIsolation is valid in test project files and isn't used elsewhere. BuildAsStandalone is set in some test infrastructure projects but does not work in specific test project files; it does work in an environment variable for tests. A lot of the complexity is that we don't want boilerplate in individual test project files, but Directory.Build.props runs very early (before per-project settings) and Directory.Build.targets runs very late (after SDK .targets files). Therefore we don't have a good place for default logic that consumes test-specific values.

I'm definitely open to ideas. One would be to add an Import at the end of every test project file. At least it would only be one line (and could replace OutputType=Library in the small set that does that), and perhaps it could simplify some of the logic in the D.B.props and .targets files but I'm not sure.

Fixes #76421
Fixes #81984

Author: markples
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

@markples
Copy link
Member Author

markples commented Apr 5, 2023

PTAL @trylek @jkoritzinsky - especially if you have comments about D.B.props/targets and how to structure the build files

cc @BruceForstall @dotnet/jit-contrib

@markples markples added this to the 8.0.0 milestone Apr 5, 2023
@markples markples added test-bug Problem in test source code (most likely) test-enhancement Improvements of test source code labels Apr 5, 2023
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.

Looks great to me, thanks Mark for all these cleanups and fixes! I'll spend some more time thinking about your suggestions for further cleanups regarding BuildAsStandalone and related, I'm certainly all for simplifying the test build logic that is already overcomplicated.

@markples
Copy link
Member Author

markples commented Apr 5, 2023

/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

markples commented Apr 5, 2023

Need to rerun extra-platforms after #84385 and outerloop after #84382

@markples
Copy link
Member Author

markples commented Apr 6, 2023

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@akoeplinger
Copy link
Member

The enabledisable test failure on Android will be fixed by #84409

@markples
Copy link
Member Author

markples commented Apr 6, 2023

outerloop green, extra-platforms looks consistent with other jobs (this change is not as disruptive at things like test merging, so I wasn't expecting a difference across platforms)

@markples markples merged commit 01567c6 into dotnet:main Apr 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr test-bug Problem in test source code (most likely) test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to build pri-1 tests superpmi unit test dependencies build issue
3 participants