Improve ref assembly generation process

[tmat]

Currently reference assembly sources are checked into the repository and generated manually. Nothing in the build system validates that the resulting reference assemblies actually match the implementation assemblies.

This is very error prone process. Especially for external contributor who doesn't know that they need to run some custom build target to regenerate the ref assembly sources. As it happened a few times already for System.Reflection.Metadata these ref assemblies get out of sync.

The unit tests do not catch these breaks since they need to reference the implementation assembly in order to access internal methods and types for testing purposes. These breaks go thus unnoticed.

[tmat]

@Petermarcu @weshaggard @ahsonkhan

[Petermarcu]

+1, this has bit me as well. :)

[tannergooding]

Could this be semi-automated by having Roslyn produce the ref-assembly for projects which are also implemented in CoreFX (and that aren't just shims for S.P.Corlib)?

[jkotas]

You just need to run ApiCompat in opposite directions (swapping implementation and reference) to do what you need. As as quick hack to see how it can work, you can add this at the end of https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/src/System.Reflection.Metadata.csproj :

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />

  <PropertyGroup>
    <TargetsTriggeredByCompilation>$(TargetsTriggeredByCompilation);ValidateReverseApiCompatForSrc</TargetsTriggeredByCompilation>
  </PropertyGroup>

  <!-- ReverseReverseApiCompat for Implementation Assemblies  -->
  <Target Name="ValidateReverseApiCompatForSrc">

    <PropertyGroup>
      <ReferenceAssembly>@(ResolvedMatchingContract)</ReferenceAssembly>
    </PropertyGroup>

    <ItemGroup>
      <_DependencyDirectoriesTemp Include="@(ReferencePath->'%(RootDir)%(Directory)')" />
      <!-- Remove duplicate directories by batching over them -->
      <!-- Add project references first to give precedence to project-specific files -->
      <_DependencyDirectories Condition="'%(_DependencyDirectoriesTemp.ReferenceSourceTarget)'=='ProjectReference'" Include="%(_DependencyDirectoriesTemp.Identity)" />
      <_DependencyDirectories Condition="'%(_DependencyDirectoriesTemp.ReferenceSourceTarget)'!='ProjectReference'" Include="%(_DependencyDirectoriesTemp.Identity)" />
      <_ContractDependencyDirectories Include="@(ResolvedMatchingContract->'%(RootDir)%(Directory)')" />
      <_ContractDependencyDirectories Include="$(ContractOutputPath)" />
    </ItemGroup>

    <PropertyGroup>
      <ReverseApiCompatArgs>$(ReverseApiCompatArgs) "$(IntermediateOutputPath)"</ReverseApiCompatArgs>
      <ReverseApiCompatArgs>$(ReverseApiCompatArgs) -contractDepends:"@(_DependencyDirectories, ','),"</ReverseApiCompatArgs>
      <ReverseApiCompatArgs>$(ReverseApiCompatArgs) -implDirs:"$(ReferenceAssembly),@(_ContractDependencyDirectories, ','),"</ReverseApiCompatArgs>
      <ReverseApiCompatExitCode>0</ReverseApiCompatExitCode>

      <ReverseApiCompatResponseFile>$(IntermediateOutputPath)ReverseApiCompat.rsp</ReverseApiCompatResponseFile>
      <ReverseApiCompatCmd>$(ToolHostCmd) "$(ToolsDir)ApiCompat.exe"</ReverseApiCompatCmd>
    </PropertyGroup>

    <MakeDir Directories="$(IntermediateOutputPath)" />
    <WriteLinesToFile File="$(ReverseApiCompatResponseFile)" Lines="$(ReverseApiCompatArgs)" Overwrite="true" />

    <Exec Command="$(ReverseApiCompatCmd) @&quot;$(ReverseApiCompatResponseFile)&quot;"
          CustomErrorRegularExpression="^[a-zA-Z]+ :"
          StandardOutputImportance="Low"
          IgnoreExitCode="true"
    >
      <Output TaskParameter="ExitCode" PropertyName="ReverseApiCompatExitCode" />
    </Exec>

    <!--
      To force incremental builds to show failures again we are invalidating
       one compile input by touching the assembly info file
    -->
    <Touch Condition="'$(ReverseApiCompatExitCode)'!='0'" Files="$(AssemblyInfoFile)" />
    <Error Condition="'$(ReverseApiCompatExitCode)'!='0'" Text="ReverseApiCompat failed for '$(TargetPath)'" />
  </Target>
</Project>

[jkotas]

You should see this list of errors with the above:

Compat issues with assembly System.Reflection.Metadata:
MembersMustExist : Member 'System.Reflection.Metadata.DebugMetadataHeader.IdStartOffset.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Reflection.PortableExecutable.DebugDirectoryBuilder.AddEntry(System.Reflection.PortableExecutable.DebugDirectoryEntryType, System.UInt32, System.UInt32)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Reflection.PortableExecutable.DebugDirectoryBuilder.AddEntry<TData>(System.Reflection.PortableExecutable.DebugDirectoryEntryType, System.UInt32, System.UInt32, TData, System.Action<System.Reflection.Metadata.BlobBuilder, TData>)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Reflection.PortableExecutable.DebugDirectoryBuilder.AddPdbChecksumEntry(System.String, System.Collections.Immutable.ImmutableArray<System.Byte>)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Reflection.PortableExecutable.DebugDirectoryEntryType System.Reflection.PortableExecutable.DebugDirectoryEntryType.PdbChecksum' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Reflection.PortableExecutable.Machine System.Reflection.PortableExecutable.Machine.Arm64' does not exist in the implementation but it does exist in the contract.
TypesMustExist : Type 'System.Reflection.PortableExecutable.PdbChecksumDebugDirectoryData' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Reflection.PortableExecutable.PEReader.ReadPdbChecksumDebugDirectoryData(System.Reflection.PortableExecutable.DebugDirectoryEntry)' does not exist in the implementation but it does exist in the contract.
Total Issues: 8

[jkotas]

If this works fine, we should put the target for reverse validation to https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/ApiCompat.targets and use it from there.

[ericstj]

We already have the target that runs GenAPI in reverse. Just build the src project with /t:GenerateReferenceSource added here: dotnet/corefx#20211. That's better even than APICompat because you don't need to manually author the source file. We don't make it automatic because we expect folks may want to massage the output a bit, but it gets you a lot further than APICompat errors.

[tannergooding]

We don't make it automatic because

It seems fine to not have it automatic for local runs (to help with a faster inner loop), but I feel we should have a CI leg that checks this (in the same manner as we enforce formatting checks, etc)

[jkotas]

We do not want to make the regeneration automatic, but the validation can be automatic for projects that opt into it. There is a ton of similar validation done by default during the build. A little bit more won't make a difference for total build times.

[tannergooding]

We do not want to make the regeneration automatic, but the validation can be automatic

Right, I was suggesting that the validation be automatic, to ensure that we never end up in an incorrect state after a PR is merged.

[jkotas]

I was commenting on the separate CI leg. I do not think separate CI leg makes sense for this. Just make it part of the full build by default.

[weshaggard]

We cannot automatically generate the ref's because we intentionally have implementations that are either not 1:1 with the ref or have API's that aren't intended to be exposed in the reference assembly. A reverse APICompat check may work for some libraries like SMR but it still won't work for things like System.Runtime because of the split of the implementation.

[tmat]

Why not have it automatic for SCI and SRM?

[weshaggard]

Why not have it automatic for SCI and SRM?

While we could potentially have them be generated automatically as part of the build for the src project we would still need to have them committed to the repo because of the way we build our repo in phases. We first build all the ref projects which have P2P references to each other. Then we build the src projects which use standard References to our ref folder we built in the ref phase to get their references. So any projects that need to reference SCI and SRM expect there to already be a reference built and in the ref folder otherwise they cannot reference them.

To avoid special logic in building and referencing for these libraries we make them consistent with the rest of the libraries in corefx and have a ref project.

[tannergooding]

@weshaggard, Roslyn shipped with a feature that lets all this be done in a single compilation pass.

The compiler simultaneously produces the ref and implementation assemblies and passes just the reference assemblies to any dependent projects.

[tannergooding]

I understand it wouldn't work for all assemblies, but it sounds like it should "just work" for projects like S.C.I or S.R.M.

I believe there is even options to explicitly produce just the implementation or just the reference assembly, if you still want those to be separate steps.

[tannergooding]

See https://github.com/dotnet/roslyn/blob/b4f6481531a601ca9d9ebf8e99f6a9fcf433f132/docs/features/refout.md

[jkotas]

I understand it wouldn't work for all assemblies

Right. We do not want every bit of CoreFX to be plumbed differently. It is complex enough as it is.

[tmat]

@jkotas Maybe we don't want SCI and SRM to be in CoreFX then.

[tmat]

I'd be fine if I got a build error that says "Ref assemblies out of date: run ... command to regenerate ref assemblies" and I wouldn't need to manually fix up the generated file.

[weshaggard]

I understand it wouldn't work for all assemblies, but it sounds like it should "just work" for projects like S.C.I or S.R.M.
I believe there is even options to explicitly produce just the implementation or just the reference assembly, if you still want those to be separate steps.

It is definitely worth considering something like this where the ref projects for these libraries are just wrappers around the src projects that put them into the reference assembly only mode and can be built during the ref phase of the corefx build. If that cannot be made to work then some ApiCompat checks can help ensure we don't accidentally ship an API miss-match.

@jkotas Maybe we don't want SCI and SRM to be in CoreFX then.

Unfortunately that ship has sailed as there are too many dependencies on them from other libraries in corefx, especially the ones that are part of the shared framework.

[ericstj]

Optional reverse API Compat seems like a reasonable thing for a library to opt in (or opt-out) of. I don't like build that updates source, every time I've tried to make that work I've gotten tons of push back or folks like @atsushikan who attrib +r their source tree (rightly) yell at me.

Anyone have a preference of opt-in vs opt-out?

[jkotas]

Turn it on by default if the project does not have ReferenceFromRuntime ?

[weshaggard]

Reverse APICompat setup and all of corefx fixed or baselined with associated issues.