From 33bf2e77badf4bb2bff76dd993c6a69d6c90a2b3 Mon Sep 17 00:00:00 2001 From: Vo Van Nghia Date: Sun, 3 May 2020 23:49:53 +0700 Subject: [PATCH 1/4] Add windows support for s3 --- .../core/platform/default/build_config.bzl | 4 +++- tensorflow/core/platform/s3/BUILD | 7 ++++++- tensorflow/core/platform/s3/s3_file_system.cc | 9 +++++++- third_party/aws/BUILD.bazel | 21 ++++++++++++++++++- third_party/aws/aws-c-common.bazel | 9 ++++++++ third_party/aws/aws-checksums.bazel | 7 ++++++- 6 files changed, 52 insertions(+), 5 deletions(-) diff --git a/tensorflow/core/platform/default/build_config.bzl b/tensorflow/core/platform/default/build_config.bzl index fa3bd466d8e..33586be9b65 100644 --- a/tensorflow/core/platform/default/build_config.bzl +++ b/tensorflow/core/platform/default/build_config.bzl @@ -658,7 +658,9 @@ def tf_additional_core_deps(): clean_dep("//tensorflow:android"): [], clean_dep("//tensorflow:ios"): [], clean_dep("//tensorflow:linux_s390x"): [], - clean_dep("//tensorflow:windows"): [], + clean_dep("//tensorflow:windows"): [ + clean_dep("//tensorflow/core/platform/s3:s3_file_system"), + ], clean_dep("//tensorflow:no_aws_support"): [], "//conditions:default": [ clean_dep("//tensorflow/core/platform/s3:s3_file_system"), diff --git a/tensorflow/core/platform/s3/BUILD b/tensorflow/core/platform/s3/BUILD index d174b108279..f99e5a0329e 100644 --- a/tensorflow/core/platform/s3/BUILD +++ b/tensorflow/core/platform/s3/BUILD @@ -81,7 +81,12 @@ cc_library( hdrs = [ "s3_file_system.h", ], - deps = [ + deps = select({ + "@org_tensorflow//tensorflow:windows": [ + "//tensorflow/core/platform:retrying_file_system" + ], + "//conditions:default": [], + }) + [ ":aws_crypto", ":aws_logging", "//tensorflow/core:lib", diff --git a/tensorflow/core/platform/s3/s3_file_system.cc b/tensorflow/core/platform/s3/s3_file_system.cc index 02658242ab7..ec03b64b733 100644 --- a/tensorflow/core/platform/s3/s3_file_system.cc +++ b/tensorflow/core/platform/s3/s3_file_system.cc @@ -48,6 +48,13 @@ limitations under the License. namespace tensorflow { namespace { +#ifdef PLATFORM_WINDOWS +// On Windows, `Aws::FileSystem::CreateTempFilePath()` return +// `C:\Users\username\AppData\Local\Temp\`. Adding suffix will cause an error. +static const char* kS3TempFileSuffix = NULL; +#else +static const char* kS3TempFileSuffix = "/tmp/s3_filesystem_XXXXXX"; +#endif static const char* kS3FileSystemAllocationTag = "S3FileSystemAllocation"; static const size_t kS3ReadAppendableFileBufferSize = 1024 * 1024; static const int64 kS3TimeoutMsec = 300000; // 5 min @@ -271,7 +278,7 @@ class S3WritableFile : public WritableFile { transfer_manager_(transfer_manager), sync_needed_(true), outfile_(Aws::MakeShared( - kS3FileSystemAllocationTag, "/tmp/s3_filesystem_XXXXXX", + kS3FileSystemAllocationTag, kS3TempFileSuffix, std::ios_base::binary | std::ios_base::trunc | std::ios_base::in | std::ios_base::out)) {} diff --git a/third_party/aws/BUILD.bazel b/third_party/aws/BUILD.bazel index fd355eeceb1..bb8903eb2ce 100644 --- a/third_party/aws/BUILD.bazel +++ b/third_party/aws/BUILD.bazel @@ -31,6 +31,14 @@ cc_library( "aws-cpp-sdk-core/source/platform/linux-shared/*.cpp", ]), "//conditions:default": [], + }) + select({ + "//conditions:default": glob([ + "aws-cpp-sdk-core/source/net/linux-shared/*.cpp", + ]), + "@org_tensorflow//tensorflow:windows": glob([ + "aws-cpp-sdk-core/source/platform/windows/*.cpp", + "aws-cpp-sdk-core/source/net/windows/*.cpp", + ]), }) + glob([ "aws-cpp-sdk-core/include/**/*.h", "aws-cpp-sdk-core/source/*.cpp", @@ -59,7 +67,6 @@ cc_library( "aws-cpp-sdk-transfer/include/**/*.h", "aws-cpp-sdk-transfer/source/**/*.cpp", "aws-cpp-sdk-core/source/monitoring/*.cpp", - "aws-cpp-sdk-core/source/net/linux-shared/*.cpp", "aws-cpp-sdk-core/source/utils/memory/*.cpp", "aws-cpp-sdk-core/source/utils/crypto/openssl/*.cpp", ]), @@ -94,6 +101,18 @@ cc_library( "ENABLE_CURL_CLIENT", "OPENSSL_IS_BORINGSSL", ], + "@org_tensorflow//tensorflow:windows": [ + "PLATFORM_WINDOWS", + "ENABLE_CURL_CLIENT", + "OPENSSL_IS_BORINGSSL", + ], + "//conditions:default": [], + }), + linkopts = select({ + "@org_tensorflow//tensorflow:windows": [ + "-DEFAULTLIB:Userenv.lib", + "-DEFAULTLIB:Version.lib", + ], "//conditions:default": [], }), includes = [ diff --git a/third_party/aws/aws-c-common.bazel b/third_party/aws/aws-c-common.bazel index f27f50a6eb3..a66fbcb1164 100644 --- a/third_party/aws/aws-c-common.bazel +++ b/third_party/aws/aws-c-common.bazel @@ -28,6 +28,9 @@ cc_library( "@org_tensorflow//tensorflow:raspberry_pi_armeabi": glob([ "source/posix/*.c", ]), + "@org_tensorflow//tensorflow:windows": glob([ + "source/windows/*.c", + ]), "//conditions:default": [], }) + glob([ "source/*.c", @@ -38,6 +41,12 @@ cc_library( "include/**/*.h", "include/aws/common/**/*.inl" ]), + linkopts = select({ + "@org_tensorflow//tensorflow:windows": [ + "-DEFAULTLIB:BCrypt.lib", + ], + "//conditions:default": [], + }), includes = [ "include/", ], diff --git a/third_party/aws/aws-checksums.bazel b/third_party/aws/aws-checksums.bazel index 5aa175795b8..759cb2e6fcf 100644 --- a/third_party/aws/aws-checksums.bazel +++ b/third_party/aws/aws-checksums.bazel @@ -9,7 +9,12 @@ exports_files(["LICENSE"]) cc_library( name = "aws-checksums", - srcs = glob([ + srcs = select({ + "@org_tensorflow//tensorflow:windows": glob([ + "source/visualc/*.c", + ]), + "//conditions:default": [], + }) + glob([ "source/intel/*.c", "source/*.c", ]), From b0d70a0404654f0987b46231845fefc0143a270d Mon Sep 17 00:00:00 2001 From: Vo Van Nghia Date: Mon, 4 May 2020 00:06:53 +0700 Subject: [PATCH 2/4] Fix s3 test on Windows --- tensorflow/core/platform/s3/BUILD | 6 ++++++ tensorflow/core/platform/s3/s3_file_system_test.cc | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tensorflow/core/platform/s3/BUILD b/tensorflow/core/platform/s3/BUILD index f99e5a0329e..ffc9f4075de 100644 --- a/tensorflow/core/platform/s3/BUILD +++ b/tensorflow/core/platform/s3/BUILD @@ -107,6 +107,12 @@ tf_cc_test( tags = [ "manual", ], + args = select({ + "@org_tensorflow//tensorflow:windows": [ + "--dynamic_mode=off", + ], + "//conditions:default": [], + }), deps = [ ":s3_file_system", "//tensorflow/core:lib", diff --git a/tensorflow/core/platform/s3/s3_file_system_test.cc b/tensorflow/core/platform/s3/s3_file_system_test.cc index 95c7467fb74..9d2067dd33b 100644 --- a/tensorflow/core/platform/s3/s3_file_system_test.cc +++ b/tensorflow/core/platform/s3/s3_file_system_test.cc @@ -232,7 +232,7 @@ TEST_F(S3FileSystemTest, HasAtomicMove) { const string fname = TmpDir("HasAtomicMove"); TF_ASSERT_OK(WriteString(fname, "test")); bool has_atomic_move = true; - TF_EXPECT_OK(s3fs.NeedsTempLocation(fname, &has_atomic_move).code()); + TF_EXPECT_OK(s3fs.HasAtomicMove(fname, &has_atomic_move).code()); EXPECT_EQ(has_atomic_move, false); } From 015150ee68a3cc5ed32ef7d55acd52a2b5f1dc92 Mon Sep 17 00:00:00 2001 From: Vo Van Nghia Date: Mon, 4 May 2020 02:53:43 +0700 Subject: [PATCH 3/4] Delete args, fix HasAtomicMove --- tensorflow/core/platform/s3/BUILD | 6 ------ tensorflow/core/platform/s3/s3_file_system.cc | 8 ++++---- tensorflow/core/platform/s3/s3_file_system_test.cc | 2 +- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/tensorflow/core/platform/s3/BUILD b/tensorflow/core/platform/s3/BUILD index ffc9f4075de..f99e5a0329e 100644 --- a/tensorflow/core/platform/s3/BUILD +++ b/tensorflow/core/platform/s3/BUILD @@ -107,12 +107,6 @@ tf_cc_test( tags = [ "manual", ], - args = select({ - "@org_tensorflow//tensorflow:windows": [ - "--dynamic_mode=off", - ], - "//conditions:default": [], - }), deps = [ ":s3_file_system", "//tensorflow/core:lib", diff --git a/tensorflow/core/platform/s3/s3_file_system.cc b/tensorflow/core/platform/s3/s3_file_system.cc index ec03b64b733..1726c9fbc6c 100644 --- a/tensorflow/core/platform/s3/s3_file_system.cc +++ b/tensorflow/core/platform/s3/s3_file_system.cc @@ -50,10 +50,10 @@ namespace tensorflow { namespace { #ifdef PLATFORM_WINDOWS // On Windows, `Aws::FileSystem::CreateTempFilePath()` return -// `C:\Users\username\AppData\Local\Temp\`. Adding suffix will cause an error. -static const char* kS3TempFileSuffix = NULL; +// `C:\Users\username\AppData\Local\Temp\`. Adding template will cause an error. +static const char* kS3TempFileTemplate = nullptr; #else -static const char* kS3TempFileSuffix = "/tmp/s3_filesystem_XXXXXX"; +static const char* kS3TempFileTemplate = "/tmp/s3_filesystem_XXXXXX"; #endif static const char* kS3FileSystemAllocationTag = "S3FileSystemAllocation"; static const size_t kS3ReadAppendableFileBufferSize = 1024 * 1024; @@ -278,7 +278,7 @@ class S3WritableFile : public WritableFile { transfer_manager_(transfer_manager), sync_needed_(true), outfile_(Aws::MakeShared( - kS3FileSystemAllocationTag, kS3TempFileSuffix, + kS3FileSystemAllocationTag, kS3TempFileTemplate, std::ios_base::binary | std::ios_base::trunc | std::ios_base::in | std::ios_base::out)) {} diff --git a/tensorflow/core/platform/s3/s3_file_system_test.cc b/tensorflow/core/platform/s3/s3_file_system_test.cc index 9d2067dd33b..224e30c6bb3 100644 --- a/tensorflow/core/platform/s3/s3_file_system_test.cc +++ b/tensorflow/core/platform/s3/s3_file_system_test.cc @@ -232,7 +232,7 @@ TEST_F(S3FileSystemTest, HasAtomicMove) { const string fname = TmpDir("HasAtomicMove"); TF_ASSERT_OK(WriteString(fname, "test")); bool has_atomic_move = true; - TF_EXPECT_OK(s3fs.HasAtomicMove(fname, &has_atomic_move).code()); + TF_EXPECT_OK(s3fs.HasAtomicMove(fname, &has_atomic_move)); EXPECT_EQ(has_atomic_move, false); } From 24fafc09247564ded4f0b083613afb5bfb3f8b53 Mon Sep 17 00:00:00 2001 From: Vo Van Nghia Date: Tue, 5 May 2020 08:09:56 +0700 Subject: [PATCH 4/4] Fix Ubuntu Sanity --- tensorflow/core/platform/s3/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorflow/core/platform/s3/BUILD b/tensorflow/core/platform/s3/BUILD index f99e5a0329e..6afe943c8e3 100644 --- a/tensorflow/core/platform/s3/BUILD +++ b/tensorflow/core/platform/s3/BUILD @@ -83,7 +83,7 @@ cc_library( ], deps = select({ "@org_tensorflow//tensorflow:windows": [ - "//tensorflow/core/platform:retrying_file_system" + "//tensorflow/core/platform:retrying_file_system", ], "//conditions:default": [], }) + [