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

[templates] remove Directory.Build.targets, add @(ProjectCapability) #1433

Merged
merged 4 commits into from
Jul 26, 2021

Conversation

jonathanpeppers
Copy link
Member

Description of Change

The Directory.Build.targets files in the templates were a collection
of workarounds that were needed until .NET MAUI was a workload. When
.NET MAUI was a NuGet package, this was the only way to set required
values for the IDE before NuGet restore. In the early days, you had to
unload/reload MAUI projects after NuGet restore.

We can remove Directory.Build.targets from templates now, and
consolidate @(ProjectCapability) so this is defined by the workload.

The following capabilities are defined for the IDE:

  • Maui
  • MauiSingleProject, LaunchProfiles, XamarinStaticLaunchProfiles
    when $(SingleProject) is true

These new capabilities are defined to identify projects (via telemetry)
that bring in portions of .NET MAUI:

  • MauiAssets
  • MauiBlazor
  • MauiCore
  • MauiEssentials

Other changes:

  • $(_KeepLaunchProfiles) is set by default when $(SingleProject)
    is true.
  • WinUI workarounds are moved into WinUI.SingleProject.targets.
  • Fixed sdk-manifests casing for src/DotNet/DotNet.csproj -t:Install

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)
  • Targets a single property for a single control (or intertwined few properties)
  • Adds the property to the appropriate interface
  • Avoids any changes not essential to the handler property
  • Adds the mapping to the PropertyMapper in the handler
  • Adds the mapping method to the Android, iOS, and Standard aspects of the handler
  • Implements the actual property updates (usually in extension methods in the Platform section of Core)
  • Tags ported renderer methods with [PortHandler]
  • Adds an example of the property to the sample project (MainPage)
  • Adds the property to the stub class
  • Implements basic property tests in DeviceTests

Does this PR touch anything that might affect accessibility?

No

This allows easily toggling of debug target TFM by selecting the platform.
If removed, top level debug targets show as a list of devices for the selected TFM.
-->
<ProjectCapability Include="XamarinStaticLaunchProfiles" />
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @joj @tondat this is probably the portion related to the IDE.

@tondat I tried to mark you as a reviewer, but maybe you don't have contributor access on this repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tondat is suggesting in Teams we might change to:

<ItemGroup>
  <ProjectCapability Include="MauiSingleProject" />
  <ProjectCapability Include="LaunchProfilesGroupByPlatformFilters"/>
  <ProjectCapability Include="BuildOnlyActiveTargetFrameworkOnDebug" />
</ItemGroup>

Also, a target that would improve the developer loop (until we get the appropriate Project System changes);

<Target Name="ComputeXamarinTargetFrameworks" BeforeTargets="_ComputeTargetFrameworkItems">
  <PropertyGroup>
    <TargetFrameworks Condition="'$(TargetFramework)' == '' AND '$(_SelectedTargetFramework)' != ''">$(_SelectedTargetFramework)</TargetFrameworks>
  </PropertyGroup>
  <Message Text="SelectedTargetFramework = $(_SelectedTargetFramework)" />
  <Message Text="TargetFramework = $(TargetFramework)" />
  <Message Text="TargetFrameworks = $(TargetFrameworks)" />
  <Message Text="BuildingInsideVisualStudio = $(BuildingInsideVisualStudio)" />
</Target>

I'll want to test this target a bit, and probably refactor it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Final capabilities might be:

<ItemGroup>
  <ProjectCapability Include="MauiSingleProject" />
  <ProjectCapability Include="XamarinStaticLaunchProfiles" Condition=" '$(VisualStudioVersion)' &lt; '17.0' " />
  <ProjectCapability Include="LaunchProfilesGroupByPlatformFilters" Condition=" '$(VisualStudioVersion)' &gt;= '17.0' " />
  <ProjectCapability Include="BuildOnlyActiveTargetFrameworkOnDebug" />
</ItemGroup>

LaunchProfilesGroupByPlatformFilters is Dev17-only, and you can't have XamarinStaticLaunchProfiles at the same time.

@Redth Redth self-requested a review June 23, 2021 18:18
@jonathanpeppers jonathanpeppers marked this pull request as draft June 23, 2021 18:50
@jonathanpeppers jonathanpeppers force-pushed the template-directory.build.targets branch from c936c6d to c53e06d Compare June 24, 2021 19:46
@jonathanpeppers
Copy link
Member Author

ComputeXamarinTargetFrameworks target

It does not appear that adding this target will currently work:

<Target Name="ComputeXamarinTargetFrameworks" BeforeTargets="_ComputeTargetFrameworkItems">
  <PropertyGroup>
    <TargetFrameworks Condition="'$(TargetFramework)' == '' AND '$(_SelectedTargetFramework)' != ''">$(_SelectedTargetFramework)</TargetFrameworks>
  </PropertyGroup>
  <Message Text="SelectedTargetFramework = $(_SelectedTargetFramework)" />
  <Message Text="TargetFramework = $(TargetFramework)" />
  <Message Text="TargetFrameworks = $(TargetFrameworks)" />
  <Message Text="BuildingInsideVisualStudio = $(BuildingInsideVisualStudio)" />
</Target>

This needs to run in the "outer" build when $(TargetFramework) is blank and $(TargetFrameworks) plural is set. The "outer" build does not even load .NET workloads, those are loaded when $(TargetFramework) is set in the "inner" build. I currently do not see a way to provide this target from MAUI.

ProjectCapability

I think the current logic here is right, but my IDE isn't working as expected. I don't have a way to select my Android emulator, it fails to deploy when I hit the Play button:

image

TBD if this is an IDE issue, or something else related to workloads.

@jonathanpeppers
Copy link
Member Author

$(TargetPlatformIdentifier) changed to Android, proper case, and that caused the IDE issue. See:

@jonathanpeppers jonathanpeppers force-pushed the template-directory.build.targets branch from c53e06d to b5033f1 Compare June 30, 2021 20:02
@Redth Redth force-pushed the template-directory.build.targets branch from b5033f1 to db88124 Compare July 20, 2021 21:02
@@ -10,20 +10,6 @@
<PublishTrimmed Condition=" '$(TargetFramework)' == 'net6.0-ios' or '$(TargetFramework)' == 'net6.0-maccatalyst' ">false</PublishTrimmed>
Copy link
Member

Choose a reason for hiding this comment

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

@mattleibow can we remove this now?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe. I know I did in my pr and I think the issue was that we were using Xcode 12. Now with Xcode 13 all over, we probably can remove it.

@Redth Redth marked this pull request as ready for review July 21, 2021 01:33
@Redth Redth requested a review from Eilon as a code owner July 21, 2021 01:33
Directory.Build.targets Outdated Show resolved Hide resolved
@Redth Redth added this to the 6.0.100-preview.7 milestone Jul 21, 2021
<Target Name="ValidateWinUIPlatform" />
<Target Name="BinPlaceBootstrapDll" />
Copy link
Member

Choose a reason for hiding this comment

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

Does this come in late enough? The WinUI targets from the nugets create this target, and we have to overwrite with a dummy.

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 is imported as late as a workload can do it:

<Import Project="WinUI.targets" Condition=" '$(TargetPlatformIdentifier)' == 'windows' " />

<PropertyGroup>
<AfterMicrosoftNETSdkTargets>$(AfterMicrosoftNETSdkTargets);$(MSBuildThisFileDirectory)Microsoft.Maui.Controls.Sdk.After.targets</AfterMicrosoftNETSdkTargets>
</PropertyGroup>

We'd need to test, but if it doesn't work we'd have to put the Directory.Build.targets back.

@mattleibow
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jonathanpeppers and others added 4 commits July 26, 2021 18:29
The `Directory.Build.targets` files in the templates were a collection
of workarounds that were needed until .NET MAUI was a workload. When
.NET MAUI was a NuGet package, this was the only way to set required
values for the IDE before NuGet restore. In the early days, you had to
unload/reload MAUI projects after NuGet restore.

We can remove `Directory.Build.targets` from templates now, and
consolidate `@(ProjectCapability)` so this is defined by the workload.

The following capabilities are defined for the IDE:

* `Maui`
* When `$(SingleProject)` is `true`:
  * `MauiSingleProject`, `LaunchProfiles`
  * `XamarinStaticLaunchProfiles` older than Dev17
  * `LaunchProfilesGroupByPlatformFilters` otherwise

These new capabilities are defined to identify projects (via telemetry)
that bring in portions of .NET MAUI:

* `MauiAssets`
* `MauiBlazor`
* `MauiCore`
* `MauiEssentials`

Other changes:

* `$(_KeepLaunchProfiles)` is set by default when `$(SingleProject)`
  is `true`.
* WinUI workarounds are moved into `WinUI.SingleProject.targets`.
* Fixed `sdk-manifests` casing for `src/DotNet/DotNet.csproj -t:Install`
@Redth Redth force-pushed the template-directory.build.targets branch from 24a67a3 to 72f5134 Compare July 26, 2021 22:30
@Redth Redth merged commit f835baf into dotnet:main Jul 26, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants