fix: Fixed bug with Additional Files metadata override.

[HavenDV]

GitHub Issue (If applicable): closes #8176

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

At the moment the Uno package does not take into account that the user may have other generators that have metadata in AdditionalFiles

What is the new behavior?

_InjectAdditionalFiles Target will correctly take into account the presence of AdditionalFiles by comparing them by full path. This will simply add new metadata to existing files that are also Page/ApplicationDefinition/PRIResource/TSBindingAssemblySource without losing the existing metadata.

PR Checklist

Please check if your PR fulfills the following requirements:

• Docs have been added/updated which fit documentation template (for bug fixes / features)
• Unit Tests and/or UI Tests for the changes have been added (for bug fixes / features) (if applicable)
• Validated PR Screenshots Compare Test Run results.
• Contains NO breaking changes
• Associated with an issue (GitHub or internal) and uses the automatic close keywords.
• Commits must be following the Conventional Commits specification.

Other information

Internal Issue (If applicable):

[gitpod-io]

[HavenDV]

It's not entirely clear to me what caused the tests to crash after the last change. Everything works correctly in VS 2022 preview 4.0, but the CI environment may have a different msbuild that doesn't support this (or a nested %()). Should I undo the change?

[jeromelaban]

This caused by:

The expression """.Contains()" cannot be evaluated. Method 'System.String.Contains' not found.

Most likely because _Pages or others can be null. Either check for empty in the expression, or make it non-empty before getting to the ItemGroup creation,

[HavenDV]

This caused by:

The expression """.Contains()" cannot be evaluated. Method 'System.String.Contains' not found.

Most likely because _Pages or others can be null. Either check for empty in the expression, or make it non-empty before getting to the ItemGroup creation,

In addition to this message, there is also such a message here, so I do not think that this is the reason

src\SourceGenerators\Uno.UI.SourceGenerators\Content\Uno.UI.SourceGenerators.props(365,47): Error MSB4184: The expression ""C:\a\1\s\src\Uno.UI.FluentTheme.v2\themeresources_v2.xaml".Contains()" cannot be evaluated. Method 'System.String.Contains' not found.

[jeromelaban]

In addition to this message, there is also such a message here, so I do not think that this is the reason

src\SourceGenerators\Uno.UI.SourceGenerators\Content\Uno.UI.SourceGenerators.props(365,47): Error MSB4184: The expression ""C:\a\1\s\src\Uno.UI.FluentTheme.v2\themeresources_v2.xaml".Contains()" cannot be evaluated. Method 'System.String.Contains' not found.

It's likely because the parameter is null or empty, and there's no signature that matches Contains without parameters.

[HavenDV]

I have updated the code according to the latest changes in the master branch. I also added the necessary checks.

[HavenDV]

There are errors in the build that I don't understand. Perhaps they are caused by the fact that the full path is always used to avoid collisions.

[MartinZikmund]

@HavenDV I'm seeing for example:

System.InvalidOperationException: Cannot locate resource from 'ms-resource:///Files/App/Linked/Test_Dictionary_Linked.xaml'

During When_Implicit_Style_And_Programmatic_Explicit_Style. You should be able to reproduce it locally when you select net461 as target and using the UnitTests-only.slnf

[HavenDV]

I checked, the error is caused by the fact that data with an absolute path was added to the dictionary instead of a relative one, as it was before. I will try to find an alternative solution instead of changing the current tests.

[HavenDV]

I made some edits to make the final output match the current one. Here is a project where you can experiment and evaluate the changes.
https://github.com/HavenDV/Uno.Issues/tree/main/uno_removes_additional_files_metadata
Ultimately, the problem was not that Uno was removing metadata, but that Uno was adding duplicate values with other metadata, causing other generators that could work with these files to only get the element with Uno's metadata (the second an element with the same FullPath and other metadata was lost)
There was also a problem with finding resources after testing on a real project, but this has also been fixed.

[jeromelaban]

Would you be able to add a regression test in the runtimetests csproj (for instance), which create a custom AdditionalFile item, then validates that after the build, it's still set properly (with the right metadata)? Adding two targets, one beforebuild and one after compile that fails the build, would make it easier to ensure we're not breaking later on. Thanks!

[HavenDV]

I want to note that the syntax for updating metadata through Condition looks erroneous if you don’t know about it (because you expect an explicit Update, but I couldn’t make code through Update at the time of the PR, although I think it’s possible, I just need something like this filter the collection). Here are some examples that this is a valid code and it works:
https://github.com/MicrosoftDocs/visualstudio-docs/blob/main/docs/msbuild/msbuild-items.md#updating-metadata-on-items-in-an-itemgroup-of-a-target
dotnet/msbuild#1618 (comment)

P.S. I'll add tests a bit later when I have time.

[MartinZikmund]

@HavenDV Is this PR still applicable?

[HavenDV]

Hi @MartinZikmund, I've resolved the conflict so it may still be relevant/working, but I don't have the free time to test it right now

[unodevops]

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-8561/index.html

[unodevops]

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-8561/index.html

[jeromelaban]

Thanks for the update. It's still causing issues at this time with some combinations in regression testing.

Aside from this, would you know of a known generator that would break when using Uno?