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

Updated Templates for Blazor apps for MacCatalyst #14196

Merged
merged 10 commits into from
Apr 11, 2023

Conversation

dustin-wojciechowski
Copy link
Contributor

@dustin-wojciechowski dustin-wojciechowski commented Mar 24, 2023

Description of Change

I updated the Maui templates to address issues brought up in #12293. I set this to draft because I am unsure of some of the approaches I've taken. I created a PR in Xamarin-macios to address the maccatalyst templates we have there: xamarin/xamarin-macios#17830

  1. Created comment section in csproj explaining how to set the correct RuntimeIdentifiers when publishing.
  2. Created conditional property groups based on Debug and Release to handle codesigning, package signing, etc.
  3. Created two Entitlement files, Debug and Release. The two were necessary because <key>com.apple.security.get-task-allow<key> is necessary to be able to use Dev tools while debugging Blazor apps, while com.apple.security.app-sandbox is required by the App Store for publishing and com.apple.security.network.client is necessary for Blazor apps when sandbox is enabled.
  4. Two keys added to the Info.plist: <ITSAppUsesNonExemptEncryption> and <LSApplicationCategoryType>

Also, if these are transferrable to other macios/iOS templates, I could also add them to their respective templates in future PR's.

Fixes #5561
Fixes #14219
Fixes #13080
Fixes #7706

@dustin-wojciechowski dustin-wojciechowski added platform/macOS 🍏 macOS / Mac Catalyst area-templates Project templates, Item Templates for Blazor and MAUI Blazor ❤️ MAUI Issues in MAUI functionality that affect Blazor, but are not bugs in Blazor itself labels Mar 24, 2023
@dustin-wojciechowski
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@jfversluis jfversluis left a comment

Choose a reason for hiding this comment

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

We also need this for the regular templates, at least the Entitlements.Debug.plist so that we can also inspect WebViews there. In that case also add this one to the "fixes" list: #7706

And some failing tests...

Comment on lines 63 to 78
<!-- Notes for Publishing on the Mac App Store:
1. For macOS it's possible to sign both the app bundle (EnableCodesigning=true) and the package (*.pkg) (EnablePackageSigning=true),
and these are signed separately and with different certificates.
CodesignKey: this is the signing key used for the app bundle
PackageSigningKey: this is the signing key used for the package
2. Publishing to the App Store requires signing both the app bundle and the package.
Must be 'Apple Distribution: ...' for the app bundle. Note that apps signed like this will not execute locally.
They have to be published to the App Store and then downloaded (Apple will resign the app with a different signing identity that allows for local execution).
Must be '3rd Party Mac Developer Installer: ...' for the pkg
3. Publishing outside of the App Store (i.e. only notarizing) requires:
Must be 'Developer ID Application: ...' for the app bundle
Must be 'Developer ID Installer: ...' for the pkg
4. During development, use the 'Apple Development: ...' signing key (typically to verify that the app works when is signed and entitlements are enforced).
5. Depending on the entitlements the app needs, a specific provisioning profile (CodesignProvision) might be needed.
6. UseHardenedRuntime must be set to true when app sandbox is enabled in Info.plist.
-->
Copy link
Member

Choose a reason for hiding this comment

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

While this is pretty epic!!!!

Should we consider moving this to a more lively document with an aka.ms link maybe? That will give us the ability to update it needed without publishing an update. And also I'm not sure if people would be really happy with paragraphs of comments in their csproj?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I'll remove it and perhaps make a link to some documentation. Would this be the right section for it? https://learn.microsoft.com/en-us/dotnet/maui/mac-catalyst/deployment/?view=net-maui-7.0

Comment on lines 81 to 84
<!--
<CodesignProvision>YOUR PROFILE NAME</CodesignProvision>
<CodesignKey>Apple Development: YOURNAME (*******)</CodesignKey>
-->
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? Should it have some instructions as well?

Comment on lines 94 to 98
<!--
<CodesignKey>Apple Development: YOURNAME (*******)</CodesignKey>
<CodesignProvision>YOUR PROFILE NAME</CodesignProvision>
<PackageSigningKey>3rd Party Mac Developer Installer: YOURNAME (*******)</PackageSigningKey>
-->
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. We have docs about this now, maybe also aka.ms link those?

Comment on lines 88 to 93
<EnableCodeSigning>True</EnableCodeSigning>
<ProvisionType>Manual</ProvisionType>
<CreatePackage>true</CreatePackage>
<EnablePackageSigning>true</EnablePackageSigning>
<CodesignEntitlements>Platforms/MacCatalyst/Entitlements.Release.plist</CodesignEntitlements>
<UseHardenedRuntime>true</UseHardenedRuntime>
Copy link
Member

Choose a reason for hiding this comment

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

This is probably why the test is failing. Should we enable this or just add some instructions?
First, this will complicate our pipeline and we need to manually add valid certs each year etc.

Second, this is not something people have today and I think the tooling will now already help get some of this in there? Or at the very least the documentation is there... But not sure what the best option is to do this.

@rmarinho rmarinho requested a review from Eilon March 30, 2023 10:52
@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Mar 30, 2023
@dustin-wojciechowski
Copy link
Contributor Author

Per @jfversluis, I've removed the content from the csproj and will update it with a link to the documentation when it's been updated. Also working on getting as much in here without breaking any tests.

@dustin-wojciechowski dustin-wojciechowski marked this pull request as ready for review April 5, 2023 00:01
@dustin-wojciechowski dustin-wojciechowski requested a review from a team as a code owner April 5, 2023 00:01
@@ -60,6 +60,9 @@
<PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="MS_EXT_VERSION" />
</ItemGroup>

<!-- Build Properties must be defined within these property groups to ensure successful publishing
to the App Store. See "Define build properties in your project file" here:
https://learn.microsoft.com/en-us/dotnet/maui/mac-catalyst/deployment/publish-app-store -->
Copy link
Member

Choose a reason for hiding this comment

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

  1. This needs to be an aka.ms link
  2. Make sure you remove the en-us from the URL so that people don't always go to the English-US docs. If you omit the en-us then it will auto-redirect to the docs in the user's preferred locale

Copy link
Member

Choose a reason for hiding this comment

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

(Same applies to all other links.)

@Eilon
Copy link
Member

Eilon commented Apr 5, 2023

Per @jfversluis 's comment, the same changes need to be present in the maui-mobile template as well (they can also use WebViews, or people can add a BlazorWebView to them).

<!-- Note for MacCatalyst:
The default runtime is maccatalyst-x64, except in Release config, in which case the default is maccatalyst-x64;maccatalyst-arm64.
When specifying both architectures, use the plural <RuntimeIdentifiers> instead of the singular <RuntimeIdentifer>.
The App Store will NOT accept apps with ONLY maccatalyst-arm64 indicated;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The App Store will NOT accept apps with ONLY maccatalyst-arm64 indicated;
The Mac App Store will NOT accept apps with ONLY maccatalyst-arm64 indicated;

In case any reader is unfamiliar with the generic sounding term "App Store," I suggest adding "Mac" to the name.

The Apple App Store guidelines specifically mention that either form is acceptable, but I think the more specific one could be more helpful.

Guidelines: https://developer.apple.com/app-store/marketing/guidelines/

App Store
Always typeset App Store with an uppercase A and an uppercase S followed by lowercase letters.

Refer to just the App Store unless you need to be more specific; in that case, you can use App Store for iPhone, App Store for iPad, Mac App Store, App Store for Apple TV, App Store for Apple Watch, or App Store for iMessage. To refer to all the versions, use this order: App Store for iPhone, iPad, Mac, Apple Watch, Apple TV, and iMessage. Don’t use terms such as Apple Watch App Store or App Store for watchOS.

@@ -2,6 +2,15 @@
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<!-- The App store requires you specify if the app uses encryption. -->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- The App store requires you specify if the app uses encryption. -->
<!-- The Mac App Store requires you specify if the app uses encryption. -->

(At a minimum the S needs to be capitalized, but I suggest also prefixing Mac.)

@@ -48,7 +56,23 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="MS_EXT_VERSION" />
<PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="$(MicrosoftExtensionsPackageVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Hmm is this change deliberate? These templates have some funky pre-processing that is applied as part of the build to substitute a specific version that ends up baked into the shipped template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof, accidentally got modified, thanks!

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Looks really good! Just a few small comments for you to consider.

Comment on lines 72 to 77
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Debug|DOTNET_TFM-ios|AnyCPU'">
<CreatePackage>false</CreatePackage>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Release|DOTNET_TFM-ios|AnyCPU'">
<CreatePackage>false</CreatePackage>
</PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

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

These 6 lines (72-77) appear here, but not in the maui-blazor template. Is that deliberate? If so, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure how this got in there, I'm not quite sure if that was generated by VS for Mac or if I mistakenly included it somehow :(

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Just one last comment 😄

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@Eilon
Copy link
Member

Eilon commented Apr 10, 2023

@dustin-wojciechowski I appreciate all your effort and patience in this issue. Updating templates is both the easiest thing in the world, and the most difficult thing in the world 😁

@rmarinho rmarinho closed this Apr 11, 2023
@rmarinho rmarinho reopened this Apr 11, 2023
@rmarinho rmarinho enabled auto-merge (squash) April 11, 2023 16:03
@rmarinho rmarinho merged commit 9fdfc84 into main Apr 11, 2023
30 checks passed
@rmarinho rmarinho deleted the dev/Update-Blazor-Templates branch April 11, 2023 18:44
@samhouts samhouts added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Jul 28, 2023
@hartez hartez added the backport/NO This change should not be backported. It may break customers. label Aug 16, 2023
@Redth
Copy link
Member

Redth commented Aug 16, 2023

Linking this for more context: #16355

We talked about cleaning up the templates so that we can remove the entitlements files from the template, as well as the csproj entries. We can instead have some of the basic entitlements set by default in the macios SDK so that this is all unnecessary to be included in our MAUI templates, and users are still able to disable the default entitlements included in these scenarios.

@Redth
Copy link
Member

Redth commented Aug 16, 2023

Here's the macios PR: xamarin/xamarin-macios#18669

@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView area-templates Project templates, Item Templates for Blazor and MAUI backport/NO This change should not be backported. It may break customers. backport/suggested The PR author or issue review has suggested that the change should be backported. Blazor ❤️ MAUI Issues in MAUI functionality that affect Blazor, but are not bugs in Blazor itself platform/macOS 🍏 macOS / Mac Catalyst
Projects
None yet
7 participants