Skip to content

Workaround UDK include issue#1364

Merged
loneursid merged 5 commits intoWNP_LRPfrom
WNP_LRP-CrisisOverride
Sep 3, 2021
Merged

Workaround UDK include issue#1364
loneursid merged 5 commits intoWNP_LRPfrom
WNP_LRP-CrisisOverride

Conversation

@loneursid
Copy link
Copy Markdown
Contributor

@loneursid loneursid commented Sep 2, 2021 •
edited
Loading

There is a bug in the FrameworkUDK resulting in a file not found during the build. The file that can't be found is PushNotificationsRT.h. The fix is simple: replace angle brackets with double quotes and it has been posted to the correct rs_xxx branches. Still, the fix hasn't been ingested into the WindowAppSDK.

This work around addresses the build failure and allows the Unpackaged Push Notifications feature (which depends on the FrameworkUDK) to compile and work properly. It does that by placing one copy of the file inside the project folder where it can be easily located by the build tools.

This is only a temporary solution to allow the feature to ship with WindowsAppSDK v1.0 and the added files will have to be removed once the FrameworkUDK has been updated.

The file contents are identical across all three platforms: amd64, arm64 and x86, so keeping a single copy of it works accross al platforms.

Other options considered but rejected:

  1. Adding the path to PushNotificationsRT.h inside FrameworkUDK package to the project settings. This works, but it means version number of the package get hardcoded in the project file and the build will break as soon as the FrameworkUDK gets updated.
  2. Using a script to copy the file from the package to a location where the build can easily access it. It has the same issue has the option above, though the script could be made to fail silently if the file can't found. The build itself wouldn't break then, because the next version of the FrameworkUDK will include a fix that will ensure PushNotificationsRT.h is found inside the package.

@loneursid loneursid added the area-Notifications Toast notification, badges, Live Tiles, push notifications label Sep 2, 2021
@ghost ghost added the needs-triage label Sep 2, 2021
@loneursid loneursid marked this pull request as ready for review September 2, 2021 22:40
@danielayala94
Copy link
Copy Markdown
Contributor

danielayala94 commented Sep 3, 2021

Can you create a deliverable to remove these copies?

hulumane
hulumane reviewed Sep 3, 2021
View reviewed changes
Comment thread dev/PushNotifications/PushNotificationsRT.h
* File built with Microsoft(R) MIDLRT Compiler Engine Version 10.00.0231
*/

#pragma warning( disable: 4049 ) /* more than 64k source lines */
Copy link
Copy Markdown
Member

@hulumane hulumane Sep 3, 2021

Choose a reason for hiding this comment

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

I'm assuming that the header isn't getting generated for some reason from the udk? Do we know when the udk fix is coming?

Copy link
Copy Markdown
Contributor Author

@loneursid loneursid Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No it isn't. It was generated by the build process that built the UDK but the contents won't change until the UDK gets updated.

We don't know when the UDK will get ingested. It's completely out of our hands at this point. It's guarantee to hapen before Reunion v1.0 launches though.

The fix has been checked-in into the rs_wdx_dxp_ixp1 branch and cherry-picked into ni_release_svc_reunion21110. From there the UDK must be taken and ingested into Project reunion.

@loneursid loneursid merged commit 0e1661e into WNP_LRP Sep 3, 2021
@loneursid loneursid deleted the WNP_LRP-CrisisOverride branch September 3, 2021 19:11
@loneursid loneursid mentioned this pull request Sep 15, 2021
Merged
loneursid added a commit that referenced this pull request Oct 13, 2021
@danielayala94 @sharath2727
@pmpurifoy @loneursid
Push Notifications Long Running Process (LRP) lifetime impl (#1186)
dbbfb8b 
* Original skeleton

* Corrected mismatches against PowerNotifications change

* Correcting few nits

* Added constants usage, std::call_once and improved abstraction

* Added StartupTask exe

* Fixed some bugs

* Mostly renaming and cleaning

* Removed timer externals and let winmain to own Platform lifetime

* Correction -> factory should own the platform

* Addressed lifetime crashes and other issues

* Improved timer/event management

* Addressed feedback

* Fixed compilation issues in other flavor+archs and added minor comments

* Added a unit test (more coming!)

* Test enhancements

* Addressed comments

* Moved timer/event logic to a platform component

* Install Frameworkudk for LRP (#1271)

* Introducing unpackaged foreground activation for PushNotifications (#1336)

* Cleaned original code

* Added IWpnNotificationSink COM interface to LRP MSIX

* Just stuff for testing. Needs to be undone

* Still working on it

* Com proxy objects not being stored correctly

* Receiving Data from LRP and changed sink container

* Foreground activation working

* Fix first round of nits

* Still working on adding nits

* Fix following FI of main

* Addressing nits

* Use com_array instead of byte*

* Update dev/PushNotifications/PushNotificationDummyDeferral.h

Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

Co-authored-by: Daniel Ayala <joayalag@microsoft.com>
Co-authored-by: Eric Langlois <erlangl@microsoft.com>
Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

* Not everything should be renamed Runtime

* Ensuring the LRP msix gets built after the framework (#1356)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Bringing build management settings for release/arm64 in line with debug/arm64

* Fixing arm64 build

* Addressed comments

* Using winrt constructs on NotificationsLongRunningPlatformImpl :)

* Channel Request for unpackaged applications (#1290)

* Channel requests for Packaged and Unpackaged applications

* Fix comments

* Update windowsappsdk_dll to locate PushNotificationsLongRunningTask.ProxyStub headers

* Address minor comments

* Fix channel.Close API to handle channels when requested through frameworkudk

* Address comments and introduce GetAppIdentifier in LRP

* Addressing nits

* Fixing a few more nits

* Update externs.h

* Store <processname,appidentifier> pair in windows storage localsettings

* Fix minor nits

* Remove helpers.h references from PushNotificationsDemoApp

* Remove unused APIs

* Using WIL type for CoInitializeEx instead of working too hard

Co-authored-by: Eric Langlois <erlangl@microsoft.com>
Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com>
Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

* Notification delivery + Protocol Activation (#1358)

* Protocol Activation skeleton

* Further impl, mostly on the LRP

* Integration with Foreground logic and some optimizations

* Added calls to the LRP from SDK, and LRP improvements

* Fixed some issues

* Improved payload deserializing

* Fixed merging goofs

* Addressed feedback + few optimizations

* Further improvements and added/removed comments

* Addressed build errors after merging

* Nit: Removed comment

Co-authored-by: Daniel Ayala
Co-authored-by: Eric Langlois

* Unpackaged notification unit tests (#1340)

* Adding initial unpackaged test series

* This function now fails silently

* This is not expected to work for unpackaged apps

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Ensure errors get logged with telemetry

* Workaround UDK include issue (#1364)

* Workaround UDK include issue

* just duplicating files seem a better approach

* Restoring vcproje files

* PushNotificationsRT.h was interfering with package creation

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Some nits

* Addressed comments

* Fixed mistake - Removed comments

* Add LRP binaries to CopyFilestoStagingDir

* Addressed comments

* Merge main to LRP

* Telemetry Update for Unpackaged Apps (#1385)

* Adding support for unpackaged apps

* Using inline statics to cache data

* Code cleanup

* Code cleanup

* Simplifying rethrow code

* Addressing feedback

* Distinguishing between legacy and modern impl

* caching legacy status to it is also available when the call fails

* Adding const to better express the intent

* Code cleanup

* Code cleanup

* PR feedback

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Update to latest UDK and undo UDK hack (#1420)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Fixing WNP_LRP tests  (#1401)

* Working on tests

* Call ReigsterFullTrust in RegisterLongRunningActivator

* Removed g_isPackaged with IsActivatorSupported

* Addressing nits

* Protocol Activation bug fixes (#1406)

This PR fixes a couple of issues:

- Truncated payloads delivered to the app: the Push payload is now processed correctly to comply with AppLifecycle command line expectations (i.e. truncation happened at the first space character - other activation types are already escaping such characters, now we do it too. Then, the payload is unescaped at GetRawNotificationEventArgs.h).
- Payloads with special characters crashed the app: Special characters weren't playing okay with wstring->string conversion at PushNotificationReceivedEventArgs, because WideCharToMultiByte was erroring out with ERROR_INSUFFICIENT_BUFFER.

* Use remoteId as const value (#1435)

* Upgrade LRP to latest UDK (#1459)

* Upgrade LRP to latest UDK

* Missed a few checks in the vcproj

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* fix bad merge

* Update CopyFilesToStagingDir.ps1

* Remove PushNotifications from outer layer of JSON

* Adding source link to projects not in main yet (#1515)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* Support foregroundtask registration for packaged applications (#1521)

* Support foregroundtask registration for packaged applications

* Address comments

* Address comments2

* Introduce PushNotificationsUtility.h/cpp

* Address comments

* Mark PushNotificationUtility.h file to be shared across the PushNotifications directory

* Add try/catch around onRawNotificationRecveived

* Set foregroundhandled to true by default

* Address comments for getappusermodelId API

* Address nits

* Adress one more nit

Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com>

* Fix for regression in protocol activation in AppLifecycle (#1558)

* Hack to address AppLifecycle regression introduced in main

* Code cleanup

* Code cleanup

* Addressing PR feedback

* Adding bare bone unit test to avoid future regression

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

* LRP E2E Scenario fixes (#1543)

* LRP fixes

* In progress

* Revert "In progress"

This reverts commit d5d021a.

* Add fixes to E2E testing

* Address nits

* Added nits from other PR and fixed PushNotificationUtiilty

* Use string converter with payload length

* Update APITests.cpp

* Include AppLifecycle fix for PushArgs

* Revert "Include AppLifecycle fix for PushArgs"

This reverts commit 33994da.

* Addressing nits

* Add Verify.h to demo app for helper function

* Added TAEF dependencies to Demo/TestApp

Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

* Updating LRP to use latest FrameworkUDK (#1587)

Co-authored-by: Eric Langlois <erlangl@microsoft.com>

Co-authored-by: Daniel Ayala <joayalag@microsoft.com>
Co-authored-by: Sharath Manchala <10109130+sharath2727@users.noreply.github.com>
Co-authored-by: Eric Langlois <erlangl@microsoft.com>
Co-authored-by: Paul Purifoy <33183370+pmpurifoy@users.noreply.github.com>
Co-authored-by: eric langlois <email@ericlanglois.com>
Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com>
Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielayala94 danielayala94 danielayala94 approved these changes

@hulumane hulumane hulumane approved these changes

@sharath2727 sharath2727 Awaiting requested review from sharath2727

@pmpurifoy pmpurifoy Awaiting requested review from pmpurifoy

@DrusTheAxe DrusTheAxe Awaiting requested review from DrusTheAxe

@jonwis jonwis Awaiting requested review from jonwis

Assignees

No one assigned

Labels

area-Notifications Toast notification, badges, Live Tiles, push notifications needs-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@loneursid @danielayala94 @hulumane