Update Test target #85
Conversation
src/Directory.Build.targets
Outdated
| <TestArgs>$(TestArgs) --test-root "$(TestRoot)"</TestArgs> | ||
| <TestArgs>$(TestArgs) $(AdditionalTestArgs)</TestArgs> | ||
|
|
||
| <_MSBuildSdksDir>/%(_DotNetPath.Directory)Sdks</_MSBuildSdksDir> |
There was a problem hiding this comment.
Is the '/' prefix intentional here? Could use use Path.Combine instead?
There was a problem hiding this comment.
Would it be better to construct this path from the NETCoreSdkVersion and dotnet root? If there are multiple SDKs installed, you'll always get the last one in the itemgroup, which may not match with --sdk-version.
There was a problem hiding this comment.
Fixed. I realized that NETCoreSdkVersion won't be set correctly in the context of the VMR. It will be set to the global.json version, not the version we're testing. So the call from the VMR can override NETCoreSdkVersion to resolve this.
| <Target Name="CreateDirectoryBuildFiles"> | ||
| <!-- Define Directory.Build.* files in the scenario tests artifacts directory to prevent the test run from | ||
| picking up the configuration from the VMR's Directory.Build.* files. We need an isolated configuration | ||
| from which to test. --> | ||
| <WriteLinesToFile File="$(TestRoot)Directory.Build.targets" | ||
| Lines="<Project />" | ||
| Overwrite="true" /> | ||
| <!-- Because the VMR (https://github.com/dotnet/dotnet) is configured to use CPM (https://github.com/dotnet/installer/pull/19286), it will | ||
| cause the scenario tests to fail with the following error: | ||
| Projects that use central package version management should not define the version on the PackageReference items but on the PackageVersion items: ... | ||
| To work around this, we explicitly disable CPM. Note that this can't be solved by including a stub Directory.Packages.props file because the default | ||
| behavior is to automatically default ManagePackageVersionsCentrally to true when it finds such a file. | ||
| https://github.com/NuGet/NuGet.Client/blob/ca13cf0a281b9774dd0238a43ab98c1927056cc2/src/NuGet.Core/NuGet.Build.Tasks/NuGet.props#L25-L33 --> | ||
| <PropertyGroup> | ||
| <_DirectoryBuildPropsContent> | ||
| <Project> | ||
| <PropertyGroup> | ||
| <ManagePackageVersionsCentrally> | ||
| false | ||
| </ManagePackageVersionsCentrally> | ||
| </PropertyGroup> | ||
| </Project> | ||
| </_DirectoryBuildPropsContent> | ||
| </PropertyGroup> | ||
| <WriteLinesToFile File="$(TestRoot)Directory.Build.props" | ||
| Lines="$(_DirectoryBuildPropsContent)" | ||
| Overwrite="true" /> | ||
| </Target> |
There was a problem hiding this comment.
Just saw this PR. I think it might be cleaner to just set the following env vars or properties in the generated projects: ImportDirectoryBuildProps=false ImportDirectoryBuildTargets=false and ImportDirectoryPackagesProps=false. That avoids creating these intermediate files and removes the need for this target.
We do that for the smoke tests that also create intermediate projects: https://github.com/dotnet/dotnet/blob/212c615a6283f3904a2e78b1a56d30363bc11634/test/Microsoft.DotNet.SourceBuild.SmokeTests/DotNetHelper.cs#L133-L136
There was a problem hiding this comment.
@mthalman just double checking, did you follow-up on this feedback or do we have a tracking issue for it?
Asking as the current approach causes issues (i.e. dotnet/sdk#41895 failed because the VMR root's Directory.Packages.props file still gets imported).
There was a problem hiding this comment.
Sorry, no, I missed that. I've logged dotnet/source-build#4489.
This is related to the work for dotnet/installer#19222. It updates the existing
Testtarget to include more functionality so that this logic doesn't need to be defined outside by the VMR.