@dotnet/illink-contrib Could someone take a look at these test failures?

System.ArgumentOutOfRangeException : Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')
  at System.Collections.Generic.List`1.get_Item(Int32 index)
   at Microsoft.NET.Publish.Tests.GivenThatWeWantToRunILLink.CheckILLinkVersion(TestAsset testAsset, String targetFramework) in /_/test/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs:line 2254
   at Microsoft.NET.Publish.Tests.GivenThatWeWantToRunILLink.ILLink_links_simple_app_without_analysis_warnings_and_it_runs(String targetFramework) in /_/test/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs:line 123
   at InvokeStub_GivenThatWeWantToRunILLink.ILLink_links_simple_app_without_analysis_warnings_and_it_runs(Object, Span`1)
   at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

This started failing with this commit range: dotnet/msbuild@6f44380...986f8ec

Even though it's unlikely, dotnet/msbuild@986f8ec is the only possible change from that diff. cc @jeffkl

Even though it's unlikely, dotnet/msbuild@986f8ec is the only possible change from that diff. cc @jeffkl

Those changes are behind change wave 17.10, does this repo opt into that? The builds succeeded but tests failed

    Microsoft.NET.Publish.Tests.GivenThatWeWantToRunILLink.ILLink_links_simple_app_without_analysis_warnings_and_it_runs(targetFramework: "net5.0") [FAIL]
      System.ArgumentOutOfRangeException : Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')
      Stack Trace:
           at System.Collections.Generic.List`1.get_Item(Int32 index)
        /_/test/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs(2254,0): at Microsoft.NET.Publish.Tests.GivenThatWeWantToRunILLink.CheckILLinkVersion(TestAsset testAsset, String targetFramework)
        /_/test/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs(123,0): at Microsoft.NET.Publish.Tests.GivenThatWeWantToRunILLink.ILLink_links_simple_app_without_analysis_warnings_and_it_runs(String targetFramework)
           at InvokeStub_GivenThatWeWantToRunILLink.ILLink_links_simple_app_without_analysis_warnings_and_it_runs(Object, Span`1)
           at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
      Output:
        > C:\h\w\B15409B6\p\d\dotnet.exe msbuild /t:Publish C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\HelloWorld.csproj /restore /p:RuntimeIdentifier=win-x64 /p:PublishTrimmed=true /p:TrimMode=copyused /p:SuppressTrimAnalysisWarnings=true
        MSBuild version 17.10.0-preview-24155-02+936d9316d for .NET
          Determining projects to restore...
        C:\h\w\B15409B6\p\d\sdk\9.0.100-ci\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.EolTargetFrameworks.targets(32,5): warning NETSDK1138: The target framework 'net5.0' is out of support and will not receive security updates in the future. Please refer to https://aka.ms/dotnet-core-support for more information about the support policy. [C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\HelloWorld.csproj]
        C:\h\w\B15409B6\p\d\sdk\9.0.100-ci\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.EolTargetFrameworks.targets(32,5): warning NETSDK1138: The target framework 'net5.0' is out of support and will not receive security updates in the future. Please refer to https://aka.ms/dotnet-core-support for more information about the support policy. [C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\HelloWorld.csproj]
          Restored C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\HelloWorld.csproj (in 309 ms).
        C:\h\w\B15409B6\p\d\sdk\9.0.100-ci\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.EolTargetFrameworks.targets(32,5): warning NETSDK1138: The target framework 'net5.0' is out of support and will not receive security updates in the future. Please refer to https://aka.ms/dotnet-core-support for more information about the support policy. [C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\HelloWorld.csproj]
        C:\h\w\B15409B6\p\d\sdk\9.0.100-ci\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(314,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\HelloWorld.csproj]
          HelloWorld -> C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\bin\Debug\net5.0\win-x64\HelloWorld.dll
          Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
          Optimizing assemblies for size. This process might take a while.
          HelloWorld -> C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\bin\Debug\net5.0\win-x64\publish\
        > C:\h\w\B15409B6\t\dotnetSdkTests\0l23thul.pwn\ILLink_links_---EB2FAB34\HelloWorld\bin\Debug\net5.0\win-x64\publish\HelloWorld.exe 

Not sure how the test would hit that?

ChangeWaves are on by default, so the test doesn't have to opt into them for them to fire.

I currently suspect ImportProjectExtensionTargets is being set to false, and that means there's some targets file that isn't being imported, so we don't care about ILLinkTargetsPath, and that's making us fail the unit tests.

Can you please repro it locally? I am very intrigued now and want to know what's going on but don't have time to investigate myself today.

I had a lot of trouble building for some reason, but I got the unit test to run. It failed in a similar way.

I think there should be a .targets file here:
C:\GitHub\sdk\artifacts\tmp\Debug\ILLink_links_---48DF87BA\HelloWorld\bin\Debug\net7.0\win-x64\publish
(on my computer, clearly)
but there didn't seem to be one.

I'm not very familiar with how that is supposed to work. Are you saying dotnet publish should place a .targets in the publish directory? And that's not happening? Can you get a binlog from the invocation?

I don't know very much about it either, and apparently I got the directory wrong, but I verified that the publish command is supposed to create this file:
"C:\GitHub\sdk\artifacts.nuget\packages\microsoft.net.illink.tasks\7.0.100-1.23211.1\build\Microsoft.NET.ILLink.targets"

I'll try to get a binlog and send it to you over teams

Thanks to @Forgind providing binlogs, I was able to figure out the problem. The unit tests are writing a file to $(MSBuildProjectExtensionsPath) before execution and have taken a dependency on that file being imported during restore and build. In the case of the one failure we looked at, the test is writing out a .g.targets which sets PublishTrimmed=true. During restore, that property is no longer set so the implicit package reference to the ILLink package isn't restored. So during build the property its expecting to read isn't set either.

I think the best solution is to fix the unit tests to no longer have a dependency on a generated MSBuild import be imported during restore and build. These tests are not testing MSBuildProjectExtensionsPath or anything so I don't see a reason for them to have a dependency on the functionality. Instead, the tests can just create the <Import /> itself explicitly so that they can be broken by changes like this. I'll assign this PR to myself and push a commit to fix the tests.

The "breaking change" here is if someone has a *.g.targets that they create before Restore and want imported during Restore. I told jeffkl that seems unusual to me; do you agree @dsplaisted?

If they just want it to be imported during the build, this change doesn't break that.

I pushed a fix at ceb443b

Are $(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.props and $(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.targets not considered public extensibility points? I would expect them to be active during evaluation for restore as well as during a normal eval.

This seems like a breaking change to me. Maybe usage of this extensibility point is low enough that no one else would be impacted, but I don't have a way of knowing either way.

Yes its considered a breaking change if:

1. You are generating files to $(MSBuildProjectExtensionsPath)
2. You need them imported for restore and build
   The idea is that they get imported during restore but they also get generated during restore so the file that gets embedded in the binlog is incorrect since the that existed on disk at the time of evaluation is what gets put in the binlog. So this is all about fixing that scenario.

As long as people generate the extensions as part of restore and don't rely on them being used during restore, its not a breaking change.

@jeffkl thanks for taking a look. Something's still wrong though:

/datadisks/disk1/work/BCBE0A45/w/BEFC09E7/e/testExecutionDirectory/It_enables_re---B82D54D7/web.csproj : error MSB4057: The target "WriteValuesToFile" does not exist in the project.

Whoops, I ran the other tests with this functionality locally and they passed so I just assumed I fixed those other ones. I'll take a look later today or tomorrow.

Yes its considered a breaking change if:

1. You are generating files to $(MSBuildProjectExtensionsPath)
2. You need them imported for restore and build
   The idea is that they get imported during restore but they also get generated during restore so the file that gets embedded in the binlog is incorrect since the that existed on disk at the time of evaluation is what gets put in the binlog. So this is all about fixing that scenario.

As long as people generate the extensions as part of restore and don't rely on them being used during restore, its not a breaking change.

If I'm following correctly, it sounds like you're saying that because NuGet generates $(MSBuildProjectName).nuget.g.props during restore, that I can't generate my own $(MSBuildProjectName).foo.g.props file before restore and expect it to be imported during restore. Maybe the evaluation logging can't handle different results for a wildcard import between restore and build.

To me, that's a breaking change I wouldn't expect. Even if I hadn't taken a dependency on it, if I was trying to use the project extensions functionality and encountered this new behavior I would probably find it very confusing and think it was a bug.

If we don't think anyone else using using project extensions, then it's fine to fix the issue in the test. (And we would like to migrate to the new -getProperty and -getItem functionality for this instead of using project extensions anyway.) But if we think other developers use project extensions, then I think we should reconsider the breaking change.

@jeffkl king ping as tests are still failing

@jeffkl king ping as tests are still failing

Sorry something else came up late last week, I'll get these working today.

The fix for this ended up being fairly simple. Instead of generating a .targets file to $(MSBuildProjectExtensionsPath), the tests now generate a file to obj and set a property CustomAfterMicrosoftCSharpTargets to get that imported at the end of evaluation to get the same behavior as before. I almost just wrote out a Directory.Build.targets to the project directory but wasn't sure if this repo is using logic like that already. But that would have been an even simpler fix.