Change LibrariesConfiguration defaults in tests #75033
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsWe can pass Line 131 in 01251a8 e.g. fails without explicitly specifying
|
| @@ -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> | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right. That's also the documented Quick Start workflow: https://github.com/dotnet/runtime/tree/main/docs/workflow/building/libraries#quick-start
There was a problem hiding this comment.
Correct, that was an overlook from my side. Thanks for clarifying that.
ViktorHofer
added
area-Infrastructure-coreclr
and removed
area-Infrastructure-libraries
labels
|
Tagging subscribers to this area: @hoyosjs Issue DetailsWe can pass Line 131 in 01251a8 e.g. fails without explicitly specifying
|
|
Where are we with this PR? Do we want to take this change, or did we decide this isn't the direction we wanted to go. |
|
I do not think we want to take the change of the defaults. The current defaults work better for local workflows. |
|
OK, let's close this for now and we can take more targeted changes as appropriate for different workflows. |
We can pass
-p:LibrariesConfiguration=to set library configuration of our choice, but setting default to$(Configuration)is better; same as:runtime/Directory.Build.props
Line 131 in 01251a8
e.g.
fails without explicitly specifying
-p:LibrariesConfiguration=Debug.