Skip to content

Support finding nearest TFM to collect assets from#18068

Merged
mattleibow merged 4 commits into
net8.0from
dev/nearest-target-framework
Oct 26, 2023
Merged

Support finding nearest TFM to collect assets from#18068
mattleibow merged 4 commits into
net8.0from
dev/nearest-target-framework

Conversation

@mattleibow
Copy link
Copy Markdown
Member

@mattleibow mattleibow commented Oct 17, 2023

Description of Change

When building a maui app that references a maui class library with assets, only the exact match of the TFMs will be used. This PR adds support for falling back to the nearest match. The same TFM used in a project reference will also be used for the assets.

In the scenario:

  • App: net8.0-windows;net8.0-android
  • Library: net8.0;net8.0-android

Before:

  • Windows: only the assets from the app project
  • Android: assets from both

After:

  • Windows: assets from both
  • Android: assets from both

Implementation was based on these docs: https://github.com/dotnet/msbuild/blob/main/documentation/ProjectReference-Protocol.md

@mattleibow mattleibow marked this pull request as ready for review October 17, 2023 01:35
@mattleibow mattleibow requested a review from a team as a code owner October 17, 2023 01:35
@mattleibow mattleibow added this to the .NET 8 GA milestone Oct 17, 2023
@Eilon Eilon added the area-single-project Splash Screen, Multi-Targeting, MauiFont, MauiImage, MauiAsset, Resizetizer label Oct 17, 2023
@mattleibow mattleibow force-pushed the dev/nearest-target-framework branch from c9a11f7 to b3b5062 Compare October 17, 2023 16:54
hartez
hartez previously approved these changes Oct 18, 2023
Comment on lines +267 to +268
Projects="@(ProjectReference)"
Projects="%(_ResolvedProjectReferencePaths.OriginalProjectReferenceItemSpec)"
BuildInParallel="true"
Properties="TargetFramework=$(TargetFramework)"
Properties="TargetFramework=%(_ResolvedProjectReferencePaths.NearestTargetFramework)"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difference here is that % will "batch" the call running <MSBuild/> N times. Where before it would run it a single time passing in multiple projects.

Is that a problem?

Copy link
Copy Markdown
Member

@jonathanpeppers jonathanpeppers Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only other question, is what happens if OriginalProjectReferenceItemSpec or NearestTargetFramework are blank? It seems like we should handle that case.

Copy link
Copy Markdown
Member Author

@mattleibow mattleibow Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first question, you just discovered a huge bug. Before, it would run all the projects with the same TFM - even if that project did not have or support it. The new way has a different TFM for each project. So I am thinking multiple builds is actually the correct way.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the second question, I will first filter out the items that are missing these values to be safe, but they should always have them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if %(OriginalProjectReferenceItemSpec) is blank it should just use %(Identity) and %(NearestTargetFramework) fall back to $(TargetFramework)?

MSBuild is so loosely typed these could randomly become blank in the future.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, Identity is the path of the dll since the _ResolvedProjectReferencePaths item group is the result of the ResolveProjectReferences target.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the things to try and look at the resolved items, and any missing ones we use the project refs. For the TFM, we first try the nearest and if that is not found then we try the current app's TFM.

@mattleibow mattleibow marked this pull request as draft October 18, 2023 19:47
@mattleibow mattleibow marked this pull request as ready for review October 19, 2023 20:25
@mattleibow mattleibow merged commit d1fee35 into net8.0 Oct 26, 2023
@mattleibow mattleibow deleted the dev/nearest-target-framework branch October 26, 2023 13:57
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 5, 2023
@samhouts samhouts added the fixed-in-8.0.100-rc.2.9530 Look for this fix in 8.0.100-rc.2.9530 label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-single-project Splash Screen, Multi-Targeting, MauiFont, MauiImage, MauiAsset, Resizetizer fixed-in-8.0.100-rc.2.9530 Look for this fix in 8.0.100-rc.2.9530

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants