Add OSS support #199

zou000 · 2019-04-17T18:25:07Z

yongtang · 2019-04-18T16:44:12Z

@zou000 Can you move the python test file to tests/ directory? In this way it could be automatically picked up by pytest.

terrytangyuan

Looks great in general. Just left a couple small comments. Perhaps this could be some follow-up work but does OSS provide any mocking utilities similar to moto for S3 so we could write some simple unit tests?

tensorflow_io/oss/kernels/ossfs/oss_file_system.cc

tensorflow_io/oss/README.md

yongtang · 2019-04-18T21:56:32Z

If mocking is not possible, maybe some simple sanity test to make sure at least it build and runs, e.g., access some publicly accessible "hello world" file on OSS?

zou000 · 2019-04-30T02:05:40Z

@zou000 Can you move the python test file to tests/ directory? In this way it could be automatically picked up by pytest.

Done.

zou000 · 2019-04-30T03:37:24Z

Looks great in general. Just left a couple small comments. Perhaps this could be some follow-up work but does OSS provide any mocking utilities similar to moto for S3 so we could write some simple unit tests?

If mocking is not possible, maybe some simple sanity test to make sure at least it build and runs, e.g., access some publicly accessible "hello world" file on OSS?

I asked around, unfortunately, it seems there is neither an OSS mocking framework nor public accessible OSS files for tests. For example the popular OSS java sdk also rely on user's own credentials for tests: https://github.com/aliyun/aliyun-oss-java-sdk/blob/master/src/test/java/com/aliyun/oss/testing/ConcurrencyTest.java#L39 I made a change to skip OSS test cases if the env variables are absent. This is not ideal but at least it can test
the OSS extension can be imported.

terrytangyuan · 2019-04-30T08:37:22Z

Thanks! I think it’s fine for now. Could you fix the Travis build?

zou000 · 2019-05-01T16:28:38Z

Thanks! I think it’s fine for now. Could you fix the Travis build?

Travis CI needs to be configured to use some specific bazel flags, as described in detail in #200. I am not familiar with your travis setup and I am wondering if you or @yongtang can make a quick change? Thanks.

yongtang · 2019-05-02T15:06:35Z

@zou000 the docker image of custom-op actually uses standalone mode in bazel which might create unintended consequences (something to be fixed). It relies on environment installation which could vary a lot.

In tensorflow-io, we use sandbox mode to make sure all dependency binaries are specified.

I have created a patch that should fixed the build failures. Can you apply the patch:

From dbf53e2d74e3eb90de99895c9aa83a6e9b05f9d2 Mon Sep 17 00:00:00 2001
From: Yong Tang <[email protected]>
Date: Thu, 2 May 2019 14:58:54 +0000
Subject: [PATCH] Add libexpat and fixed several dependency issues

Signed-off-by: Yong Tang <[email protected]>
---
 WORKSPACE                     | 10 ++++++++++
 third_party/libaprutil1.BUILD |  7 +++++--
 third_party/libexpat.BUILD    | 30 ++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 third_party/libexpat.BUILD

diff --git a/WORKSPACE b/WORKSPACE
index f105f64..0376dea 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -290,6 +290,16 @@ http_archive(
     ],
 )

+http_archive(
+    name = "libexpat",
+    build_file = "//third_party:libexpat.BUILD",
+    sha256 = "574499cba22a599393e28d99ecfa1e7fc85be7d6651d543045244d5b561cb7ff",
+    strip_prefix = "libexpat-R_2_2_6/expat",
+    urls = [
+        "http://github.com/libexpat/libexpat/archive/R_2_2_6.tar.gz",
+    ],
+)
+
 http_archive(
     name = "libapr1",
     build_file = "//third_party:libapr1.BUILD",
diff --git a/third_party/libaprutil1.BUILD b/third_party/libaprutil1.BUILD
index 6545137..ad9b6ac 100644
--- a/third_party/libaprutil1.BUILD
+++ b/third_party/libaprutil1.BUILD
@@ -4,7 +4,10 @@ licenses(["notice"])  # Apache license (for libapr1)

 cc_library(
     name = "libaprutil1",
-    srcs = [
+    srcs = glob([
+        "include/private/apu_*.h",
+        "include/private/apr_*.h",
+    ]) + [
         "buckets/apr_brigade.c",
         "buckets/apr_buckets.c",
         "buckets/apr_buckets_alloc.c",
@@ -112,10 +115,10 @@ cc_library(
         "-lrt",
         "-lcrypt",
         "-lpthread",
-        "-lexpat",
     ],
     strip_include_prefix = "include",
     deps = [
         "@libapr1",
+        "@libexpat",
     ],
 )
diff --git a/third_party/libexpat.BUILD b/third_party/libexpat.BUILD
new file mode 100644
index 0000000..f0e617c
--- /dev/null
+++ b/third_party/libexpat.BUILD
@@ -0,0 +1,30 @@
+# Description:
+#   Expat library
+
+licenses(["notice"])
+
+exports_files(["COPYING"])
+
+cc_library(
+    name = "libexpat",
+    srcs = [
+        "lib/xmlparse.c",
+        "lib/xmlrole.c",
+        "lib/xmltok.c",
+    ],
+    hdrs = glob([
+        "lib/*.h",
+    ]) + [
+        "lib/xmltok_impl.c",
+        "lib/xmltok_ns.c",
+    ],
+    copts = [
+        "-DHAVE_MEMMOVE",
+        "-DXML_POOR_ENTROPY",
+    ],
+    includes = [
+        "lib",
+    ],
+    visibility = ["//visibility:public"],
+    deps = [],
+)
--
2.17.2

Also see attachment:
0001.patch.txt

yongtang · 2019-05-02T15:06:49Z

@zou000 With the above the build should pass.

terrytangyuan · 2019-05-02T15:08:44Z

@yongtang Perhaps let's get that patch PR'ed into master first. Otherwise the commit history here might be messed up and we might encounter issues with the CLA.

yongtang · 2019-05-02T15:58:13Z

@zou000 The PR #210 has been merged. You can rebase the master and make the following changes in libaprutil1.BUILD:

diff --git a/third_party/libaprutil1.BUILD b/third_party/libaprutil1.BUILD
index 6545137..ad9b6ac 100644
--- a/third_party/libaprutil1.BUILD
+++ b/third_party/libaprutil1.BUILD
@@ -4,7 +4,10 @@ licenses(["notice"])  # Apache license (for libapr1)
 
 cc_library(
     name = "libaprutil1",
-    srcs = [
+    srcs = glob([
+        "include/private/apu_*.h",
+        "include/private/apr_*.h",
+    ]) + [
         "buckets/apr_brigade.c",
         "buckets/apr_buckets.c",
         "buckets/apr_buckets_alloc.c",
@@ -112,10 +115,10 @@ cc_library(
         "-lrt",
         "-lcrypt",
         "-lpthread",
-        "-lexpat",
     ],
     strip_include_prefix = "include",
     deps = [
         "@libapr1",
+        "@libexpat",
     ],
 )

zou000 · 2019-05-02T18:38:43Z

@zou000 The PR #210 has been merged. You can rebase the master and make the following changes in libaprutil1.BUILD:
Thanks @yongtang and @terrytangyuan for looking into this and adding libexpat library. I made the change and I think it should pass CI this time.

Like @yongtang mentioned above it would be great to have a development docker container consistent with CI and release environment. #203 introduced 2 changes that accidentally made OSS work. Besides the libexpat library, the image also contains a /etc/bazelrc file that circumvented a couple of bazel-in-docker bugs. @yongtang's suggested patch also circumvented this bug, but in a different way. Personally, I think the bazelrc way is better as it makes development the same in docker and on host. Though I will leave it to you guys to decide.

I will update #200 with the info and close it.

terrytangyuan

Thanks for the efforts! @zou000 I'll merge this once the CI passes.

yongtang · 2019-05-02T20:31:22Z

@zou000 @terrytangyuan Looks like there are still some issues on certain platforms. I think you could remove -lexpat and -lcrypt, and add boringssl as the dependency

In other word, apply the following patch:

diff --git a/third_party/libaprutil1.BUILD b/third_party/libaprutil1.BUILD
index 038a361..3b0d5d0 100644
--- a/third_party/libaprutil1.BUILD
+++ b/third_party/libaprutil1.BUILD
@@ -113,13 +113,12 @@ cc_library(
     ],
     linkopts = [
         "-lrt",
-        "-lcrypt",
         "-lpthread",
-        "-lexpat",
     ],
     strip_include_prefix = "include",
     deps = [
         "@libapr1",
         "@libexpat",
+        "@boringssl//:ssl",
     ],
 )

zou000 · 2019-05-02T20:37:53Z

Thanks and done. It would be great if we have a whitelist of allowed dynamic libraries available on systems and a submit checker to enforce it. -- Li

…

On Thu, May 2, 2019 at 1:31 PM Yong Tang ***@***.***> wrote: @zou000 <https://github.com/zou000> @terrytangyuan <https://github.com/terrytangyuan> Looks like there are still some issues on certain platforms. I think you could remove -lexpat and -lcrypt, and add boringssl as the dependency In other word, apply the following patch: diff --git a/third_party/libaprutil1.BUILD b/third_party/libaprutil1.BUILD index 038a361..3b0d5d0 100644--- a/third_party/libaprutil1.BUILD+++ b/third_party/libaprutil1.BUILD@@ -113,13 +113,12 @@ cc_library( ], linkopts = [ "-lrt",- "-lcrypt", "-lpthread",- "-lexpat", ], strip_include_prefix = "include", deps = [ ***@***.***", ***@***.***",+ ***@***.***//:ssl", ], ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#199 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADV3ANI6U7MXQGYX3MUMXLPTNFRZANCNFSM4HGWW26A> .

…MacOS

yongtang · 2019-05-03T19:04:25Z

@zou000 We are almost there. I managed to get MacOS building correctly with additional changes:
2efaf48

The Travis CI passed with the above commit applied on top of your PR:

https://travis-ci.org/yongtang/io/builds/527888963

Can you apply the changes in the commit and update the PR?

zou000 · 2019-05-03T23:58:17Z

Thanks Yong. I was working on the same direction but using conditional macros. It seems working in CI. -- Li

…

On Fri, May 3, 2019 at 12:04 PM Yong Tang ***@***.***> wrote: @zou000 <https://github.com/zou000> We are almost there. I managed to get MacOS building correctly with additional changes: 2efaf48 <2efaf48> The Travis CI passed with the above commit applied on top of your PR: https://travis-ci.org/yongtang/io/builds/527888963 Can you apply the changes in the commit and update the PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#199 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADV3AI6X72T435EATGPXPTPTSEDXANCNFSM4HGWW26A> .

terrytangyuan · 2019-05-04T02:44:52Z

Thanks for all the efforts!

zou000 mentioned this pull request Apr 17, 2019

Strange compilation problem in Docker container #200

Closed

yongtang requested a review from terrytangyuan April 18, 2019 16:28

terrytangyuan reviewed Apr 18, 2019

View reviewed changes

zou000 added 11 commits May 2, 2019 11:06

Add OSS support

8934a0b

* Update WORKSPACE

0420de0

* buildifier

b6a3224

* fix lint

e246e23

* disable invalid names

ba0b68d

* address comments

d5e4c12

Adress comments

1d4f891

* fix lint

5102873

* add BUILD file

4830c0c

* move symbol to top level

ee3362d

* update lib

e0496eb

zou000 force-pushed the oss2 branch from 415055e to e0496eb Compare May 2, 2019 18:19

* remove unused BUILD rules

ab06756

terrytangyuan approved these changes May 2, 2019

View reviewed changes

* remove dependency on pre-installed dynamic libs

472b49b

zou000 added 2 commits May 2, 2019 13:51

* lint

7982d21

* remove apr_passwd.c, which depneds on crypt.h, which is missing on …

1416c4b

…MacOS

zou000 added 2 commits May 3, 2019 12:34

* no need for boringssl

474aff5

* macos

d5b487d

zou000 force-pushed the oss2 branch from 987994b to d5b487d Compare May 3, 2019 22:03

terrytangyuan merged commit 2b4265a into tensorflow:master May 4, 2019

zou000 deleted the oss2 branch May 4, 2019 18:12

i-ony pushed a commit to i-ony/io that referenced this pull request Feb 8, 2021

Add OSS support (tensorflow#199)

fd70940

Add OSS support #199

Add OSS support #199

Uh oh!

Conversation

zou000 commented Apr 17, 2019

Uh oh!

yongtang commented Apr 18, 2019

Uh oh!

terrytangyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yongtang commented Apr 18, 2019

Uh oh!

zou000 commented Apr 30, 2019

Uh oh!

zou000 commented Apr 30, 2019

Uh oh!

terrytangyuan commented Apr 30, 2019

Uh oh!

zou000 commented May 1, 2019

Uh oh!

yongtang commented May 2, 2019

Uh oh!

yongtang commented May 2, 2019

Uh oh!

terrytangyuan commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yongtang commented May 2, 2019

Uh oh!

zou000 commented May 2, 2019

Uh oh!

terrytangyuan left a comment

Choose a reason for hiding this comment

Uh oh!

yongtang commented May 2, 2019

Uh oh!

zou000 commented May 2, 2019 via email

Uh oh!

yongtang commented May 3, 2019

Uh oh!

zou000 commented May 3, 2019 via email

Uh oh!

terrytangyuan commented May 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

terrytangyuan commented May 2, 2019 •

edited

Loading