ILC: Allow OOB reference to upgrade framework assembly #109988
Conversation
| @@ -99,16 +100,20 @@ public override bool Execute() | |||
| var list = new List<ITaskItem>(); | |||
| var assembliesToSkipPublish = new List<ITaskItem>(); | |||
| var satelliteAssemblies = new List<ITaskItem>(); | |||
| var nativeAotFrameworkAssembliesToUse = new HashSet<string>(); | |||
| var nativeAotFrameworkAssembliesToUse = new Dictionary<string, ITaskItem>(); | |||
There was a problem hiding this comment.
Should this use OrdinalIgnoreCase like some of the other path comparisons below? Or are the filenames guaranteed to have some sort of normalized casing?
There was a problem hiding this comment.
I don't think it is needed since the comparison is done on file names which should use consistent casing between the framework and OOB packages. I tried breaking it by using Reference with casing that doesn't match the filename:
<Reference Include="C:\path\to\.nuget\packages\system.diagnostics.diagnosticsource\9.0.0\lib\net8.0\system.diagnostics.diagnosticsource.dll" />
but this breaks the build before we get to ILC (tried on windows and linux).
There was a problem hiding this comment.
Nice, thanks! I think there is now a theoretical chance that if someone manually specified an ILCompiler package to use and this package is older than the framework package, we wouldn't use anything from the ILCompiler package (even corelib would be CoreCLR's). I guess we can live with that, the failure mode should be easy to spot.
Yeah, I don't have one either. Maybe if we created an assembly named system.diagnostics.diagnosticsource in a test and gave it version 100.0.0, it could work? Not sure. |
Should we fails with a good error message for attempt to override CoreLib? I am sure that it is going to save some troubleshooting in future. |
|
I hope this error message is good enough. It could be a little confusing because the intention of an old ILC reference is to override with an older version of SPC, but the message will suggest they tried to use a newer version. I hope putting the file paths in the message makes it clear. |
|
Not sure we have a good place to test changes to the ILC build integration right now. The SDK has tests for the linker build integration even though the MSBuild logic now lives in runtime. We could do the same for ILC, but I'm not convinced it's worth it for this change - though I would definitely add tests when fixing #108909. |
|
/backport to release/9.0-staging |
|
/backport to release/8.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/11959116638 |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/11959118198 |
|
Hi all. Happy to see this is resolved. How do we go about applying this fix, is it a case of waiting for a fix version of the SDK to be released? |
Fixes #109872, taking the approach @jkotas suggested in #108909 (comment).
Tested locally with the repro from #109872, using
IlcBuildTasksPathto point to a local build of the task.