[tensorflow-cc/tensorflow-common] Add uwp port #14394
Conversation
…se customized bazel config is stored in wrong directory
- add patch to fix debug builds - add patch to fix exports for static linking - really build debug (instead of cloning release) - override bazel build options for debug (work around bazel bug) - bazel doesn't support static libraries: work around by building dynamic library and constructing static linkage commands from build log - Windows .pdb file can't be >4GB even on x64: work around using reduced debug information - Windows doesn't support .lib files >4GB even on x64, so split into multiple libs - vcpkg requires equal amount of libs for debug and release: work around using handcrafted empty dummy libs - fix naming of libs (.dll on Windows and .dylib on macOS)
- adapt patch files to tensorflow code changes - update bazel from v0.25.2 to v3.1 - on Windows use python installed on the host instead of embedded python obtained via vcpkg because embedded python lacks pip, which we need to obtain numpy - on Windows add MSYS2 to the PATH so that bazel tools can access MSYS2 GIT - add support for custom CA certificates when using HTTPS_PROXY
The existing implementation totally screwed up commands if the command's arguments contained semicolons (this is the case, e.g., in the FindPython modules of the cmake distribution).
…tually are use cases for this
incorporate changes from microsoft:master
…-more' as required for find_package(Python3)
Revert "incorporate changes from microsoft:master"
…eading the last real libraries contents over the required number of libraries
…/vcpkg into fix-static-builds-and-more
… python) for Windows as we need numpy
|
@PhoebeHui I fixed version files, so also x86 CI errors are solved now. |
PhoebeHui
changed the title
|
I think these changes are not official and we should wait for upstream approval. |
|
@jgehw, for the patch, could you submit an issue or PR to Upstream? |
@PhoebeHui An according issue was already filed as tensorflow/tensorflow#44463 by @ianier several months ago. Upstream said that they don't have enough resources to officially support this, and suggested that we support it via a community build. So, we created this port as a community build. |
|
@PhoebeHui: Come on. If Microsoft shows no commitment to UWP (not even upstream), who will? |
|
@jgehw, thanks for the information! @strega-nil @ras0219-msft, could you help further review? |
There was a problem hiding this comment.
Thanks for this PR!
and suggested that we support it via a community build. So, we created this port as a community build.
I assume that by "community build", they mean that they will accept PRs upstream but won't provide testing resources.
Given the extreme complexity of tensorflow, we're very hesitant to accept large patches like this without some amount of upstream support. To be extra-clear about what we would want to see: we don't need tensorflow to provide builds for arm64 or uwp, but we do need upstream to commit to reviewing and accepting PRs to make it work. Specifically, we need to see most of the patches and changes in this PR made against upstream and merged.
We'd love to see tensorflow on UWP, but we (vcpkg) cannot commit to the complexity required by this PR without upstream willingness to accept contributions.
| message(STATUS "UWP build requires a patched header to be force-included. As this currently is not possible with bazel, we will copy '__vcpkg_uwppatch.h' to '$ENV{VCToolsInstallDir}/include/'. This might require vcpkg to be run as Administrator if the next command fails.") | ||
| configure_file(${CMAKE_CURRENT_LIST_DIR}/uwppatch.h $ENV{VCToolsInstallDir}/include/__vcpkg_uwppatch.h COPYONLY) |
There was a problem hiding this comment.
This is unfortunately against vcpkg policy and cannot be merged as-is due to modifying global system state.
There was a problem hiding this comment.
Ok, I understand. So, as all other approaches I tried didn't work in the end, we need to wait until someone else has a solution or somebody answers the according question I posted for this: https://stackoverflow.com/questions/65184913/how-do-i-add-an-additional-system-include-directory-using-bazel
JackBoosY
added
depends:upstream-changes
The unfortunate fact is that upstream support for Windows features is limited, and it won’t get any better without at least some involvement from Microsoft. This is a chicken-and-egg problem: the platform won’t gain users if the features aren’t there, and developers won’t add features for platforms where there are no users. |
|
As my question on StackOverflow for a clean solution of the bazel issue has continuously been viewed but not answered yet after more than two months, and as my work-around is against vcpkg policy, we can put this PR on hold from my side since there's nothing more I can do about it. At least UWP users can check out this branch and build for the time being. |
|
@ianier I don't currently know of an appropriate contact to put you in touch with unfortunately. Let me restate how I feel we can make progress with this PR:
|
|
@ras0219-msft : As a developer, my incentive here is to get features implemented in my UWP app. I felt vcpkg was a reasonable way to do that and I can’t thank @jgehw enough for getting me there. |
|
@ras0219-msft Some of the changes of this PR won't get merged upstream as they are quick hacks to work around compilation errors, forsaking some minor tensorflow features most people never use. I'm pretty sure, upstream would require clean removal of these un-compilable features by adding compilation switches etc. This involves quite some work. As I'm not a UWP user, and never will be, I can't donate enough time to cover this. After @ianier found a way to make tensorflow compile on UWP, I just incorporated his findings into patches and a vcpkg port. |
And the way things are going, I'm afraid eventually nobody will :-( |
I guess you may close this one on the account of the WinRT debacle. BTW, I just realized that the other 'official' UWP triplet is arm-uwp (32-bit ARM is today a truly dead platform), while arm64-uwp is a community triplet. Go figure. |
|
We have covered arm64-windows and x64-uwp, so I think this is intended, some issues would be detected by arm64-windows, similar to x64-uwp, we covered more arch/platfrom, it would be helpful to track and investgate the issues. |
Implementation of patches discussed in #14252.
I suggest to wait with reviewing until #14392 is merged into master, because this PR builds upon #14392, and once #14392 is merged the file diffs of this PR will collapse to just a few simple changes.
Current state: work-in-progress