Flow NuGetPackageRoot out of Tools.proj restore #14879
Conversation
|
I think this is okay, but I think it would be good to verify source builds. Can you push a branch to the AzDO dotnet-dotnet repo with those changes to arcade (src\arcade) and run https://dev.azure.com/dnceng/internal/_build?definitionId=1219 (internal link). /cc @ViktorHofer @dotnet/source-build-internal |
| <ItemGroup Condition="'$(Restore)' == 'true'"> | ||
| <_RestoreToolsProps Include="@(_CommonProps)"/> | ||
| <_RestoreToolsProps Include="BaseIntermediateOutputPath=$(ArtifactsToolsetDir)Common"/> | ||
| <_RestoreToolsProps Include="ExcludeRestorePackageImports=true"/> |
There was a problem hiding this comment.
📝 Setting this property forces Tools.proj.nuget.g.props to not be included. This is fine on the restore path, but we need to avoid it when invoking the ReturnNuGetPackageRoot target.
| @@ -1,13 +1,13 @@ | |||
| <!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. --> | |||
| <Project> | |||
| <Project Sdk="Microsoft.Build.Traversal"> | |||
There was a problem hiding this comment.
📝 This is the easiest way to include Microsoft.Common.targets, which is how Tools.proj.nuget.g.props gets included during evaluation.
|
|
||
| <Import Project="BuildStep.props" /> | ||
|
|
||
| <!-- Properties requires by NuGet.targets to restore PackageReferences --> | ||
| <PropertyGroup> | ||
| <TargetFramework>net472</TargetFramework> | ||
| <!-- Workaround changes from newer MSBuild requiring additional properties --> | ||
| <TargetFrameworkVersion Condition="'$(TargetFrameworkVersion)' == ''">5</TargetFrameworkVersion> |
There was a problem hiding this comment.
📝 This value was simply wrong, and after updating to use Microsoft.Build.Traversal it was resulting in build errors.
|
lgtm |
| @@ -1,13 +1,13 @@ | |||
| <!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. --> | |||
| <Project> | |||
| <Project Sdk="Microsoft.Build.Traversal"> | |||
There was a problem hiding this comment.
Where is the version for this msbuild sdk stored?
There was a problem hiding this comment.
I didn't add it anywhere so far. Do I need to?
There was a problem hiding this comment.
NuGet needs a version for any to be imported msbuild sdk. This change will break all consuming repositories that don't define a version in their global.json file.
There was a problem hiding this comment.
➡️ Switched to Microsoft.Build.NoTargets, which has a version already defined in global.json
There was a problem hiding this comment.
@sharwell I don't think that's the case for all consuming repositories, and there are 150+ branches/repos that consume arcade built from this branch. That will be a big lift to get everyone onboarded.
There was a problem hiding this comment.
Is there a way to do this as an opt-in, or provide a smooth transition?
There was a problem hiding this comment.
➡️ This no longer references a separate SDK
| @@ -1,13 +1,13 @@ | |||
| <!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. --> | |||
| <Project> | |||
| <Project Sdk="Microsoft.Build.Traversal"> | |||
There was a problem hiding this comment.
The Traversal msbuild sdk should be used when a project needs to traverse the msbuild project graph. I don't see any ProjectReferences defined in this project so Microsoft.Build.NoTargets would be better.
There was a problem hiding this comment.
In testing, Microsoft.Build.NoTargets seemed to work fine.
There was a problem hiding this comment.
➡️ Now switched to Microsoft.Build.NoTargets
There was a problem hiding this comment.
➡️ Now switched to a local definition
There was a problem hiding this comment.
Please add a comment to this and the other two files at the beginning of the file and link to the origin.
No description provided.