Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Resizetizer assumes all projects use the same TFM version #1935

Open
mattleibow opened this issue Aug 4, 2021 · 7 comments · Fixed by #1936
Open

[Bug] Resizetizer assumes all projects use the same TFM version #1935

mattleibow opened this issue Aug 4, 2021 · 7 comments · Fixed by #1936
Labels
area-single-project Splash Screen, Multi-Targeting, MauiFont, MauiImage, MauiAsset, Resizetizer t/bug Something isn't working
Milestone

Comments

@mattleibow
Copy link
Member

mattleibow commented Aug 4, 2021

Description

We had an issue in Resizetizer where it passes the TFM from the head project to the dependencies in order to try and locate assets. As a result the TFM that is specified in the dependency is never run and could potentially get missed. This was never noticed in the past as the previous collection target did not depend on anything and never ran anything but its own target. However, this just hid the underlying issue and could potentially just never returned anything.

After PR #1905, the MSBuild tasks changed a bit so that it expected the TFMs to be restored. But, since the TFM from the head project did not match the TFM in the dependency, it fails.

The correct fix is for the dependency project to somehow resolve the incoming TFM to match one of the one it has. Not sure how easy this is.

Workaround

A workaround would be for the library project to multi-target for bot hthe head project and the desired TFM.

The issue has a workaround where all the projects in the chain target the exact same TFM. However, this is not always viable since you may have a project that targets a lower TFM for compatibility reasons.

Logs

Microsoft.Maui.WinUI-Debug.binlog.zip

@mattleibow mattleibow added this to the 6.0.100-rc.1 milestone Aug 4, 2021
mattleibow added a commit that referenced this issue Aug 4, 2021
@Eilon
Copy link
Member

Eilon commented Aug 4, 2021

Hmm not sure I fully grasp. The AppProj and SharedProj will frequently not have the same TFM, right? And the SharedProj can use only its declared TFMs for restore/resolving things. I don't think you can just pick another TFM to do the resolution because that could produce incompatible results (even if that TFM is a superset, e.g. using net6.0-android to resolve something in a net6.0 project).

cc @javiercn

@mattleibow
Copy link
Member Author

Yeah, that is the issue. The shared project was say windows8 and the app was windows10. The bug is that we pass windows10 to the shared project, and that is bad.

What needs to happen is we pass windows10 to the shared project, and then we need that shared project to properly select a TFM to use.

I heard that @jsuarezruiz might have done something relating to this before? And then @jonathanpeppers just knows everything with MSBuild.

@jonathanpeppers
Copy link
Member

@mattleibow can you see if we can remove this:

Properties="TargetFramework=$(TargetFramework)"

I vaguely remember there was some issue where it was needed, but maybe we don't.

@javiercn
Copy link
Member

javiercn commented Aug 4, 2021

@jonathanpeppers Am I correct in thinking that the right logic when calling referenced projects is something along the lines of:

  • If the project reference has a SetTargetFramework (_ProjectReferenceExistent.SetTargetFramework) metadata, use that.
  • Otherwise, undefine the TargetFramework when calling the MSBuild task to avoid flowing the parent target framework to the child?

@jonathanpeppers
Copy link
Member

@javiercn if it were me, I would try to remove TargetFramework completely if you can.

I've seen folks use something like:

<ProjectReference Include="..\foo\foo.csproj" AdditionalProperties="TargetFramework=netstandard2.0" />

Similar to: dotnet/sdk#2280 (comment)

Since this is another case, maybe it would be best to try to avoid TargetFramework?

rmarinho pushed a commit that referenced this issue Aug 4, 2021
@javiercn
Copy link
Member

javiercn commented Aug 4, 2021

@jonathanpeppers we encountered something similar when we were doing the original version of static web assets.

The original recommendation we got was to pass in SetTargetFramework if it existed, so as to avoid multiple re-evaluations.

What we ended up doing for Maui was something along the lines of, undefine TargetFramework unless the reference has a SetTargetFramework metadata defined. The reason for this I believe is multi-targeting.

I'm not an expert in this area, but in general I believe the ProjectReference protocol does a call to GetTargetFrameworks when is preparing the project references to resolve what target framework to pass if needed base on the context of the current project.

That said, It's not my area of expertise.

The goal of passing SetTargetFramework, SetConfiguration and so on, I believe is to avoid re-evaluations of the project.

@Redth Redth added the area-single-project Splash Screen, Multi-Targeting, MauiFont, MauiImage, MauiAsset, Resizetizer label Aug 4, 2021
mattleibow added a commit that referenced this issue Aug 4, 2021
@mattleibow
Copy link
Member Author

We don't want to remove this as we may have the shared project have different assets depending on the TFM referenced. Not sure if that is a thing we want to support.

We could have platform specific shared assets.

@Redth Redth added the t/bug Something isn't working label Aug 5, 2021
@Redth Redth modified the milestones: 6.0.100-rc.1, .NET 7 Sep 8, 2021
@VincentBu VincentBu added the s/verified Verified / Reproducible Issue ready for Engineering Triage label Mar 15, 2022
@samhouts samhouts removed the s/verified Verified / Reproducible Issue ready for Engineering Triage label Apr 5, 2023
@samhouts samhouts modified the milestones: .NET 8 Planning, Under Consideration Sep 27, 2023
@PureWeen PureWeen modified the milestones: Under Consideration, Backlog Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-single-project Splash Screen, Multi-Targeting, MauiFont, MauiImage, MauiAsset, Resizetizer t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants