-
Notifications
You must be signed in to change notification settings - Fork 329
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
Reunion Developer Experience #493
Conversation
785f545
to
ab1d7bc
Compare
I think we skipped a step or three in the Proposal Review Process with this PR.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend "Dev/stevenki/dev experience" for this PR's title? Perhaps you meant "Reunion Developer Experience"?
1. What kicks in the MSIX Package tooling? | ||
2. Can developers opt-out of the pri generation? | ||
3. How does building a Sparse Package look? | ||
- A Sparse Package is a Win32 app that has identity, but isn’t using MSIX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite. A 'sparse package' is a package that contains 'footprint' files but no 'content'. It's typically used in conjunction with an ExternalLocation
telling Windows where 'content' files can be found.
SUGGESTION: Remove lines 13+14. These (badly) define what a Sparse Package is, but nothing else has definitions -- #1 doesn't define what "MSIX Package" or "MSIX Package tooling" is. #2 doesn't define what "pri" is (P.S. Isn't it PRI?). #4 doesn't define what "unpackaged applications" are.
ALTERNATE SUGGESTION: Change Sparse Package
to be a hyperlink to more information e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite. A 'sparse package' is a package that contains 'footprint' files but no 'content'. It's typically used in conjunction with an ExternalLocation telling Windows where 'content' files can be found.
This may have been worded poorly because it is using an empty msix file. But I think the below definition is simpler to understand. I don't want to leak implementation details in the definition of the feature. All of that should be opaque to the developer.
- Reunion Tooling – MSBuild and VisualStudio tooling used to build Reunion apps. | ||
- Workflow – The steps a developer will take in Visual Studio to be successful using Reunion. | ||
- WAP – A project used to generate an .msix or .msixbundle. | ||
- Sparse Package – An unpackaged Win32 app with package Identity via registration of an .msix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definition is incorrect in multiple ways. Please see comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this definition incorrect?
- Reunion – The effort to unify the Win32 and UWP platforms | ||
- Reunion Tooling – MSBuild and VisualStudio tooling used to build Reunion apps. | ||
- Workflow – The steps a developer will take in Visual Studio to be successful using Reunion. | ||
- WAP – A project used to generate an .msix or .msixbundle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: Use the full term, not just the acronym e.g.
- Windows Application Packaging (WAP) - A Visual Studio project...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, feel free to use the suggestion feature so I can merge this in directly :)
|
||
## 2.0 – Definitions | ||
- Reunion – The effort to unify the Win32 and UWP platforms | ||
- Reunion Tooling – MSBuild and VisualStudio tooling used to build Reunion apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it "VisualStudio" or "Visual Studio"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, should be "Visual Studio" - can you provide a suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, should be "Visual Studio" - can you provide a suggestion?
I'd think global search/replace would do it :-)
|
||
## 2.0 – Definitions | ||
- Reunion – The effort to unify the Win32 and UWP platforms | ||
- Reunion Tooling – MSBuild and VisualStudio tooling used to build Reunion apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a "Reunion app"? Sounds like a new type of application (which it's not).
SUGGESTION: "...to build apps using Project Reunion".
Creating new WinUI3 apps will have the proper manifest files included, and project files by design. Developers will use Visual Studio to create new projects, and .NET developers will have the options of using the .NET CLI. There will be a separate doc more specifically focused on the new project experience for Windows developers that will go into more detail. | ||
|
||
##### 5.2.1.1 - Manifest Files | ||
All new WinUI3 apps will have a package.appxmanifest file, but the SxS manifest should be removed. The tooling will supply default manifest files, which it can use mt.exe to merge. The “default” manifests include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...the SxS manifest should be removed.
What SxS manifest are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a default one in new WinUI3 projects
|
||
##### 5.2.1.1 - Manifest Files | ||
All new WinUI3 apps will have a package.appxmanifest file, but the SxS manifest should be removed. The tooling will supply default manifest files, which it can use mt.exe to merge. The “default” manifests include: | ||
- WinRT registrations for WinUI (and Reunion ) types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're referring to WinRT registrations for WinUI (and Reunion) types? In a SxS manifest, for use via RegFreeWinRT? If so then can be removed.
A SxS manifest isn't necessary (or desirable). WinRT types in the Project Reunion framework package are listed in the framework's appxmanifest.xml.
These WinRT types are used by packaged apps that declare <PackageDependency Name="Microsoft.ProjectReunion".../>
through the magic of MSIX and Windows' deployment and WinRT stacks.
Not-packaged apps use Dynamic Dependencies to get access to framework packages' content (be they ProjectReunion or other framework packages). This finds WinRT types defined in the framework package's appxmanifest.xml.
Past practices using RegFreeWinRT are unnecessary (and, in some ways, counter productive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is an implementation detail that probably doesn't deserve to be in this spec. Thanks for pointing this out.
|
||
##### 5.2.1.1 - Manifest Files | ||
All new WinUI3 apps will have a package.appxmanifest file, but the SxS manifest should be removed. The tooling will supply default manifest files, which it can use mt.exe to merge. The “default” manifests include: | ||
- WinRT registrations for WinUI (and Reunion ) types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...WinUI (and Reunion)...
WinUI is part of Project Reunion. It's not a separate thing.
All new WinUI3 apps will have a package.appxmanifest file, but the SxS manifest should be removed. The tooling will supply default manifest files, which it can use mt.exe to merge. The “default” manifests include: | ||
- WinRT registrations for WinUI (and Reunion ) types | ||
- PerMonV2 (and other DPI awareness) settings | ||
The reason for this is that over time, we want to consolidate on a single manifest. This certainly won’t be possible today, and we won’t try to tell that story yet. There are things that can only be accomplished with a SxS manifest, and those scenarios will absolutely still work. Note, that in order to consolidate on a single manifest, we don’t need to change the OS or runtime implementation. We could use the package.appxmanifest format to produce a SxS manifest for unpackaged apps, just like we produce an appxmanifest.xml file for packaged/sparse apps today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is that over time, we want to consolidate on a single manifest
What multiple manifests today are you implying? SxS? MSIX? package.appxmanifest? Other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manifest files that exist in the developers project that they use to configure their application. We really shouldn't have two, and should move anything that can only be accomplished in a SxS manifest into the Package.appxmanifest file (could use a possible rename).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move anything that can only be accomplished in a SxS manifest into the Package.appxmanifest file
Did you mean anything that can be accomplished via appxmanifest should be moved out of SxS manifest?
(as-is it reads the other way - if both work use SxS. But the other direction makes more sense)
|
||
>msbuild WindowXamlAppCs.sln /m /p:Platform=x64 /p:UapAppxPackageBuildMode=StoreOnly /p:AppxBundle=Always /p:AppxBundlePlatforms="x64|arm64" /p:Configuration=Release | ||
|
||
This invokes MSBuild in a recursive manner which rebuilds the project for multiple architectures. This behavior adds enormous complexity to the build and is difficult to maintain. Furthermore, a complex build is a slow build, and so this behavior *will not* be carried forward. Instead, we will only build individual .msix files per architecture, and we will enable developers to submit their apps to the store either by using the [Windows Store Azure DevOps task](https://marketplace.visualstudio.com/items?itemName=MS-RDX-MRO.windows-store-publish), and the VS `Publish->Create App Packages` UI will be updated to handle individual .msix files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and the VS
Publish->Create App Packages
UI will be updated to handle individual .msix files.
By 'individual .msix files' you mean not bundles?
Are you aware of Flat bundles? Will that be supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, at least not the bundles that contain multiple arch's. I was not aware of flat bundles, but that does look super promising! What's the workflow for creating a flat bundle? Do I need to author a special manifest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenbrix here's more info on flat bundles. The key difference is running makeappx.exe
with a different command line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we skipped a step or three in the Proposal Review Process with this PR.
I share similar concerns.
Given there's more detail here (well beyond the Issue) I've added comments. I'll also update the Issue shortly with my feedback but thought you'd find this helpful as some potentially larger issues are noted.
|
||
#### 5.3.5 - .NET Developer Experience | ||
With .NET Core, there is the new SDK-style project format, which presents greatly simplified project format and improved experience for developers. Because of this, we want the Reunion workflow to be familiar to .NET developers. | ||
While the above MSBuild properties (GenerateAppx*OnBuild) will work for .NET developers, “Build” is not the verb used for preparing your app for distribution. The verb for .NET is “Publish”, and the Publish stage of the Build is the only place where you can do certain things like AOT compile, assembly trimming, and self-contained deployment. Because of this, we will introduce two new properties, which are similarly named: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...we will introduce two new properties, which are similarly named:
Incomplete sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no I was refactoring and missed this change. Thanks, I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments left noting ambiguities, fuzziness over your intent and some areas needing correction.
@DrusTheAxe, which larger issues are you referring to? |
@riverar, I totally agree, we've been discussing this internally, and wanted to open the discussion to customers. Since this is my last week before I'll be joining a new team, I wanted to get this out, and that's why it may feel a little rushed. @andrewleader will be taking over this effort, and so I'll assign to him. Could you make sure this is properly tracked? |
- Update nuget references to WinUI3 supported packages (i.e. Win2D & WCT) | ||
6. The existing Manifest designer tool in VS must work for these projects | ||
7. The existence of a Package.appxmanifest file will enable Packaged Desktop apps by default. | ||
8. The Reunion NuGet package will provide tooling for generating the Sparse Package and Dynamic Dependencies registration code. (link to existing issues) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also allow the user to disable the build task and customize that code if wanted.
### 5.1.3 - Other Compat/Migration Concerns | ||
None of the tooling will be WinUI/Reunion specific, and the tooling could be used for all existing UWP apps. However, the hypothesis is that trying to move existing UWP apps to the new tooling pipeline would result in noise and over-complicate the story, as well as require more documentation updates. Therefore, we will not be advertising it as such. However, one of the key components of the tooling will be that it is easily migratable, and therefore we cannot require users to make any changes to their existing build customizations. By design, this forces the tooling to maintain compatibility. | ||
|
||
With that being said, one thing that will be supported is users having a mix of old UWP and newer Reunion projects in the same .sln file or repository. This will allow developers to migrate slowly at their own pace. In order to do this, the task assemblies that ship as part of Reunion must be renamed. Otherwise, MSBuild will mistakenly use the wrong assemblies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Task assemblies are the .NET assemblies that contain VS build task code.
@stevenbrix, I'm closing old or abandoned pull requests. If you'd like to revisit this and pick it up again, feel free to reopen when you're ready to work on it. |
This is the more detailed spec for #491