-
Notifications
You must be signed in to change notification settings - Fork 387
Move dotnet-test into the artifacts folder #5602
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
Conversation
steveisok
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as long as we're all aligned.
| <Import ConfigFile="Debugger.Tests.Common.txt" /> | ||
| <DotNetRoot Condition="'$(TargetArchitecture)' != 'x86'">$(RepoRootDir)\.dotnet-test</DotNetRoot> | ||
| <DotNetRoot Condition="'$(TargetArchitecture)' == 'x86'">$(RepoRootDir)\.dotnet-test\x86</DotNetRoot> | ||
| <DotNetRoot>$(ArtifactsDir)\runtime</DotNetRoot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we had a specialized directory for x86. Is that specialization no longer needed? It appears like someone testing both x64 and x86 would have the file paths collide in the same directory and the test wouldn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this was implemented to mimic actual dotnet installs. @hoyosjs mentioned that on other platforms there are subfolders with different architectures as well (MacOS uses /x64).
Given this is intended to be used as a test asset, I didn't think we benefitted from copying this behavior.
eng/InstallRuntimes.proj
Outdated
| $(VersionsPropsPath) - path of Versions.props | ||
| From Directory.Build.props: | ||
| $(ArtifactsRuntimeDir) - artifacts\runtime directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new directory name of 'runtime' feels a bit ambiguous to me. "dotnet-test" conveyed both that the files were in the dotnet layout (as opposed to CORE_ROOT layout) and also that the files were a test asset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the folder to artifacts/dotnet-test
| $(RepositoryEngineeringDir) - the "eng" directory | ||
| $(VersionsPropsPath) - path of Versions.props | ||
| From Directory.Build.props: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing is before we supported SxS testing of x86 and x64. I am not married to this, if you're ok with that.
Currently
.dotnet-testis located in the repo root. This presents challenges when moving tests to helix as the test root has config files used during testing.I moved the dotnet install to
/artifacts/dotnet-testwhich can be packaged and sent up to helix.