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

Temp - Remove 'native' target for Notifications package to unblock CI #3570

Merged
merged 2 commits into from Nov 20, 2020

Conversation

@michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented Nov 20, 2020

Fixes #3564

Remove the 'native' target framework that is causing issues on VS 16.8 for now until a long-term solution can be found.

Current theory is 16.8 broke the workaround we were using to address this issue: NuGet/Home#5154

FYI @andrewleader @azchohfi

@msftbot
Copy link
Contributor

@msftbot msftbot bot commented Nov 20, 2020

Thanks michael-hawker for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@msftbot msftbot bot requested review from azchohfi and Kyaa-dost Nov 20, 2020
@msftbot msftbot bot added the bug 🐛 label Nov 20, 2020
@michael-hawker
Copy link
Member Author

@michael-hawker michael-hawker commented Nov 20, 2020

@azchohfi looks like the Unit Test for the Notifications is failing to build. It's expecting the winmd output for one section of the tests.

We just need to remove the UnitTests.Notifications.WinRT.csproj file from the solution to prevent it building right?

@andrewleader
Copy link
Contributor

@andrewleader andrewleader commented Nov 20, 2020

@michael-hawker I'm okay with removing the WinRT unit test project from the solution for now while we get this figured out.

@michael-hawker
Copy link
Member Author

@michael-hawker michael-hawker commented Nov 20, 2020

Thanks @andrewleader, yeah I'll leave the code there in place, but just remove the project from the solution. Let me fix that now.

@michael-hawker
Copy link
Member Author

@michael-hawker michael-hawker commented Nov 20, 2020

@azchohfi @Kyaa-dost @RosarioPulella looks like we're building again! 🎉🎉🎉

Anyone up for a quick review to audit me?

Then @Sergio0694 we should be able to add the multi-targets for .NET 5 back to those 2-3 PRs, eh?

@azchohfi would it be easy to backport the .NET 5 targeting you did for this Notifications package as well so the next 7.0.0 preview can support .NET 5 as well? (Or at least point @andrewleader to your change commit?)

@azchohfi
Copy link
Contributor

@azchohfi azchohfi commented Nov 20, 2020

You didn't have to remove it from the Solution, but only disable it from the Release/AnyCPU build.
There is not a single commit on that file that did that, since there were many merges later, but this is the file:
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/winui/Microsoft.Toolkit.Uwp.Notifications/Microsoft.Toolkit.Uwp.Notifications.csproj

@michael-hawker
Copy link
Member Author

@michael-hawker michael-hawker commented Nov 20, 2020

Thanks @azchohfi, I really wish there was an easier way to configure all that stuff, the VS interface is a mess and the solution file is unreadable. (Maybe a good UWP sample app for the Toolkit to make a thing that loads a SLN and can easily just muck with all that... 🤔)

I wasn't sure if folks locally trying to run the test suite would run into issues as well, so I figured it was a bit safer to just remove it entirely out of view for now until such time we resolve the issue and can just revert the one commit I did for that...

Thoughts?

@RosarioPulella
Copy link
Contributor

@RosarioPulella RosarioPulella commented Nov 20, 2020

Not sure if you are still working on it, but when I build all in VS I get two types of errors.

error CS0006: Metadata file 'WindowsCommunityToolkit\Microsoft.Toolkit.Uwp.UI.Controls.Markdown\bin\Debug\uap10.0.17763\Microsoft.Toolkit.Uwp.UI.Controls.Markdown.dll' could not be found
And
WindowsCommunityToolkit\UnitTests\UnitTests.XamlIslands.UWPApp\XamlIslandsTest_Gaze.cs(24,19,24,33): error CS1929: 'DispatcherQueue' does not contain a definition for 'ExecuteOnUIThreadAsync' and the best extension method overload 'DispatcherHelper.ExecuteOnUIThreadAsync(CoreApplicationView, Action, CoreDispatcherPriority)' requires a receiver of type 'CoreApplicationView'

@Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Nov 20, 2020

@RosarioPulella Is that branch you're working on up to date? I'm pretty sure I removed ExecuteOnUIThreadAsync in #3498 🤔

@michael-hawker
Copy link
Member Author

@michael-hawker michael-hawker commented Nov 20, 2020

@RosarioPulella everything's building in the CI here with this fix, so sounds like you need to clean and update whatever branch you're on.

@azchohfi
Copy link
Contributor

@azchohfi azchohfi commented Nov 20, 2020

I don't think we build or run the Xaml Islands tests on the CI, so it might be broken with the change in #3498.

@Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Nov 20, 2020

Oof, so it seems like we might've missed some refactoring to do in #3498?
To be clear, that PR only had some renamings for the most part, so hopefully a simple replace all should fix that 😅

@Kyaa-dost
Copy link
Contributor

@Kyaa-dost Kyaa-dost commented Nov 20, 2020

Thanks, @michael-hawker! Building like a charm 🚀 🚀

@michael-hawker michael-hawker merged commit 5129d2c into master Nov 20, 2020
5 checks passed
5 checks passed
Toolkit Releases Toolkit Releases succeeded
Details
Toolkit-CI Build #7.0.0-build.498+a4041aa353 succeeded
Details
WIP Ready for review
Details
auto-merge.config.enforce No dynamic merge policies are applicable.
license/cla All CLA requirements met.
Details
@delete-merged-branch delete-merged-branch bot deleted the michael-hawker-patch-1 branch Nov 20, 2020
@michael-hawker michael-hawker added this to the 7.0 milestone Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.