AddNewNuGetConfigForDependencies: don't append to NuGet.config

[corngood]

When building VMR on darwin, I encountered this problem:

src/repos/projects/Directory.Build.targets(106,5): error MSB4018: The "AddSourceToNuGetConfig" task failed unexpectedly.
src/repos/projects/Directory.Build.targets(106,5): error MSB4018: System.Xml.XmlException->Microsoft.Build.Framework.BuildException.GenericBuildTransferredException: There are multiple root elements. Line 9, position 2.

Description

PR Checklist

If you are upgrading the version of a repo submodule, please follow this
checklist:

• Provide a link to an issue in the consuming repo describing the need for
  the upgrade. Both this PR and the PR doing the upgrade in the consuming repo
  should link to that issue.
• Are you targeting the correct branch? For example, if you need to
  reference this version in 9.0, you should target the 9.0 branch.
• Have you done your due diligence to ensure the upgrade can be completed
  in the consuming repo in a timely manner? If consuming the dependency flow of
  this update takes a long time or needs to be backed out, it may require the
  reversion of the upgrade in this PR. That's something we want to avoid.

After Merge

• Once the package gets published via darc, you may need to manually trigger
  the darc subscription on the configuration from the consuming repo. Subscriptions
  for release branches, for example, are typically disabled.
• When consuming the dependency flow from this repo for the purposes of a
  version upgrade, consider using a separate PR or at least changing the title of
  the dependency flow PR to accurately reflect the purpose of the change. Seeing
  a PR named "Upgrade IdentityModel to 8.0.1", for example, provides better
  clarity than "Update dependencies from dotnet/source-build-externals".
• If this PR targeted a release branch, port the changes to higher release
  and main branches.

[corngood]

I'm not 100% sure why there was an existing NuGet.config, and why it only happened on darwin, but not linux. I couldn't find a race in the particular msbuild run where this task is executed.

[ellahathaway]

I'm not 100% sure why there was an existing NuGet.config, and why it only happened on darwin, but not linux. I couldn't find a race in the particular msbuild run where this task is executed.

Hi @corngood. This is a repo used for source-building the product, and we do not currently support source-building on Darwin: https://github.com/dotnet/source-build?tab=readme-ov-file#support. Please see https://github.com/dotnet/dotnet?tab=readme-ov-file#building for information on how to build the VMR (both for source-build and non-source-build scenarios).

I'm going to close your PR per the above information, but please feel free to either file an issue, start a discussion, or add a follow-up comment in this PR if you'd like to continue this discussion. Thanks!

[corngood]

I'm not asking for support with darwin, and I'm not sure that this problem is limited to darwin.

I'm pretty sure that it's still a good change, because this code is writing a full configuration document to the file:

      <NewNuGetConfigContent>
<![CDATA[
<configuration>
  <packageSources>
    <clear />
  </packageSources>
</configuration>
]]>
      </NewNuGetConfigContent>
    </PropertyGroup>
    <WriteLinesToFile Lines="$(NewNuGetConfigContent)" File="$(NewNuGetConfigFile)" Overwrite="True" />

There's no situation where appending it would make sense.

[ellahathaway]

I see. I'll go ahead and re-open this PR.

@MichaelSimons - can I get input on the above? TIA.

[MichaelSimons]

@corngood, would you have a binlog that you can share from an environment which this happened? I am concerned that the RemoveNuGetConfig target may not be running. If that is the case, there could be other scenarios which your proposed fix may not address.

[corngood]

Hmm, I think RemoveNuGetConfig is broken.

These paths are in the clone and don't actually exist (on a first build) until after PrepareInnerClone:

  <PropertyGroup>
    <NuGetConfigFile Condition="'$(NuGetConfigFile)' == '' and Exists('$(ProjectDirectory)NuGet.config')">$(ProjectDirectory)NuGet.config</NuGetConfigFile>
    <NuGetConfigFile Condition="'$(NuGetConfigFile)' == '' and Exists('$(ProjectDirectory)NuGet.Config')">$(ProjectDirectory)NuGet.Config</NuGetConfigFile>
    <NuGetConfigFile Condition="'$(NuGetConfigFile)' == '' and Exists('$(ProjectDirectory)src\NuGet.config')">$(ProjectDirectory)src\NuGet.config</NuGetConfigFile>
    <NuGetConfigFile Condition="'$(NuGetConfigFile)' == '' and Exists('$(ProjectDirectory)src\NuGet.Config')">$(ProjectDirectory)src\NuGet.Config</NuGetConfigFile>
  </PropertyGroup>

I think what's different on darwin is just the default case insensitive filesystem. Instead of creating NuGet.config next to the existing NuGet.Config, it appends to it.

On linux with a case-sensitive filesystem I end up with both config files, and NuGet.config takes precedence.

There's definitely something else that can be fixed here, but I still think this change is valid since we never want to append the nuget config. Maybe it should also be using WriteOnlyWhenDifferent?

[mmitche]

Yeah, I think this could be fixed. We should be writing the NuGet config in a new temporary location rather than overwriting the existing NuGet.config. So I think that the Overwrite=true is probably incorrect, That sounds like we're attempting to write twice for some reason.

Yeah, a binlog would be super helpful if you can get it.