Actually use TargetOS=AnyOS in the build-test-job. #61745
Conversation
Match the name that's used in the job title and enable us to use TargetOS to pre-filter OS-specific tests with the new Xunit test wrapper generator.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsMatch the name that's used in the job title and enable us to use TargetOS to pre-filter OS-specific tests with the new Xunit test wrapper generator.
|
| @@ -147,7 +147,7 @@ jobs: | |||
| # Zip and publish managed test components | |||
| - template: /eng/pipelines/common/upload-artifact-step.yml | |||
| parameters: | |||
| rootFolder: $(managedTestArtifactRootFolderPath) | |||
There was a problem hiding this comment.
Nit - should we redefine managedTestArtifactRootFolderPath rather than open-coding the rootFolder so that we don't get out of sync in other yml scripts?
There was a problem hiding this comment.
This is a single-use special case of this path, so I think we want it to be different. The "osGroup" parameter to this job is still not "AnyOS" as it needs to match the libraries build so we can correctly locate the artifacts, but we want to download this artifact into the correct folder in each other build (which is all of the other use cases of $(managedTestArtifactRootFolderPath).
There was a problem hiding this comment.
I think that ultimately I would be happier if we could express this in a cleaner way, e.g. by introducing a new parameter "logicalOsGroup" that would be AnyOS in this case and propagating it all the way down to
but I guess we can offload that to a future cleanup task and possibly present is as an up-for-grabs issue for infra volunteers.
There was a problem hiding this comment.
In other words, I don't see this as blocking feedback but please consider creating the cleanup issue and adding it to the CoreCLR infrastructure backlog.
There was a problem hiding this comment.
I actually believe such a task to be very instructive for junior team members eager to ramp up on yml scripts.
| <GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeFlavor=$(RuntimeFlavor)"</GroupBuildCmd> | ||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:RuntimeVariant=$(RuntimeVariant)"</GroupBuildCmd> | ||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:CLRTestBuildAllTargets=$(CLRTestBuildAllTargets)"</GroupBuildCmd> | ||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:__TestGroupToBuild=$(__TestGroupToBuild)"</GroupBuildCmd> | ||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:__SkipRestorePackages=1"</GroupBuildCmd> | ||
| <GroupBuildCmd>$(GroupBuildCmd) /nodeReuse:false</GroupBuildCmd> | ||
| <GroupBuildCmd>$(GroupBuildCmd) /maxcpucount</GroupBuildCmd> | ||
| <GroupBuildCmd>$(GroupBuildCmd) /bl:$(ArtifactsDir)/log/$(Configuration)/InnerManagedTestBuild.$(__TestGroupToBuild).binlog</GroupBuildCmd> |
There was a problem hiding this comment.
Thanks for putting this back, I think I lost that in my build.proj refactoring.
| - ${{ if eq(parameters.runtimeFlavor, 'mono') }}: | ||
| - name: liveRuntimeBuildParams | ||
| value: ${{ format('mono.corelib+libs.ref+libs.native -rc {0} -c {1} -arch {2} -ci', coalesce(parameters.liveRuntimeBuildConfig, parameters.buildConfig), parameters.liveLibrariesBuildConfig, parameters.archType) }} | ||
| - name: liveRuntimeArtifactsPathArg | ||
| value: ${{ format('/p:MonoOverridePath={0}/artifacts/bin/mono/{1}{2}.{3}.{4}', $(Build.SourcesDirectory), parameters.osGroup, parameters.osSubgroup, parameters.archType, coalesce(parameters.liveRuntimeBuildConfig, parameters.buildConfig)) }} |
There was a problem hiding this comment.
Curious and to understand, why is this needed now?
We didn't use the runtime to build the tests?
There was a problem hiding this comment.
This is needed now because there's no alternative to specify a different path to the CoreCLR/Mono artifacts for System.Private.CoreLib without overriding the whole path (unlike Libraries, which has overrides for various different paths that we can do when we set TargetOS=AnyOS)
There was a problem hiding this comment.
With #61754 or some equivalent PR, we can remove this since we won't be referencing System.Private.CoreLib directly.
| @@ -448,14 +446,17 @@ | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:TargetArchitecture=$(TargetArchitecture)"</GroupBuildCmd> | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:Configuration=$(Configuration)"</GroupBuildCmd> | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:LibrariesConfiguration=$(LibrariesConfiguration)"</GroupBuildCmd> | |||
| <GroupBuildCmd>$(GroupBuildCmd) "/p:LibrariesTargetOSConfigurationArchitecture=$(LibrariesTargetOSConfigurationArchitecture)"</GroupBuildCmd> | |||
There was a problem hiding this comment.
We usually refer to this in the libraries world as BuildSettings might make it easier to read this property, something like LibrariesBuildSettings
|
Timeouts seem to be happing across multiple PRs and occur in legs that don't use this job, so I'm going to merge. |
)" This reverts commit 6ff57f1.
…otnet#61745)" (dotnet#61799)" This reverts commit ca24ab0.
ghost
locked as resolved and limited conversation to collaborators
Match the name that's used in the job title and enable us to use TargetOS to pre-filter OS-specific tests with the new Xunit test wrapper generator.