Restore broken when ContinuousIntegrationBuild is true

[JoeRobich]

#15821 made the following change

-    <!-- If the PrebultUsage feature is enabled, don't fallback to default cache locations. -->
-    <NuGetPackageRoot Condition="'$(NuGetPackageRoot)' == '' and '$(PrebuiltUsage)' == 'true'">$([MSBuild]::NormalizeDirectory('$(ArtifactsDir)', '.prebuilt-packages'))</NuGetPackageRoot>
+    <!-- When building source-only or with the ci flag, don't fallback to default cache locations. -->
+    <NuGetPackageRoot Condition="'$(NuGetPackageRoot)' == '' and
+                                 ('$(ContinuousIntegrationBuild)' == 'true' or '$(DotNetBuildSourceOnly)' == 'true')">$([MSBuild]::NormalizeDirectory('$(RepoRoot)', '.packages'))</NuGetPackageRoot>

This causes Roslyn's bootstrap build to fail in CI with the following errors.

D:\a\_work\1\s\artifacts\obj\CSharpSyntaxGenerator\Release\netstandard2.0\.NETStandard,Version=v2.0.AssemblyAttributes.cs(4,46): error CS0234: The type or namespace name 'TargetFrameworkAttributeAttribute' does not exist in the namespace 'System.Runtime.Versioning' (are you missing an assembly reference?) [D:\a\_work\1\s\src\Tools\Source\CompilerGeneratorTools\Source\CSharpSyntaxGenerator\CSharpSyntaxGenerator.csproj::TargetFramework=netstandard2.0]
D:\a\_work\1\s\artifacts\obj\CSharpSyntaxGenerator\Release\netstandard2.0\.NETStandard,Version=v2.0.AssemblyAttributes.cs(4,46): error CS0234: The type or namespace name 'TargetFrameworkAttribute' does not exist in the namespace 'System.Runtime.Versioning' (are you missing an assembly reference?) [D:\a\_work\1\s\src\Tools\Source\CompilerGeneratorTools\Source\CSharpSyntaxGenerator\CSharpSyntaxGenerator.csproj::TargetFramework=netstandard2.0]
D:\a\_work\1\s\artifacts\obj\CSharpSyntaxGenerator\Release\netstandard2.0\CSharpSyntaxGenerator.AssemblyInfo.cs(13,30): error CS0234: The type or namespace name 'AssemblyCompanyAttributeAttribute' does not exist in the namespace 'System.Reflection' (are you missing an assembly reference?) [D:\a\_work\1\s\src\Tools\Source\CompilerGeneratorTools\Source\CSharpSyntaxGenerator\CSharpSyntaxGenerator.csproj::TargetFramework=netstandard2.0]
D:\a\_work\1\s\artifacts\obj\CSharpSyntaxGenerator\Release\netstandard2.0\CSharpSyntaxGenerator.AssemblyInfo.cs(13,30): error CS0234: The type or namespace name 'AssemblyCompanyAttribute' does not exist in the namespace 'System.Reflection' (are you missing an assembly reference?) [D:\a\_work\1\s\src\Tools\Source\CompilerGeneratorTools\Source\CSharpSyntaxGenerator\CSharpSyntaxGenerator.csproj::TargetFramework=netstandard2.0]
...

It is as if netstandard2.0 refs were missing.

From looking at binlogs we can see that the NETStandard.Library does not seem to be in the NuGetPackageRoot

Just to be sure things are sane, the NuGetPackageRoot is the $(RepoRoot)/.packages

Inspecting the Restore Messages I see the following

Installed NETStandard.Library 2.0.3 from https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json to C:\Users\cloudtest\.nuget\packages\netstandard.library\2.0.3

Restore is downloading packages to the default location (C:\Users\cloudtest\.nuget\packages) and not the updated NuGetPackageRoot (D:\a\_work\1\s\.packages).

As a workaround I've set the NUGET_PACKAGES env variable, but I don't think it should be necessary.

PR where I try taking a new SDK - https://github.com/dotnet/roslyn/pull/79394/files

[ViktorHofer]

This one is REALLY interesting. The ContinuousIntegrationBuild=true property signals that a build is running on a CI server which triggers the following two important changes:

• Use a different versioning schema: -ci
• Use a different package cache folder: ~repo/.packages

I would argue that the package cache setting is the most important aspect with ContinuousIntegrationBuild=true.

Before the change that you linked to above, the package cache property (NuGetPackageRoot) was (in my opinion incorrectly) pointing to the default nuget package cache location. Because your YML code path doesn't use the eng/common scripts but dotnet build directly, the NUGET_PACKAGES env var doesn't get set. NuGet relies on that env var as it can't depend on the msbuild property as it isn't available until after restore.

There are two options:

• Revert the change that sets NuGetPackageRoot based on ContinuousIntegrationBuild=true with the caveat that on CI the default package cache location is used (which goes against the intent of that property).
• Keep the existing behavior which doesn't use the default the package cache if the property is set but validate that the NUGET_PACKAGES env var is set when ContinuousIntegrationBuild=true and error with a clear description if not.

Thoughts?

[JoeRobich]

Before the change that you linked to above, the package cache property (NuGetPackageRoot) was (in my opinion incorrectly) pointing to the default nuget package cache location.

I noticed that and agree that in the -ci context it shouldn't have been using the default cache location.

Because your YML code path doesn't use the eng/common scripts but dotnet build directly, the NUGET_PACKAGES env var doesn't get set.

Ah, I didn't realize your scripts were settings the NUGET_PACKAGES var. I think it is reasonable for Arcade to keeps its existing behavior and for Roslyn to either switch to using the eng/common scripts or set NUGET_PACKAGES as we are currently.

Thanks @ViktorHofer!

cc: @jaredpar

[jaredpar]

and for Roslyn to either switch to using the eng/common scripts or set NUGET_PACKAGES as we are currently.

How would we switch to the eng/common scripts? There are a number of places where we have .ps1 / .sh that both run locally and in CI that call into dotnet build? What is the correct alternative? It seems broken if we can't just run dotnet build in a script and have it work.

The work around that we're employing on the PR @JoeRobich linked to seems very fragile.

   # Since this build is running in CI we need to set the nuget package directory so that we restore locally.
    $env:NUGET_PACKAGES = Join-Path $RepoRoot '.packages\'
    $env:RESTORENOCACHE = $true

Effectively we're encoding arcade implementation details into our scripts. That will break the next time @ViktorHofer renames the package folder.

[ViktorHofer]

I agree that dotnet build should just work (at least for local scenario). If you don't set /p:ContinuousIntegrationBuild=true, things actually work and the default package location is used.

If that property gets explicitly set to true though, you want to make sure that NUGET_PACKAGES points to the expected location (encoded in the Arcade SDK). The eng/common scripts handle that automatically.

[jaredpar]

Why don't the arcade templates just set the ENV variable NUGET_PACKAGES at the start of every job vs. inside the build? That would mean that all execution saw the new value.

If that property gets explicitly set to true though, you want to make sure that NUGET_PACKAGES points to the expected location (encoded in the Arcade SDK). The eng/common scripts handle that automatically.

Much of our infra sets /p:ContinuousIntegrationBuild=true because that is the standard for CI. At the same time we also run dotnet commands because we're the dotnet team. This approach means that in all our scripts we effectively have to replicate an implementation detail of arcade. That doesn't seem reasonable.

If we're going to go this route then we should have an opt-out in arcade for this feature.