Apply DependentUpon Convention Only to .resx Files

[benvillalobos]

Description

Fix #4740, build or runtime failures that arise because non-resx resources get embedded with an unexpected name.

Customer Impact

Builds fail with CS1508 or applications fail to extract a resource at runtime. Roadblock to adopting the final 3.0 SDK and to migrating to .NET Core 3.0 for applications with embedded resources (UI applications in particular).

Reported by multiple internal partners (CoreFX, EF) and customers.

Workaround

Users can explicitly opt out of the new behavior back into preview8 behavior with a property

<EmbeddedResourceUseDependentUponConvention>false</EmbeddedResourceUseDependentUponConvention>

Regression?

Yes, builds fail in cases that passed before. Introduced with #4597, a new feature to support (resx) resources without project-file impact.

Testing

Automated tests, including a new one for this specific case. e2e validation in failing scenarios using privates.

Risk

Low. Targeted fix in a conditioned codepath, so the existing workarounds will continue to work and can mitigate any problems.

---

Taking the suggestion made by @nguerrera to restrict dependentupon convention to .resx files.
#4740 (comment)

Restricting to resx makes sense because that is the case that would have been persisted with DependentUpon in a classic project. I'm not aware of anything that would have put DependentUpon for non-resx in your project unless you did it manually so I don't think it should have been part of the convention to do so.

Added a check on the Type metadata and file extension to prevent dependent upon convention from being used on non-resx files.

Users can manually assign the Type metadata when including embedded resources in their projects like so:
<EmbeddedResource Include="MyResource.txt" WithCulture="false" Type="Non-Resx" />

If the metadata isn't set, it should automatically be populated by the time the task runs.

In case the Type metadata is missing for whatever reason when the task runs, a check on the file's extension will be made to determine whether or not it's a resx file.

[nguerrera]

I'm not familiar with when Type=Resx is set, is that up to the user or to targets that precede this do it?

[rainersigwald]

Yes, please document the current state at least in the PR and maybe in the comment here.

[benvillalobos]

Users can manually set the Type metadata to Resx or Non-Resx. If that metadata isn't set, it is automatically populated by the time the task runs. Currently trying to track down when it gets set.

[benvillalobos]

It's set through the SplitResourcesByCulture target here

      <EmbeddedResource Include="@(_MixedResourceWithNoCulture);@(_MixedResourceWithCulture)" Condition="'%(Extension)'=='.resx' or '%(Extension)'=='.restext'">
        <Type Condition="'%(_MixedResourceWithNoCulture.Type)'=='' and '%(_MixedResourceWithCulture.Type)'==''">Resx</Type>
      </EmbeddedResource>
      <EmbeddedResource Include="@(_MixedResourceWithNoCulture);@(_MixedResourceWithCulture)" Condition="'%(Extension)'!='.resx' and '%(Extension)'!='.restext'">
        <Type Condition="'%(_MixedResourceWithNoCulture.Type)'=='' and '%(_MixedResourceWithCulture.Type)'==''">Non-Resx</Type>
      </EmbeddedResource>

[nguerrera]

My follow up here is do we need this fallback to extension, then? It seems like we should expect Type to be set?

[rainersigwald]

Yeah, I think I now lean toward "assume it's set and if it's not, no convention" as well. I'll push that.

[nguerrera]

Tests for the convention are failing. I assume they are test defects at this point?

[rainersigwald]

They are failing because the tests don't set the Type metadata, yes. Unfortunately, that metadata isn't documented in the CreateCSharpManifestResourceName docs, so I'm wavering in my commitment to relying on it. Are our tests representative of real-world calls, or special?

I am seeing some third-party calls to the task: https://github.com/search?l=XML&q=CreateCSharpManifestResourceName&type=Code

[rainersigwald]

Two relevant offline conversations yesterday:

• @nguerrera is now willing to take the double-checked extension as a belt-and-suspenders approach to direct callers of the task that don't use the usual common.targets flow to assign Type for inputs.
• Bar check for 16.3.x passed, so we need a build.

I'm going to merge.

[benvillalobos]

Now that the rules for manifest resource name selection have changed, refer to this dotnet docs issue for a detailed explanation on how this selection works now.