Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more packages to Version.Details for source-build #8940

Merged
merged 2 commits into from Jun 29, 2023
Merged

Conversation

mthalman
Copy link
Member

Contributes to dotnet/source-build#3528

Context

When attempting to run an MSBuild command from an SDK that was source-built using the Mono runtime, the following exception occurs:

MSBUILD : error MSB1021: Cannot create an instance of the logger. Microsoft.Build.BackEnd.Logging.CentralForwardingLogger Could not load type of field 'Microsoft.Build.Shared.TypeLoader:_context' (4) due to: Could not load file or assembly 'System.Reflection.MetadataLoadContext, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies.

This is a regression caused by the change in dotnet/installer#16637. That change allowed the msbuild repo to compile against the package versions that it defines rather than source-build overriding the package version with the current source-built version. For example, the msbuild repo will target System.Reflection.MetadataLoadContext.7.0.0:

<SystemReflectionMetadataLoadContextVersion>7.0.0</SystemReflectionMetadataLoadContextVersion>

Prior to the change in dotnet/installer#16637, source-build would have overridden that version and forced the msbuild repo to use the version of System.Reflection.MetadataLoadContext that was just produced during its build. But after the change, the 7.0.0 version is being referenced. The referencing of a 7.0.0 version doesn't cause prebuilts because that version is defined in SBRP.

Referencing a 7.0.0 version is fine as long as it's only a build-time dependency and not runtime. A runtime dependency will cause an attempt to load that 7.0.0 version which will not exist for an 8.0 source-built SDK. That's exactly what happens here with this error. There must be some code path specific to Mono that causes a code path which depends on System.Reflection.MetadataLoadContext.

Changes Made

Added the following packages to Version.Details.xml:

  • System.Collections.Immutable.7.0.0
  • System.Reflection.Metadata.7.0.0
  • System.Reflection.MetadataLoadContext.7.0.0

Their presence in this file will cause source-build to override the corresponding Version property (e.g. SystemReflectionMetadataLoadContextVersion) so that it uses the "live" version (the version just produced by source-build in the current build). Even though the dependency only exists on System.Reflection.MetadataLoadContext, the other two packages need to be listed as well since those are transitive dependencies. Not listing them would cause package downgrade errors since a newer version of System.Reflection.MetadataLoadContext would be referenced with older versions of the other assemblies.

I've also updated the comments in this file so that they are applied to each package. This avoids ambiguity as to which packages the original comment applied to.

@rainersigwald
Copy link
Member

I think the problem is

<ItemGroup Condition="'$(MonoBuild)' == 'true'">
<PackageReference Include="System.Reflection.Metadata" />
</ItemGroup>

We no longer support that build configuration (it's for pre-unification Mono, not the current mono runtime). Would it be helpful to sourcebuild if we scrubbed that + the -MONO configurations from our build? I'm not sure I understand how it's getting built in this configuration today though.

@mthalman
Copy link
Member Author

I'm not sure I understand how it's getting built in this configuration today though.

Yeah, me neither. It looks like it's only set if the build config is Debug-MONO or Release-MONO. But I don't know that could get set like that. Here's a link to the binlog for the msbuild repo in the context of this Mono build leg if you want to investigate further.

@rainersigwald
Copy link
Member

Aha.

<PackageReference Include="System.Reflection.Metadata" Condition="'$(MonoBuild)' == 'true'" />
<PackageReference Include="System.Reflection.MetadataLoadContext" />

But SRM is a transitive dependency of System.Reflection.MetadataLoadContext, so that condition doesn't matter.

Then in ResolvePackageFileConflicts,

Encountered conflict between 'Reference:/vmr/src/msbuild/artifacts/source-build/self/package-cache/system.reflection.metadata/7.0.0/lib/net7.0/System.Reflection.Metadata.dll' and 'Reference:/vmr/src/msbuild/artifacts/source-build/self/package-cache/microsoft.netcore.app.ref/7.0.5/ref/net7.0/System.Reflection.Metadata.dll'. Choosing 'Reference:/vmr/src/msbuild/artifacts/source-build/self/package-cache/microsoft.netcore.app.ref/7.0.5/ref/net7.0/System.Reflection.Metadata.dll' because file version '7.0.523.17405' is greater than '7.0.22.51805'.
...
Encountered conflict between 'Platform:System.Reflection.Metadata.dll' and 'Runtime:/vmr/src/msbuild/artifacts/source-build/self/package-cache/system.reflection.metadata/7.0.0/lib/net7.0/System.Reflection.Metadata.dll'. Choosing 'Platform:System.Reflection.Metadata.dll' because file version '7.0.523.17405' is greater than '7.0.22.51805'.`

So SRM is the new, consistent, sourcebuilt version. But MetadataLoadContext doesn't see a conflict and the 7.0.0 version is passed to the compiler.

@JanKrivanek
Copy link
Member

JanKrivanek commented Jun 23, 2023

@mthalman I appologize - but I'm bit lost :-)

  • This issue exist regardless of targetting Mono in source build - correct? As otherwise I was under impression MSBuild no longer supports Mono.
  • Does this mean all transitive package references need to be explicitly listed? Or is there something special around System.Reflection.MetadataLoadContext?

@rainersigwald
Copy link
Member

@JanKrivanek yes, the Mono thing was a red herring I think. It looks like what must be listed is "any transitive dependency that is also listed as a direct dependency in some other project".

@JanKrivanek JanKrivanek merged commit 8784fbd into main Jun 29, 2023
8 checks passed
@JanKrivanek JanKrivanek deleted the sb3528 branch June 29, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants