New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AVRO-2229] Test using a Docker image #336

Merged
merged 2 commits into from Oct 5, 2018

Conversation

Projects
None yet
4 participants
@Fokko
Copy link
Contributor

Fokko commented Sep 28, 2018

Jira ticket: https://issues.apache.org/jira/browse/AVRO-2229

First version of the Docker image with all the dependencies and the ability to run the full test suite using Docker. I would like to add a Travis CI after this PR: https://issues.apache.org/jira/browse/AVRO-2233

The distribution part is still pending, but before fixing this, I'd like to:

  • Fix the C++ testsuite and enable it back in the testsuite, fixed in #335 :
Run C++ tests
-- Boost version: 1.55.0
-- Found the following Boost libraries:
--   filesystem
--   system
--   program_options
--   iostreams
Enabled snappy codec
-- Configuring done
-- Generating done
-- Build files have been written to: /avro/lang/c++/build
-- Boost version: 1.55.0
-- Found the following Boost libraries:
--   filesystem
--   system
--   program_options
--   iostreams
Enabled snappy codec
-- Configuring done
-- Generating done
-- Build files have been written to: /avro/lang/c++/build
[  1%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Compiler.cc.o
/avro/lang/c++/impl/Compiler.cc:174:15: warning: ‘std::string avro::nameof(const NodePtr&)’ defined but not used [-Wunused-function]
 static string nameof(const NodePtr& n)
               ^
[  2%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Node.cc.o
[  4%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/NodeImpl.cc.o
[  5%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/ResolverSchema.cc.o
[  6%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Schema.cc.o
[  8%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Types.cc.o
[  9%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/ValidSchema.cc.o
[ 10%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Zigzag.cc.o
[ 12%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/BinaryEncoder.cc.o
[ 13%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/BinaryDecoder.cc.o
[ 14%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Stream.cc.o
[ 16%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/FileStream.cc.o
[ 17%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/Generic.cc.o
[ 18%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/GenericDatum.cc.o
[ 20%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/DataFile.cc.o
In file included from /avro/lang/c++/api/Specific.hh:30:0,
                 from /avro/lang/c++/api/DataFile.hh:26,
                 from /avro/lang/c++/impl/DataFile.cc:19:
/avro/lang/c++/api/AvroTraits.hh: In static member function ‘static char (& avro::is_defined<T>::test(char (*)[sizeof (U)]))[1]’:
/avro/lang/c++/api/AvroTraits.hh:61:63: warning: no return statement in function returning non-void [-Wreturn-type]
     template <class U> static yes& test(char(*)[sizeof(U)]) { };
                                                               ^
/avro/lang/c++/api/AvroTraits.hh: In static member function ‘static char (& avro::is_defined<T>::test(...))[2]’:
/avro/lang/c++/api/AvroTraits.hh:63:47: warning: no return statement in function returning non-void [-Wreturn-type]
     template <class U> static no& test(...) { };
                                               ^
/avro/lang/c++/api/AvroTraits.hh: In static member function ‘static char (& avro::is_not_defined<T>::test(char (*)[sizeof (U)]))[1]’:
/avro/lang/c++/api/AvroTraits.hh:81:63: warning: no return statement in function returning non-void [-Wreturn-type]
     template <class U> static yes& test(char(*)[sizeof(U)]) { };
                                                               ^
/avro/lang/c++/api/AvroTraits.hh: In static member function ‘static char (& avro::is_not_defined<T>::test(...))[2]’:
/avro/lang/c++/api/AvroTraits.hh:83:47: warning: no return statement in function returning non-void [-Wreturn-type]
     template <class U> static no& test(...) { };
                                               ^
[ 21%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/parsing/Symbol.cc.o
[ 22%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/parsing/ValidatingCodec.cc.o
[ 24%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/parsing/JsonCodec.cc.o
[ 25%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/parsing/ResolvingDecoder.cc.o
[ 27%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/json/JsonIO.cc.o
[ 28%] Building CXX object CMakeFiles/avrocpp_s.dir/impl/json/JsonDom.cc.o
In file included from /avro/lang/c++/impl/json/JsonDom.cc:19:0:
/avro/lang/c++/impl/json/JsonDom.hh: In instantiation of ‘void std::__cxx1998::vector<_Tp, _Alloc>::_M_insert_aux(std::__cxx1998::vector<_Tp, _Alloc>::iterator, const _Tp&) [with _Tp = avro::json::Entity; _Alloc = std::allocator<avro::json::Entity>; std::__cxx1998::vector<_Tp, _Alloc>::iterator = __gnu_cxx::__normal_iterator<avro::json::Entity*, std::__cxx1998::vector<avro::json::Entity, std::allocator<avro::json::Entity> > >; typename std::__cxx1998::_Vector_base<_Tp, _Alloc>::pointer = avro::json::Entity*]’:
/usr/include/c++/4.9/bits/stl_vector.h:925:28:   required from ‘void std::__cxx1998::vector<_Tp, _Alloc>::push_back(const value_type&) [with _Tp = avro::json::Entity; _Alloc = std::allocator<avro::json::Entity>; std::__cxx1998::vector<_Tp, _Alloc>::value_type = avro::json::Entity]’
/usr/include/c++/4.9/debug/vector:407:22:   required from ‘void std::__debug::vector<_Tp, _Allocator>::push_back(const _Tp&) [with _Tp = avro::json::Entity; _Allocator = std::allocator<avro::json::Entity>]’
/avro/lang/c++/impl/json/JsonDom.cc:72:43:   required from here
/avro/lang/c++/impl/json/JsonDom.hh:64:17: error: non-static const member ‘const size_t avro::json::Entity::line_’, can’t use default assignment operator
 class AVRO_DECL Entity {
                 ^
In file included from /usr/include/c++/4.9/vector:69:0,
                 from /avro/lang/c++/impl/json/JsonDom.hh:26,
                 from /avro/lang/c++/impl/json/JsonDom.cc:19:
/usr/include/c++/4.9/bits/vector.tcc:343:16: note: synthesized method ‘avro::json::Entity& avro::json::Entity::operator=(const avro::json::Entity&)’ first required here 
    *__position = __x_copy;
                ^
make[2]: *** [CMakeFiles/avrocpp_s.dir/impl/json/JsonDom.cc.o] Error 1
CMakeFiles/avrocpp_s.dir/build.make:514: recipe for target 'CMakeFiles/avrocpp_s.dir/impl/json/JsonDom.cc.o' failed
make[1]: *** [CMakeFiles/avrocpp_s.dir/all] Error 2
CMakeFiles/Makefile2:425: recipe for target 'CMakeFiles/avrocpp_s.dir/all' failed
Makefile:147: recipe for target 'all' failed
make: *** [all] Error 2
  • Also I'd like to bump Node from 4.x to 8.x since 4.x has been deprecated, this ties in with #333:
================================================================================
================================================================================

                              DEPRECATION WARNING                            

  Node.js 4.x LTS Argon is no longer actively supported!

  You will not receive security or critical stability updates for this version.

  You should migrate to a supported version of Node.js as soon as possible.
  Use the installation script that corresponds to the version of Node.js you
  wish to install. e.g.

   * https://deb.nodesource.com/setup_8.x — Node.js 8 LTS "Carbon" (recommended)
   * https://deb.nodesource.com/setup_10.x — Node.js 10 Current

  Please see https://github.com/nodejs/Release for details about which
  version may be appropriate for you.

  The NodeSource Node.js distributions repository contains
  information both about supported versions of Node.js and supported Linux
  distributions. To learn more about usage, see the repository:
    https://github.com/nodesource/distributions

================================================================================
================================================================================
  • Fix the Javadoc since this does not allow us to do on Java8, please refer to #334
[AVRO-2229] Test using a Docker image
First version of the Docker image with all the dependencies
and the ability to run the full test suite using Docker

@Fokko Fokko force-pushed the Fokko:AVRO-2229 branch from bc7c733 to 80ee285 Sep 28, 2018

headline "Run C++ tests"
cd /avro/lang/c++
# The current cpp tests are failing:
# https://issues.apache.org/jira/projects/AVRO/issues/AVRO-2230

This comment has been minimized.

@kojiromike

kojiromike Sep 28, 2018

Contributor

It's possible that upgrading to C++11 will also fix this issue. That may be preferable to my fix in #335
Up to you if you want to give it a shot in this PR. https://stackoverflow.com/a/25639772/418413

This comment has been minimized.

@Fokko

Fokko Sep 29, 2018

Contributor

My C++ skills are not so great. After university I haven't done much, so I'm not familiar with the different versions. My preference would be to update #335 after this one gets merged and then enable the test again 👍


headline "Run Java tests"
cd /avro/lang/java
./build.sh test

This comment has been minimized.

@kojiromike

kojiromike Sep 28, 2018

Contributor

What would you say to using a for loop here for all the langs?

This comment has been minimized.

@Fokko

Fokko Sep 29, 2018

Contributor

I've considered it a loop, so let me change it if you prefer a loop 👍

# See the License for the specific language governing permissions and
# limitations under the License.

function headline(){

This comment has been minimized.

@kojiromike

kojiromike Sep 28, 2018

Contributor

It's slightly more portable to omit the function keyword.

This comment has been minimized.

@Fokko

Fokko Sep 29, 2018

Contributor

Good one, thanks

# Install mono modules
RUN mkdir -p /tmp/nunit/ && \
cd /tmp/nunit/ && \
curl -L -s -o nunit.zip https://github.com/nunit-legacy/nunitv2/releases/download/2.7.0/NUnit-2.7.0.zip && \

This comment has been minimized.

@kojiromike

kojiromike Sep 28, 2018

Contributor

One pattern I've found useful in Dockerfile lately is using multistage builds to encapsulate all download/unpack/cleanup in a image before the main build. This means if you make a cache-invalidating change early in the main build, it doesn't have to re-fetch the download. Something like this:

FROM java:8-jdk as fetchcache # Use any base here with curl, zip, tar, etc.
WORKDIR /fetch/
RUN mkdir -p nunit && cd nunit && curl -L -s -o nunit.zip https://github.com/nunit-legacy/nunitv2/releases/download/2.7.0/NUnit-2.7.0.zip && unzip nunit.zip && rm nunit.zip
# ... Ditto for Forrest and other artifacts ...

FROM java:8-jdk as main
...
# Install mono modules
COPY --from=fetchcache /fetch/nunit /tmp/nunit
... etc

This comment has been minimized.

@Fokko

Fokko Sep 29, 2018

Contributor

I'm aware of multistage builds, but I don't think it will add a lot of value in this case. I also had some issues with it on the Dockerhub builder when using multistage. Then the imported file from the earlier stage would be 0bytes.

The reason I don't think it is useful, for the CI we won't cache the image at all. The build.sh docker-test will run on the CI, and for now it doesn't store the image somewhere. For local development, it makes more sense to use build.sh docker. Also, if the Dockerfile isn't changed, the layers are cached and it will just reuse the existing image.


echo "Run Perl tests"
cd /avro/lang/perl
./build.sh test

This comment has been minimized.

@kojiromike

kojiromike Sep 28, 2018

Contributor

Please consider running shellcheck on the shell scripts here. (Granted, it'll find bugs that have been around forever and weren't introduced in this PR, but it's still worth fixing anything relevant here.)

This comment has been minimized.

@Fokko

Fokko Sep 29, 2018

Contributor

Thanks for the tip, I didn't know spellcheck, but it really helps. I've added some double quotes, as suggested by spellcheck.

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Sep 28, 2018

Thank you so much for doing this work.

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Sep 29, 2018

@kojiromike My pleasure, I've updated the PR based on your comments. Let me know what you think.

@kojiromike
Copy link
Contributor

kojiromike left a comment

Besides the parsing-ls issue, this all looks good to me.


set -e

for lang in $(ls /avro/lang/)

This comment has been minimized.

@kojiromike

kojiromike Sep 29, 2018

Contributor

Please write this using a glob expression

for lang in /avro/lang/*/

This comment has been minimized.

@Fokko

Fokko Sep 30, 2018

Contributor

Thanks, just did 👍

@Fokko Fokko force-pushed the Fokko:AVRO-2229 branch from 6a49439 to ce52d96 Sep 30, 2018

@kojiromike
Copy link
Contributor

kojiromike left a comment

:shipit: imo

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Sep 30, 2018

@busbey @nandorKollar @cutting PTAL. Any further suggestions, or it this good to go?

@busbey

This comment has been minimized.

Copy link
Contributor

busbey commented Sep 30, 2018

If I'm going to provide a review you'll have to be extremely patient. I have very limited volunteer time these days and right now it mostly gets used up in email related to the project.

Given the importance of this changeset, I'd expect my turn around time to be 1-2 weeks. That's not to say anyone should block on me; I'm just setting expectations.

@cutting

This comment has been minimized.

Copy link
Contributor

cutting commented Oct 1, 2018

The changes look good to me, but I'm not currently able to test this. Can someone (besides Fokko) verify that, with this patch, tests in all languages now pass under Docker? If so, I'm happy to commit. Thanks!

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 1, 2018

@cutting well, to be strictly correct, we know that C++ tests do not pass, irrespective of this PR. That is AVRO-2230 and fixed in #335 as previously mentioned in @Fokko 's comments. This PR runs the C++ tests, but then continues even when they fail.

Otherwise, I will run the rest of the tests a couple different ways and get back to you.

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 1, 2018

@Fokko Could you please rebase or merge master into this PR so we can have the fix for AVRO-2196?

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 1, 2018

Locally, I rebased master into this branch. Then I ran ./build.sh docker-test. That passed.

Then, I ran ./build.sh docker followed by ./build.sh test inside the container. That failed, due to rat, which complains that BUILD.md does not have a valid license header.

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Oct 2, 2018

@kojiromike This is actually a valid point of the rat checker. The licenses need to be set correctly I think I'll integrate the run-tests.sh in the root build.sh.

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 3, 2018

Bypassing the rat test and all the known-passing tests, I ran the rest of the ./build.sh test in docker as described above. The very last step failed:

+ /bin/bash share/test/interop/bin/test_rpc_interop.sh
TEST: share/test/interop/rpc/add/onePlusOne
18/10/03 18:16:25 INFO util.log: Logging initialized @337ms to org.eclipse.jetty.util.log.Slf4jLog
18/10/03 18:16:25 INFO server.Server: jetty-9.4.z-SNAPSHOT
18/10/03 18:16:25 INFO handler.ContextHandler: Started o.e.j.s.ServletContextHandler@441772e{/,null,AVAILABLE}
18/10/03 18:16:25 INFO server.AbstractConnector: Started ServerConnector@79924b{HTTP/1.1,[http/1.1]}{0.0.0.0:46719}
18/10/03 18:16:25 INFO server.Server: Started @813ms
2
Closing server.
18/10/03 18:16:27 INFO server.AbstractConnector: Stopped ServerConnector@79924b{HTTP/1.1,[http/1.1]}{0.0.0.0:0}
18/10/03 18:16:27 INFO handler.ContextHandler: Stopped o.e.j.s.ServletContextHandler@441772e{/,null,UNAVAILABLE}
18/10/03 18:16:27 INFO util.log: Logging initialized @324ms to org.eclipse.jetty.util.log.Slf4jLog
18/10/03 18:16:27 INFO server.Server: jetty-9.4.z-SNAPSHOT
18/10/03 18:16:28 INFO handler.ContextHandler: Started o.e.j.s.ServletContextHandler@441772e{/,null,AVAILABLE}
18/10/03 18:16:28 INFO server.AbstractConnector: Started ServerConnector@79924b{HTTP/1.1,[http/1.1]}{0.0.0.0:33359}
18/10/03 18:16:28 INFO server.Server: Started @420ms
2
Closing server.
18/10/03 18:16:29 INFO server.AbstractConnector: Stopped ServerConnector@79924b{HTTP/1.1,[http/1.1]}{0.0.0.0:0}
18/10/03 18:16:29 INFO handler.ContextHandler: Stopped o.e.j.s.ServletContextHandler@441772e{/,null,UNAVAILABLE}
18/10/03 18:16:30 INFO util.log: Logging initialized @323ms to org.eclipse.jetty.util.log.Slf4jLog
18/10/03 18:16:30 INFO server.Server: jetty-9.4.z-SNAPSHOT
18/10/03 18:16:30 INFO handler.ContextHandler: Started o.e.j.s.ServletContextHandler@441772e{/,null,AVAILABLE}
18/10/03 18:16:30 INFO server.AbstractConnector: Started ServerConnector@79924b{HTTP/1.1,[http/1.1]}{0.0.0.0:34025}
18/10/03 18:16:30 INFO server.Server: Started @419ms
/home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:127:in `validate_map': undefined method `keys' for nil:NilClass (NoMethodError)
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:103:in `validate_recursive'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:156:in `block in validate_possible_types'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:154:in `map'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:154:in `validate_possible_types'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:141:in `validate_union'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:105:in `validate_recursive'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:110:in `block in validate_recursive'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:108:in `each'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:108:in `validate_recursive'
        from /home/michaels/avro/lang/ruby/lib/avro/schema_validator.rb:67:in `validate!'
        from /home/michaels/avro/lang/ruby/lib/avro/schema.rb:97:in `validate'
        from /home/michaels/avro/lang/ruby/lib/avro/io.rb:503:in `write_data'
        from /home/michaels/avro/lang/ruby/lib/avro/io.rb:499:in `write'
        from /home/michaels/avro/lang/ruby/lib/avro/ipc.rb:136:in `write_handshake_request'
        from /home/michaels/avro/lang/ruby/lib/avro/ipc.rb:105:in `request'
        from lang/ruby/test/tool.rb:69:in `send_message'
        from lang/ruby/test/tool.rb:137:in `main'
        from lang/ruby/test/tool.rb:143:in `<main>'

I don't know if this is reflected in any other ticket.

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Oct 5, 2018

@kojiromike I'm working on integrating the Docker testing with the existing build.sh's. But this requires some more love. At the current state of the PR, with the docker tests we'd only execute the unit tests. I think it is important to also add the interop tests, but this needs some more attention. I hope to finish this the upcoming days.

Recently there was a mail about the Python versions. I've checked and the current CI uses Python 2.7 and Python 3.4. We should decide on Python versions, and add them to the BUILD.md and the setup.py of the specific project to communicate which versions of Python we support.

I've created a Jira to move from the deprecated java docker image to openjdk: https://issues.apache.org/jira/browse/AVRO-2238
Updating is not trivial since the java is based on Debian Jessie, and openjdk is based on Debian Stretch. This will update a lot of different packages, for example, it comes with a newer version of Perl and PHP.

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 5, 2018

I don't think the test_rpc_interop.sh test breakage has anything to do with this PR. I think it's a bug in the ruby implementation. I think this PR is fine and (besides the rat thing) moves the project forward. My vote fwiw is that this changeset get merged in so we can move forward with other incremental fixups.

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 5, 2018

Apologies, I hit enter too soon -- I want to add that I confirmed that the test_rpc_interop.sh fails with the exact same error in master. It has nothing to do with this PR.

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Oct 5, 2018

@kojiromike I fully agree. But instead instead of introducing a new file for Docker testing (https://github.com/apache/avro/pull/336/files#diff-be31a75fe0e789d10685a4ab885644ad), I would prefer to use the current tests: https://github.com/apache/avro/blob/master/build.sh#L40-L75

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Oct 5, 2018

(For your notes in the description, #335 is closed, duplicates work done in #326, which is now merged.)

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Oct 5, 2018

I've created a ticket for fixing the interop tests: https://issues.apache.org/jira/browse/AVRO-2239

@cutting cutting merged commit a11bd49 into apache:master Oct 5, 2018

@Fokko

This comment has been minimized.

Copy link
Contributor

Fokko commented Oct 5, 2018

Thanks for merging @cutting. I was still working on the PR, should have put a [WIP] in the title. I have a follow up PR here: #343

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment