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

Restore WAP solution and project for bundle publishing #1349

Merged
merged 5 commits into from
Sep 7, 2023

Conversation

Scottj1s
Copy link
Member

@Scottj1s Scottj1s commented Sep 1, 2023

(and remove win32)

@Scottj1s
Copy link
Member Author

Scottj1s commented Sep 1, 2023

@niels9001, @karkarl I had to revert #1339 because it introduced a new dependency on WinUI 2.7. The Microsoft.UI.Xaml.dll in WinUI 2.7 then overrides the one from WinAppSDK and does not export XamlCheckProcessRequirements, causing a crash at startup. Note that this only occurs with the wapproj build (which had been removed but is now restored in this PR), and not in the single-project build.

@michael-hawker
Copy link
Contributor

@Scottj1s there shouldn't be any change to the WinUI dependency with the package changes... We've been referencing 2.7 for months:

https://github.com/CommunityToolkit/Tooling-Windows-Submodule/blob/main/MultiTarget/PackageReferences/Uwp.props

The app should override this right? An app using WinUI 2.8 should be able to use a library built against 2.7?

Oh, this is the WinUI 3 gallery, so this is the bug again about multi-targeting for UWP and Windows App SDK in the same package? That was fixed in WASDK 1.3 I thought? This was the internal link for this before related to this issue we had. Maybe this regressed in 1.4, though we just tried a 1.4 app today and didn't have issues.

Since this was fixed in 1.3, I re-enabled multi-targeting and we hadn't seen issues elsewhere yet.

@Scottj1s
Copy link
Member Author

Scottj1s commented Sep 2, 2023

@Scottj1s there shouldn't be any change to the WinUI dependency with the package changes... We've been referencing 2.7 for months:

The last commit to this PR is the revert, to make it easy to compare. If you 'nuget restore -force WinUIGallery.DesktopWap.sln' with and without that commit, you see in .\WinUIGallery\obj\WinUIGallery.DesktopWap.Package\project.assets.json that all the new WCT package references now bring in "Microsoft.UI.Xaml": "2.7.0".

This isn't a multi-targeting issue - it's a collision of two incompatible microsoft.ui.xaml.dlls.

@michael-hawker
Copy link
Contributor

@Scottj1s I'm aware that's what happening, I'm saying is that we saw that before pre-1.3 too. It was because our package multitargeted uap and net6.0-windows that pulled both Microsoft.UI.Xaml versions in and caused a conflict because something in WASDK was pulling in the UAP targets.

That was supposedly fixed in 1.3, so we added the multi-targeting to our package back in which the latest rc has but not the older labs packages. We haven't seen this issue elsewhere though with the WASDK again with the new packages...

@Scottj1s
Copy link
Member Author

Scottj1s commented Sep 6, 2023

Note to reviewers: we'll wait until WCT is updated with a fix for the regression, which should be later today, and then update this PR in turn to use that.

@michael-hawker
Copy link
Contributor

The TFMs look good on the new packages. @niels9001 I think I forgot to ask if you tested them in your wapproj test you had. Think I confused it with the other test we were doing. MainLatest feed should have from this build when it's done: https://github.com/CommunityToolkit/Windows/actions/runs/6102571346

@Scottj1s would it be best for us to open an issue in the WASDK GitHub for the wapproj problem in general for the future?

Also, we decided we should release in the PT AM, so polishing up everything this afternoon and packages should be up on Nuget.org tomorrow with build number 8.0.230907 if all goes well (lets me sync with Niels for final validation too 😋). 🤞

@niels9001
Copy link
Collaborator

The TFMs look good on the new packages. @niels9001 I think I forgot to ask if you tested them in your wapproj test you had. Think I confused it with the other test we were doing. MainLatest feed should have from this build when it's done: https://github.com/CommunityToolkit/Windows/actions/runs/6102571346

@Scottj1s would it be best for us to open an issue in the WASDK GitHub for the wapproj problem in general for the future?

Also, we decided we should release in the PT AM, so polishing up everything this afternoon and packages should be up on Nuget.org tomorrow with build number 8.0.230907 if all goes well (lets me sync with Niels for final validation too 😋). 🤞

Tested in a standalone WAP proj, and the changes seem to work :-)! I'll add a PR to re-revert the SettingsCard changes and test it once we have everything on Nuget.org!

@Scottj1s
Copy link
Member Author

Scottj1s commented Sep 7, 2023

@Scottj1s would it be best for us to open an issue in the WASDK GitHub for the wapproj problem in general for the future?

Tested in a standalone WAP proj, and the changes seem to work :-)! I'll add a PR to re-revert the SettingsCard changes and test it once we have everything on Nuget.org!

I tested in WinUI 3 Gallery, and the changes work there as well.

Yes, let's open an issue in WASDK github to protect against inadvertently combining WinUI 2.7 and WinAppSDK.

@Scottj1s
Copy link
Member Author

Scottj1s commented Sep 7, 2023

@niels9001 thanks for the fix!
@karkarl I've updated to CommunityToolkitWinUIVersion 8.0.230907

WinUIGallery/WinUIGallery.DesktopWap.Package.wapproj Outdated Show resolved Hide resolved
WinUIGallery/WinUIGallery.DesktopWap.Package.wapproj Outdated Show resolved Hide resolved
@Scottj1s
Copy link
Member Author

Scottj1s commented Sep 7, 2023

/azp run

@Scottj1s Scottj1s merged commit 10d21fb into main Sep 7, 2023
2 checks passed
@Scottj1s Scottj1s deleted the scottj1s/restore_wap branch September 7, 2023 20:53
@Scottj1s Scottj1s restored the scottj1s/restore_wap branch September 8, 2023 00:31
@Scottj1s Scottj1s deleted the scottj1s/restore_wap branch September 8, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants