The UpdateXlfOnBuild on build property is making resource DLL builds non-reproducable

[jaredpar]

When running the C# determinism suite on my machine it was failing saying that our resource DLLs were not reproducable. The difference came down to the manifest resource names in the resource assembly.

• Building Locally: Microsoft.CodeAnalysis.C_.Users.jaredpar.code.roslyn.src.Dependencies.Collections.Internal.Strings.cs.resources
• Building in CI: ManifestResourceName = Microsoft.CodeAnalysis.Internal.Strings.cs

Notice that when building locally it is including file paths from my local machine. That is true even though this build occurs with -p:DeterministicSourcePaths. Even with that though, it's strange that a resource name includes a file path.

The manifest resource name is generated by the CreateManifestResourceName target. It will use EmbeddedResources->Link as the basis for the name if it's set, otherwise it will fall back to looking at the file path. In CI it's using Link but locally it's using the file path.

In a typical build the Link metadata is set inside AssignLinkMetadata. Later on there GatherXlfSources maps @(EmbeddedResources) to @(XlfSource) and TranslateSourceFromXlf maps @(XlfSource) back to @(EmbeddedResources). Then CreateManifestResourceName reads this final value of @(EmbeddedResources). This is how it works in CI.

In a build where UpdateXlfOnBuild is set to true though this does not happen. The outer build will run a MSBuild task on the individual TFMS with a target of UpdateXlf. This causes GatherXlfSources to be executed. That inner build finishes, the normal build proceeds and leads to another inner build on the project. That re-uses the evaluation from the UpdateXlf build, it has already run GatherXlfSources hence the cached data is used on the second build. Effectively it causes GatherXlfSources to be evaluated out of order, the Link metadata is not set and manifest resource names end up having file paths in them.

This is problematic because arcade defaults to UpdateXlfOnBuild being true for local builds. Basically local builds are not reflective of our final product.

To fix this I think that either:

1. We should change the MSBuild with the UpdateXlf target to have a new global property. That way it will not share an evaluation with the later build
2. Change the model here to have a check mode where it issues a diagnostic when XLF files are out of date. That can be the new arcade default. This could run safely as a part of the normal build and users could then explicitly invoke dotnet build -t:UpdateXlf to resolve.

[nguerrera]

Hello friends!

Regarding the options presented so far:

1. We should change the MSBuild with the UpdateXlf target to have a new global property. That way it will not share an evaluation with the later build

• This adds developer inner loop time by doubling MSBuild evaluation cost.
• I'm not sure this fix will work for non-multi-targeted projects. It's possible that the same symptom can happen for single targeted builds because UpdateXlfOnBuild would still re-order targets wrong with its BeforeTargets=BeforeBuild.

2. Change the model here to have a check mode where it issues a diagnostic when XLF files are out of date. That can be the new arcade default.

• This adds developer pain when changing a resource.
• This mode already exists and is the default:

  arcade/src/Microsoft.DotNet.XliffTasks/build/Microsoft.DotNet.XliffTasks.props

  Line 17 in 2fd9b39

  <ErrorOnOutOfDateXlf Condition="'$(ErrorOnOutOfDateXlf)' == ''">true</ErrorOnOutOfDateXlf>

---

I would strongly urge considering option 3: Fix the bug without double-evaluation. 😊 I can't promise a timeline, but I will take a look at this and see if I can propose a fix.

[jaredpar]

I'm not sure this fix will work for non-multi-targeted projects. It's possible that the same symptom can happen for single targeted builds because UpdateXlfOnBuild would still re-order targets wrong with its BeforeTargets=BeforeBuild.

Can you elaborate on this a bit? My mental model is that UpdateXlf uses an inner MSBuild task, if we add a new global property that would use a new eval. Thus when the normal build moved on, it will use an eval that does not have that global property.

Does the single framework version of this happen differently?

This mode already exists and is the default

Oh that's interesting.

[nguerrera]

Can you elaborate on this a bit?

Yes.

For multi-targeted builds, we go through here:

arcade/src/Microsoft.DotNet.XliffTasks/buildCrossTargeting/Microsoft.DotNet.XliffTasks.targets

Lines 14 to 21 in 2fd9b39

	<!--
	These inner builds cannot be parallelized as they would race to
	update the same inner xlf files.
	-->
	<MSBuild Projects="$(MSBuildProjectFile)"
	Targets="UpdateXlf"
	Properties="TargetFramework=%(_XlfTargetFramework.Identity)" />
	</Target>

For single-targeted builds, above file is never imported and we go through here:

arcade/src/Microsoft.DotNet.XliffTasks/build/Microsoft.DotNet.XliffTasks.targets

Lines 57 to 60 in 2fd9b39

	<Target Name="UpdateXlfOnBuild"
	Condition="'$(EnableXlfLocalization)' == 'true' and '$(UpdateXlfOnBuild)' == 'true' and '$(_IsInnerXlfBuild)' != 'true'"
	BeforeTargets="BeforeBuild"
	DependsOnTargets="UpdateXlf" />

And for inner builds of multi-targeted projects, above is skipped by the condition, making these mutually exclusive.

[nguerrera]

Is it a quick check for you to see if your bug reproduces if you make the project single-targeted?

[nguerrera]

I'm suspicious of this as a possible culprit, that is new to the arcadified XliffTasks:

https://github.com/dotnet/arcade/blob/2fd9b394c6ef3ad22639d83b8da649c9350f88f6/eng/Microsoft.DotNet.XliffTasks.InTree.targets#L12C1-L13C1

[nguerrera]

Hmm, maybe not that's for building inside the arcade repo only, it seems. OK, I'm at the limit of what I can analyze without running a repro :)

[nguerrera]

Trying to repro here: https://github.com/nguerrera/repros

I don't have it reproing yet.

[jaredpar]

Pretty sure The resx file needs to be outside the cone of the project. The ones inside seemed to work just fine.

[nguerrera]

Yeah I have that: https://github.com/nguerrera/repros/blob/757d9de7114b77bde640829cb89a11ae763362a0/XliffResourceNames/XliffResourceNames.csproj#L11

My latest attempt even tried to pull in the other resx thing from arcade: GenerateSource=true thinking maybe it was a bad interaction between XliffTasks and that. Still no luck.

When you get a chance, can you shoot me a binlog of working and not working and/or repro steps?

[jaredpar]

Is it a quick check for you to see if your bug reproduces if you make the project single-targeted?

Yes it does.

Clone this branch https://github.com/jaredpar/roslyn/tree/repro-arcade-15296

Then do the following

> cd src\Compilers\Core\Portable
> dotnet build -bl

That will still have the file path based manifest resource.