-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
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 🙌 |
@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? |
@michael-hawker I'm okay with removing the WinRT unit test project from the solution for now while we get this figured out. |
Thanks @andrewleader, yeah I'll leave the code there in place, but just remove the project from the solution. Let me fix that now. |
@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?) |
You didn't have to remove it from the Solution, but only disable it from the Release/AnyCPU build. |
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? |
Not sure if you are still working on it, but when I build all in VS I get two types of errors.
|
@RosarioPulella Is that branch you're working on up to date? I'm pretty sure I removed |
@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. |
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. |
Oof, so it seems like we might've missed some refactoring to do in #3498? |
Thanks, @michael-hawker! Building like a charm 🚀 🚀 |
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