Changing the TestPath to have more info built in to avoid dir clashes#12634
Conversation
| <TargetGroup Condition="'$(TargetGroup)'=='' and $(Configuration.Contains('uap101aot'))">uap101aot</TargetGroup> | ||
| <TargetGroup Condition="'$(TargetGroup)'=='' and $(Configuration.Contains('uap101'))">uap101</TargetGroup> | ||
|
|
||
| <ProjectRunConfig Condition="'$(TargetGroup)'!=''">$(OSGroup).$(Platform).$(TargetGroup).$(ConfigurationGroup).$(TestTFM)</ProjectRunConfig> |
There was a problem hiding this comment.
you could simply not special case it and if targetgroup was "" it would just show up as double dots.
There was a problem hiding this comment.
Yeah that's how I had it on my first iteration but I didn't like the double "." I think that might lead to people thinking something is not set correctly.
There was a problem hiding this comment.
+1 to being against double dots. It will always trigger the "is that a bug?" response for me.
| <IntermediateOutputPath Condition="'$(IntermediateOutputPath)' == ''">$(IntermediateOutputRootPath)$(MSBuildProjectName)/$(TargetOutputRelPath)</IntermediateOutputPath> | ||
|
|
||
| <TestPath Condition="'$(TestPath)'==''">$(TestWorkingDir)$(OSPlatformConfig)/$(MSBuildProjectName)/$(TargetOutputRelPath)</TestPath> | ||
| <TestPath Condition="'$(TestPath)'==''">$(TestWorkingDir)$(ProjectRunConfig)/$(MSBuildProjectName)/$(TargetOutputRelPath)</TestPath> |
There was a problem hiding this comment.
Does this take care of where tests will publish to or does this also require consumption of ProjectRunConfig by BuildTools?
There was a problem hiding this comment.
It does, publishtest.targets consume $(TestPath) so no need to make a change there.
|
LGTM, other than I had imagined the ProjectRunConfig string might be defined in buildtools. |
This was my initial plan, but the problem is that buildtools properties are imported before OSGroup, TargetGroup and some other properties get defined, which themselves are kind of tied to the place they are now because they depend on other stuff to be defined as well. Basically it could probably be done in buildtools, but it would be very hard to figure out the ordering of dir.props right, in order to not break any other property. |
|
Ah. In that case, perhaps we should eventually have a concept of "some .props / .targets that is always pulled in first if present" to allow BuildTools to trump properties if it has reason. Outside of the scope of this work. It's definitely worth running a manual helix test build against packages with this once to make sure all the archives are produced correctly, but if you're feeling bold a clean CI is fine too. |
| <IntermediateOutputPath Condition="'$(IntermediateOutputPath)' == ''">$(IntermediateOutputRootPath)$(MSBuildProjectName)/$(TargetOutputRelPath)</IntermediateOutputPath> | ||
|
|
||
| <TestPath Condition="'$(TestPath)'==''">$(TestWorkingDir)$(OSPlatformConfig)/$(MSBuildProjectName)/$(TargetOutputRelPath)</TestPath> | ||
| <TestPath Condition="'$(TestPath)'==''">$(TestWorkingDir)$(ProjectRunConfig)/$(MSBuildProjectName)/$(TargetOutputRelPath)</TestPath> |
There was a problem hiding this comment.
For now I would just update TargetOutputRelPath (or another property) to also include TestTFM. Once we move these to buildtools we can start having properties trying to combine the properties in logical groups that people can depend on.
There was a problem hiding this comment.
I'll go with this approach instead. I will however use a different property, because TargetOutputRelPath is used in some other places that would break things if I change this one directly.
joperezr
force-pushed
the
FixTestBuilds
branch
from
0097f17 to
f0b8a93
Compare
Changing the TestPath to have more info built in to avoid dir clashes Commit migrated from dotnet/corefx@dba738f
6 participants
Progress towards dotnet/buildtools#1121
cc: @weshaggard @MattGal @karajas @danmosemsft
With this change, I'm introducing this common build property that represents all aspects of the build configuration of a project, and I'm using it to set
$(TestPath)which was using$(OSPlatformConfig)before and that was causing that we had some folder clashes when we introduced test builds that hadTargetGroup=netcoreapp1.1. This is just the start of a batch of changes(some of which will have to go in buildtools) that will basically replaceOSPlatformConfigwith this new variable. This change will fix the current build test issues where we fail while trying to zip folderAat the same time that a subfolderBis being modified by a different build.