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

Support both Update and Include for maui #14576

Closed
wants to merge 12 commits into from
Closed

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Apr 13, 2023

Description of Change

In previous versions of the IDE, adding packages only supported Include and as a result we could check at the end and see what was added and what we needed. However, in newer versions, the IDE is now smarter and can detect that we added a package, and if it is from outside the csproj, it uses Update.

As a result, this PR makes sure that we always add the NuGet so that newer IDEs (Update) have something to actually update, and to support older IDEs (Include) we remove those implicit packages to avoid duplicates.

This may be replaced by #14770

@mattleibow
Copy link
Member Author

forgot auto imports went to all the things...

@Eilon Eilon added the area-setup Installation, setup, requirements, maui-check, workloads, platform support label Apr 14, 2023
@mattleibow mattleibow marked this pull request as ready for review April 14, 2023 19:14
Comment on lines 45 to 47
<ItemGroup Condition=" '$(UseMauiNuGets)' != 'true' and '$(EnableMauiImplicitPackageReferences)' != 'false' and '$(TargetFrameworkVersion)' != '' and $([MSBuild]::VersionEquals('$(TargetFrameworkVersion)', '@MAUI_DOTNET_VERSION@')) ">
<PackageReference Include="@(_MauiImplicitPackageReference)" Exclude="@(PackageReference)" />
</ItemGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

This pre-adds all the nugets that we need into the project at the top - before the csproj. This means Update will now work (thanks IDE) but causes issues if we have an Include.

Comment on lines 29 to 35
<ItemGroup Condition=" '$(UseMauiNuGets)' != 'true' ">
<_MauiImplicitPackageReference Remove="@(PackageReference)" />
<PackageReference Include="@(_MauiImplicitPackageReference)" />
<_MauiAddedImplicitPackageReference Include="@(PackageReference->HasMetadata('IsMauiImplicitPackageReference'))" />
<_MauiAddedUserPackageReference Include="@(PackageReference->WithMetadataValue('IsMauiImplicitPackageReference',''))" />
<_MauiMissingImplicitPackageReference Include="@(_MauiAddedImplicitPackageReference)" Exclude="@(_MauiAddedUserPackageReference)" />
<PackageReference Remove="@(PackageReference)" />
<PackageReference Include="@(_MauiAddedUserPackageReference);@(_MauiMissingImplicitPackageReference)" />
</ItemGroup>
Copy link
Member Author

@mattleibow mattleibow Apr 14, 2023

Choose a reason for hiding this comment

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

So we fix that here by removing the duplicates that we added. If a developer references a package then we can detect that as it will not have the IsMauiImplicitPackageReference metadata. This now fixes the double Includes (from us and from the csproj)

This may be the case from a previous project that is now built with this workload.

@jonathanpeppers
Copy link
Member

@dsplaisted do we think moving implicit @(PackageReference) to AutoImport.props is the general right idea?

It makes sense to me, so customers can modify the values from their .csproj file.

@mattleibow
Copy link
Member Author

One thing to think about is that older versions (not much as it may just be changed in the preview VS) that the package manager only did Include, now it does Update.

Not sure if that difference is important for any other workloads.

@Redth
Copy link
Member

Redth commented Apr 17, 2023

@dsplaisted @marcpopMSFT @jonathanpeppers any concerns here?

@marcpopMSFT
Copy link
Member

Since the VS ide package manager doesn't handle conditional packagereferences correctly today, will this impact that experience some (I understand they are discussing options): NuGet/Home#4681

@mattleibow
Copy link
Member Author

Not sure about the impact. This autoimports is adding a package for all tfms. Not sure if workloads can handle packages added in this way?

Is there a design to allow workloads to depend in nugets? Or is the preferred way to use nugets in the csproj?

We used to just add things in the targets, but now the IDE does not handle that anymore and the PM does an update package making it mostly useless. Unless the PM should revert that change and we go back to adding additional nugets via the targets?

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

I think this is worth trying, and will probably work pretty well.

src/Workload/Microsoft.Maui.Sdk/Sdk/AutoImport.in.props Outdated Show resolved Hide resolved
src/Workload/Microsoft.Maui.Sdk/Sdk/AutoImport.in.props Outdated Show resolved Hide resolved
</_MauiImplicitPackageReference>
</ItemGroup>
<ItemGroup Condition=" '$(UseMauiNuGets)' != 'true' and '$(EnableMauiImplicitPackageReferences)' != 'false' and '$(TargetFrameworkVersion)' != '' and $([MSBuild]::VersionEquals('$(TargetFrameworkVersion)', '@MAUI_DOTNET_VERSION@')) ">
<PackageReference Include="@(_MauiImplicitPackageReference)" Exclude="@(PackageReference)" />
Copy link
Member

Choose a reason for hiding this comment

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

I believe that instead of specifying IsMauiImplicitPackageReference on each individual item, you could set it once here instead.

Also, I don't think Exclude="@(PackageReference)" does anything here, so it's probably best to remove it for clarity.

@dsplaisted
Copy link
Member

Since the VS ide package manager doesn't handle conditional packagereferences correctly today, will this impact that experience some (I understand they are discussing options): NuGet/Home#4681

I think that this is probably not going to be a problem here. I think the linked issue has to do with packages that are referenced for some of the target frameworks used in a project but not others. That is not normally going to be the case here, as even though the conditions do depend on the target framework version, I don't think it is common to have a Maui project multi-target to different versions of .NET (they multi-target to different platforms instead).

@mattleibow
Copy link
Member Author

Superseded by #14770

@mattleibow mattleibow closed this Apr 28, 2023
@mattleibow mattleibow deleted the dev/support-packages branch April 28, 2023 07:52
@mattleibow
Copy link
Member Author

Actually, this will still occur for existing projects because maui will still be added via the targets and thus the PM will do an Update.

@mattleibow mattleibow restored the dev/support-packages branch April 28, 2023 07:54
@mattleibow mattleibow reopened this Apr 28, 2023
@mattleibow
Copy link
Member Author

Closing this for now as the correct way would be to add the package reference manually in net8. We can revisit this later if that is not pretty.

@mattleibow mattleibow closed this May 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-setup Installation, setup, requirements, maui-check, workloads, platform support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants