[tensorflow-cc] add support for x64-uwp

[ianier]

Feature request: add support for the x64-uwp triplet to the 2.x tensorflow-cc port.

The expected benefit is to be able to use TF, at least for inference, on Store apps.

It looks like this is realistically feasible (see PR #13028). The resulting TF library may not support all features, though (e.g. those relying on file system access).

[ianier]

I'm looking myself into this. There are no major issues with the code itself that prevent it from running under WinRT, and the missing features won't be showstoppers. I can already share a list of impacted code files and the possible workarounds.

However, the challenge is that some external dependencies (protoc.exe, grpc_cpp_plugin.exe, nasm.exe, def_file_filter.exe), which are a byproduct of the TF build process, are used during the build process itself. This means that these dependencies shouldn't target uwp, but the host config (say x64). .

What I don't know, though, is whether some of the objects built as part of these dependencies are also linked into the final output. If so, we'd have to find a way around this issue. If not, we need to figure out how to pass the right build switches only to the final output (tensorflow_cc.dll) and not the external deps.

[ianier]

What I don't know, though, is whether some of the objects built as part of these dependencies are also linked into the final output.

They are, but luckily the TF build framework already addresses this issue. The switch --distinct_host_configuration does exactly what we want (see https://docs.bazel.build/versions/master/guide.html). Setting it to true allows us to use different configs for the compiler and linker, via --host_copt and --host_linkopt when building the tools that are used in the build process itself. Note that the default value for --distinct_host_configuration is true, but TF sets it to false for Windows builds in the .bazelrc config file, as it assumes that there’s never a need for cross-compilation on Windows.

Now, regarding the source code, as mentioned in #13028, we have to compile with -DWINAPI_FAMILY=WINAPI_FAMILY_APP and -D_WIN32_WINNT=0x0A00, and link with /APPCONTAINER.

It’s worth noting that a couple of TF dependencies already support UWP. So, if we assume that these dependencies are used only to build the main library and not the build tools themselves, we can just configure them, specifically:

• SQLite (.bzl_bazel_devtools\ozkcue5a\execroot\org_tensorflow\external\org_sqlite): in BUILD.bazel, set "-DSQLITE_OS_WINRT=1"
• CURL (.bzl_bazel_devtools\ozkcue5a\execroot\org_tensorflow\external\curl): in BUILD.bazel, remove the #defines USE_WINDOWS_SSPI and USE_SCHANNEL
• ICU (.bzl_bazel_devtools\ozkcue5a\execroot\org_tensorflow\external\icu): in BUILD.bazel, set "-DU_PLATFORM_HAS_WINUWP_API=1" (however, file udata.cpp also has to be patched to avoid the call to setCommonICUDataPointer when U_PLATFORM_HAS_WINUWP_API is defined; this is a bug in ICU)

That’s all for the projects that support UWP, so these will build just fine.

Regarding the rest of the code:

std::getenv isn’t available on UWP and TF uses it in many places. We could just create a function with the same signature that returns nullptr and put it in an include file somewhere. Similarly, no-op versions of the many unsupported Windows functions could be also implemented.

So, eventually it all builds, but I’m nowhere near a final solution: the final DLL is linked against the desktop CRT, not the WinRT one, so I guess I’ll have to build with the /ZW switch or find another solution. More importantly, my main use case fails when loading a frozen graph from memory.

I’ll continue to play with this as I want a UWP TF port so badly. However, given the extent of the patches to be applied, I recognize this would be much better addressed by the upstream project(s) than by vcpkg.

Edit: formatting

[ianier]

I recognize this would be much better addressed by the upstream project(s) than by vcpkg.

So I also opened an issue in the TF repository.

[ianier]

More importantly, my main use case fails when loading a frozen graph from memory.
This was just a silly config bug, not an actual issue. The resulting DLL works fine with my test model.

Here's a summary of my current approach:

• Modify the three BUILD.bazel configs as explained above (including patching udata.cpp).
• Compile with -DWINAPI_FAMILY=WINAPI_FAMILY_APP and -D_WIN32_WINNT=0x0A00, and link with /APPCONTAINER and WindowsApp.lib. I added all these options to the .bazelrc config file during development, and of course set --distinct_host_configuration=true
• Redirect windows.h to a header file that implements the missing UWP bits as no-op.
• Patch \execroot\org_tensorflow\external\aws\aws-cpp-sdk-core\source\platform\windows\FileSystem.cpp to include the redirected windows.h
• Redirect cstdlib to a header file that implements std::getenv() by always returning nullptr.

During development, to redirect the headers I modified the actual header files in VC++/Windows SDK on a dev PC. @jgehw : Can you suggest a clean solution to achieve something similar from within vcpkg or bazel, just for building TF? There are way too many files impacted (including external TF deps) to patch them one by one.

The only remaining issue at the moment is the linkage with the desktop CRT (i.e. it should use the libs at VC\Tools\MSVC\14.27.29110\lib\x64\store instead of the ones at VC\Tools\MSVC\14.27.29110\lib\x64). I tried changing the LIB environment variable but unfortunately it didn't help.

[ianier]

The only remaining issue at the moment is the linkage with the desktop CRT

I finally solved this by setting the /LIBPATH linker command-line switch to point to the 'store' version of the CRT libs. Now I have a UWP-compatible TF DLL, which passes the local App Cert Kit validation. My next step is to test it and, if it works, publish a real app in the Store using it.

For the record, Bazel calls VCVARSALL.BAT in windows_cc_configure.bzl to set the INCLUDE/LIB environment vars. While VCVARSALL.BAT supports specifying the environment type (e.g. VCVARSALL.BAT amd64 store), it looks like this is not foreseen in the current implementation of windows_cc_configure.bzl. Hopefully someone will look into this at tensorflow/tensorflow#44463

[jgehw]

During development, to redirect the headers I modified the actual header files in VC++/Windows SDK on a dev PC. @jgehw : Can you suggest a clean solution to achieve something similar from within vcpkg or bazel, just for building TF? There are way too many files impacted (including external TF deps) to patch them one by one.

I think the best point to address this would be where bazel interacts with VCVARSALL.BAT. An easier dirty workaround might be using the compiler command line switches /X (to ignore the default include path) and /I (to add an include path with a patched version).

[ianier]

My next step is to test it...

It works! I'm running the Spleeter 2 & 4-stem models for audio source separation, which are BTW pretty demanding, in a UWP app. It's worth noting that the 'Code Generation' capability needs to be checked in the app manifest.

The ultimate validation will be getting the app update published in the Store.

I think the best point to address this would be where bazel interacts with VCVARSALL.BAT

Sure it is. However, it's really difficult, as my understanding is that windows_cc_configure.bzl is part of the bazel distribution, and the changes required aren't trivial.

@jgehw : It's been great to get my problem solved thanks to you! It would be a pity (and selfish) to stop here, though.
Based on what I've documented so far, do you believe it's feasible, or worth it, to do this in vcpkg? I will of course provide line numbers, file paths, content of the patched header files, etc.

[jgehw]

Sure it is. However, it's really difficult, as my understanding is that windows_cc_configure.bzl is part of the bazel distribution, and the changes required aren't trivial.

Yes, patching bazel is no fun... That's why I was also working with a workaround for x64-windows in the portfile until my PR in the bazel repo made it into the release.

@jgehw : It's been great to get my problem solved thanks to you! It would be a pity (and selfish) to stop here, though.
Based on what I've documented so far, do you believe it's feasible, or worth it, to do this in vcpkg? I will of course provide line numbers, file paths, content of the patched header files, etc.

If I didn't miss something, beside patching headers and passing various compile/link options, you didn't need to patch anything in the bazel distribution, just bazel files in the build tree, right? Yes, then it should be feasible to do this in vcpkg, please share.

[ianier]

If I didn't miss something, beside patching headers and passing various compile/link options, you didn't need to patch anything in the bazel distribution, just bazel files in the build tree, right? Yes, then it should be feasible to do this in vcpkg, please share.

Right. Here's the full process:

• Out of laziness, do a 'vcpkg install tensorflow-cc:x64-windows' first and wait until done, just to get the build tree. I'm patching only the Release build, as I believe that the Debug build would be pointless under UWP.
• Patch the file \vcpkg\buildtrees\tensorflow-cc\x64-windows-rel.bazelrc as follows:
  Replace the line 'build:windows --distinct_host_configuration=false' (line 321) by the following block (the VC and SDK versions will need to be replaced with the actual ones):
  build:windows --distinct_host_configuration=true
  build:windows --copt=-DWINAPI_FAMILY=WINAPI_FAMILY_APP
  build:windows --copt=-D_WIN32_WINNT=0x0A00
  build:windows --linkopt=/APPCONTAINER
  build:windows --linkopt=WindowsApp.lib
  build:windows --linkopt=""/LIBPATH:C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.27.29110\lib\x64\store""
  build:windows --linkopt=""/LIBPATH:C:\Program Files (x86)\Windows Kits\10\lib\10.0.18362.0\um\x64""
  build:windows --linkopt=/FORCE:MULTIPLE
  build:windows --host_copt=-UWINAPI_FAMILY
  build:windows --host_linkopt=/APPCONTAINER:NO
• Patch the file \vcpkg\buildtrees.bzl_bazel_currentuser\hash\execroot\org_tensorflow\external\org_sqlite\BUILD.bazel by adding in line 16 the following, including the quotes and the comma (note the user name and hash in my file path corresponding to the Release build tree):
  "-DSQLITE_OS_WINRT=1",
• Patch the file \vcpkg\buildtrees.bzl_bazel_currentuser\hash\execroot\org_tensorflow\external\curl\BUILD.bazel to remove or comment out lines 475 & 477 ("#define USE_WINDOWS_SSPI 1" and "#define USE_SCHANNEL 1").
• Patch the file \vcpkg\buildtrees.bzl_bazel_currentuser\hash\execroot\org_tensorflow\external\icu\BUILD.bazel by adding in line 47 the following:
  "-DU_PLATFORM_HAS_WINUWP_API=1",
• Patch the file \vcpkg\buildtrees.bzl_bazel_currentuser\hash\execroot\org_tensorflow\external\icu\icu4c\source\common\udata.cpp by moving the end of the comment from line 711 to line 715 (i.e. to comment out the call to setCommonICUDataPointer)
• Patch the file \vcpkg\buildtrees.bzl_bazel_currentuser\hash\execroot\org_tensorflow\external\aws\aws-cpp-sdk-core\source\platform\windows\FileSystem.cpp to include <windows.h> somewhere (e.g. line 23)
• Find a way to force the build to use the attached windows.h (which references uwppatch.h) and cstdlib instead of the default ones.

That's all. Now rebuilding the Release build using the bash script you provided at #13028 (comment) yields a uwp-ready tensorflow-cc.dll.

@jgehw : Sounds feasible?

[jgehw]

@jgehw : Sounds feasible?

Thanks for the details. Directly patching files being present after extracting or configuring is straight-forward. Some files of the external dependencies will only be created during the bazel build process; beside a really ugly hack (run bazel twice, patch in between), patching bazel scripts to apply patches during the bazel process, is a bit more complicated but still sounds feasible with passable effort. There is a bit of uncertainty yet concerning the patched version of windows.h: either my /X /I idea works, then it's straight forward; otherwise we'll have some fun here... We'll see.

[ianier]

The ultimate validation will be getting the app update published in the Store.

Validated! The app release is now in the Store. I also wrote a short blog post about the feature (the app isn't free, but has a fully-functional trial).

@jgehw : A stretch goal could be to get the UWP DLL to compile for ARM64 too. I have hardware to test it on.

[jgehw]

I implemented the patches you described. As I'm probably missing some UWP tools on my machine (detect-compiler for UWP fails), I just created a work-in-progress PR to see what happens on CI.

@jgehw : A stretch goal could be to get the UWP DLL to compile for ARM64 too. I have hardware to test it on.

Looking at https://github.com/bazelbuild/bazel/releases there doesn't seem to be ARM support for Windows, only Linux...

[ianier]

Looking at https://github.com/bazelbuild/bazel/releases there doesn't seem to be ARM support for Windows, only Linux...

@jgehw : I meant cross-compiling to ARM64 target on an x64 host, now that we have set --distinct_host_configuration=true. All this would be better addressed in bazel itself, though, so at some point I'll raise the question there (i.e. support for ARM64 as an architecture on Windows and UWP as a platform).

[ianier]

cross-compiling to ARM64 target on an x64 host

@jgehw: I gave it a try and found out that they assume pretty much all over the TF .bzl files that the only possible target on Windows is x64. However, Bazel does support ARM64 as a compilation target through '--cpu=x64_arm64_windows', so I opened tensorflow/tensorflow#45151 for this purpose.