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.

Sign up for GitHub

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

Jump to bottom

Change LibrariesConfiguration defaults in tests #75033

Closed
wants to merge 3 commits into from
Closed

Change LibrariesConfiguration defaults in tests #75033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b0a192
Change LibrariesConfiguration defaults in tests
am11 Sep 2, 2022
7f6158e
Set config in CI script explicitly
am11 Sep 2, 2022
818eb65
Disambiguate from powershell's $True/$False
am11 Sep 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions eng/pipelines/coreclr/nativeaot-post-build-steps.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ parameters:
osSubgroup: ''
platform: ''
pgoType: ''
libsConfig: ''
runtimeVariant: ''
uploadTests: false
testFilter: tree nativeaot
Expand All @@ -16,16 +17,16 @@ steps:

# Build coreclr native test output
- ${{ if eq(parameters.osGroup, 'windows') }}:
- script: $(Build.SourcesDirectory)/src/tests/build.cmd nativeaot $(buildConfigUpper) ${{ parameters.archType }} ${{ parameters.testFilter }} /p:BuildNativeAotFrameworkObjects=true
- script: $(Build.SourcesDirectory)/src/tests/build.cmd nativeaot $(buildConfigUpper) ${{ parameters.archType }} ${{ parameters.testFilter }} /p:BuildNativeAotFrameworkObjects=true /p:LibrariesConfiguration=${{ parameters.libsConfig }}
displayName: Build tests
- ${{ if ne(parameters.osGroup, 'windows') }}:
- script: $(Build.SourcesDirectory)/src/tests/build.sh nativeaot $(buildConfigUpper) ${{ parameters.archType }} '${{ parameters.testFilter }}'
- script: $(Build.SourcesDirectory)/src/tests/build.sh nativeaot $(buildConfigUpper) ${{ parameters.archType }} '${{ parameters.testFilter }}' -p:LibrariesConfiguration=${{ parameters.libsConfig }}
displayName: Build tests

- ${{ if eq(parameters.runSingleFileTests, true) }}:
- ${{ if eq(parameters.osGroup, 'windows') }}:
- script: $(Build.SourcesDirectory)/src/tests/run.cmd runnativeaottests $(buildConfigUpper) ${{ parameters.archType }}
- script: $(Build.SourcesDirectory)/src/tests/run.cmd runnativeaottests $(buildConfigUpper) ${{ parameters.archType }} /p:LibrariesConfiguration=${{ parameters.libsConfig }}
displayName: Run tests in single file mode
- ${{ if ne(parameters.osGroup, 'windows') }}:
- script: $(Build.SourcesDirectory)/src/tests/run.sh --runnativeaottests $(buildConfigUpper) ${{ parameters.archType }}
- script: $(Build.SourcesDirectory)/src/tests/run.sh --runnativeaottests $(buildConfigUpper) ${{ parameters.archType }} -p:LibrariesConfiguration=${{ parameters.libsConfig }}
displayName: Run tests in single file mode
6 changes: 6 additions & 0 deletions eng/pipelines/runtime.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ jobs:
nameSuffix: NativeAOT
buildArgs: -s clr.aot+libs -rc $(_BuildConfig) -lc Release
extraStepsTemplate: /eng/pipelines/coreclr/nativeaot-post-build-steps.yml
extraStepsParameters:
libsConfig: Release
condition: >-
or(
eq(dependencies.evaluate_paths.outputs['SetPathVars_coreclr.containsChange'], true),
Expand All @@ -221,6 +223,8 @@ jobs:
nameSuffix: NativeAOT
buildArgs: -s clr.aot+libs -rc $(_BuildConfig) -lc Release
extraStepsTemplate: /eng/pipelines/coreclr/nativeaot-post-build-steps.yml
extraStepsParameters:
libsConfig: Release
condition: >-
or(
eq(dependencies.evaluate_paths.outputs['SetPathVars_coreclr.containsChange'], true),
Expand All @@ -245,6 +249,8 @@ jobs:
nameSuffix: NativeAOT
buildArgs: -s clr.aot+libs -rc $(_BuildConfig) -lc Release
extraStepsTemplate: /eng/pipelines/coreclr/nativeaot-post-build-steps.yml
extraStepsParameters:
libsConfig: Release
condition: >-
or(
eq(dependencies.evaluate_paths.outputs['SetPathVars_coreclr.containsChange'], true),
Expand Down
2 changes: 1 addition & 1 deletion src/tests/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<LibrariesConfiguration>Release</LibrariesConfiguration>
Copy link
Member

Choose a reason for hiding this comment

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

We explicitly chose to use release libraries here as we generally want to use release libraries in these tests in local builds. I’m not sure if we want to change this default.
cc: @trylek @hoyosjs @jkotas

Copy link
Member Author

Choose a reason for hiding this comment

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

This is aligning the behavior of product and test build scripts:

Before:

./build.sh -c Debug => src/tests/build.sh Debug -p:LibrariesConfiguration=Release
./build.sh -c Release => src/tests/build.sh Release
./build.sh -c Debug -lc Release => src/tests/build.sh Debug

After:

./build.sh -c Debug => src/tests/build.sh Debug
./build.sh -c Release => src/tests/build.sh Release
./build.sh -c Debug -lc Release => src/tests/build.sh Debug -p:LibrariesConfiguration=Release

IMO, it is easier to reason about instead of remembering this one-off rule.

Additionally, we can also add a shorthand -lc option in test build script.

Copy link
Member

Choose a reason for hiding this comment

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

The current default is documented in number of places (e.g. https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/coreclr/windows-test-instructions.md#building-tests).

Local workflow of number of engineers on the team depend on the current default. -p:LibrariesConfiguration=Release is very long to type for the most common use case. This would need shorthand, like -lc shorthand used for the main build script.

Would it make sense to change the runtime to pick the last flavor of libraries by default, similar to how libraries tests pick up the last flavor of runtime by default?

Copy link
Member

@ViktorHofer ViktorHofer Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is already a shorthand for libraries configuration (lc).

Would it make sense to change the runtime to pick the last flavor of libraries by default, similar to how libraries tests pick up the last flavor of runtime by default?

Correct me if I'm wrong but I don't think that libraries pick-up the last built runtime. Instead they use the passed in runtime configuration which defaults to Debug.

Additionally, when building clr and libs we always need to explicitly pass in -rc Release. It might make more sense to default to Release and only pick Debug/Checked if Release doesn't exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is already a shorthand for libraries configuration (lc).

lc is available in eng/build, but not in src/tests/build scripts.

The reason why folks use src/tests/build scripts directly more often than other internal scripts (src/coreclr/build-runtime, src/native/libs/build-native and src/native/corehost/build) is because eng/build is not wired to tests build script. Consequently, we have the ability to build sources and libraries tests in one step, e.g. build clr+libs -test, but for runtime tests, it is always a two-step process, with different argument syntax (-c {Config} vs. -{Config}) and defaults.

Copy link
Member

@jkotas jkotas Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that libraries pick-up the last built runtime. Instead they use the passed in runtime configuration which defaults to Debug.

The comment was about libraries tests. When you are building and running libraries tests (iterating on a single test, without building the runtime and libraries), they will pick up the runtime flavor that you have built last.

when building clr and libs we always need to explicitly pass in -rc Release

Depends on which component you are working on. If you are working on runtime, you typically pass in -lc Release.

It might make more sense to default to Release and only pick Debug/Checked if Release doesn't exist.

Do you mean default to Release everywhere and let people override when they want a Debug flavor of a specific component?

Copy link
Member

@ViktorHofer ViktorHofer Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean default to Release everywhere and let people override when they want a Debug flavor of a specific component?

I care less about defaulting the libraries' configuration to Release as I never build/run the runtime or the installer/host tests but when iterating on libraries themselves and running tests, we (the team) usually pass in -rc Release / /p:RuntimeConfiguration=Release (to the root build).

The comment was about libraries tests. When you are building and running libraries tests (iterating on a single test, without building the runtime and libraries), they will pick up the runtime flavor that you have built last.

We probably mean the same thing but just to be accurate: When running framework dependent tests, the host and runtime is used that is part of in the artifacts/bin/testhost folder. The testhost folder is populated by the "libs.pretest" subset / pretest.proj msbuild project which runs when the subset is explicitly invoked or as part of the parent subset. "libs". The runtime and host bits are determined by the provided RuntimeConfiguration and HostConfiguration msbuild properties, and both default to Debug.

In addition to that, the testhost folder itself also encodes the configuration in its path which means when building libraries tests, the corresponding testhost folder is used (either Debug or Release).

For self-contained deployment, the story is entirely different and I believe that the RuntimeConfiguration flag is honored just-in-time as part of the library test project's build/test.

Copy link
Member

Choose a reason for hiding this comment

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

but when iterating on libraries themselves and running tests, we (the team) usually pass in -rc Release / /p:RuntimeConfiguration=Release.

Not for each invocation. I build once from root with e.g.:

build clr+libs -rc release

and then when iterating on an individual library I just do:

dotnet build /t:test

without explicitly specifying /p:RuntimeConfiguration=Release.

Copy link
Member

Choose a reason for hiding this comment

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

Right. That's also the documented Quick Start workflow: https://github.com/dotnet/runtime/tree/main/docs/workflow/building/libraries#quick-start

Copy link
Member

Choose a reason for hiding this comment

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

Correct, that was an overlook from my side. Thanks for clarifying that.

<LibrariesConfiguration Condition="'$(LibrariesConfiguration)' == ''">$(Configuration)</LibrariesConfiguration>
</PropertyGroup>

<Import Project="$(MSBuildThisFileDirectory)\Common\dir.sdkbuild.props" Condition="'$(UsingMicrosoftNETSdk)' == 'true'" />
Expand Down