Skip to content
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

docker alpine based image #563

Merged
merged 2 commits into from
Jan 28, 2023
Merged

Conversation

StefanSchoof
Copy link
Contributor

In #545 the idea of an alpine based docker image come up.

In #560 a PR from @maxberger adding a alpine based image was added by mistake.

I took this commit from @maxberger and cleaned the file up.

Building with a amd64 image with docker build --pull . is working.

But the arm build docker build --pull --platform=linux/arm/v6 . fails:

------                                                                                  
 > [builder 8/8] RUN cmake -DBUILD_TEST=off     && make     && make install:            
#13 0.639 CMake Warning:                                                                
#13 0.639   No source or binary directory provided.  Both will be assumed to be the
#13 0.639   same as the current working directory, but note that this warning will
#13 0.639   become a fatal error in future CMake releases.
#13 0.639 
#13 0.640 
#13 1.436 -- The C compiler identification is GNU 12.2.1
#13 2.157 -- The CXX compiler identification is GNU 12.2.1
#13 2.261 -- Detecting C compiler ABI info
#13 3.271 -- Detecting C compiler ABI info - done
#13 3.379 -- Check for working C compiler: /usr/bin/cc - skipped
#13 3.381 -- Detecting C compile features
#13 3.386 -- Detecting C compile features - done
#13 3.418 -- Detecting CXX compiler ABI info
#13 4.385 -- Detecting CXX compiler ABI info - done
#13 4.490 -- Check for working CXX compiler: /usr/bin/c++ - skipped
#13 4.492 -- Detecting CXX compile features
#13 4.499 -- Detecting CXX compile features - done
#13 4.506 CMake Deprecation Warning at CMakeLists.txt:3 (cmake_minimum_required):
#13 4.506   Compatibility with CMake < 2.8.12 will be removed from a future version of
#13 4.506   CMake.
#13 4.506 
#13 4.506   Update the VERSION argument <min> value or use a ...<max> suffix to tell
#13 4.506   CMake that the project does not need compatibility with older versions.
#13 4.506 
#13 4.506 
#13 4.714 Compiling for target ''
#13 4.715 -- using gcc compiler GNU
#13 4.718 -- checking if -Wno-ignored-qualifiers works
#13 4.719 -- Performing Test W_NO_IGNORED_QUALIFIERS
#13 5.650 -- Performing Test W_NO_IGNORED_QUALIFIERS - Success
#13 5.655 -- Performing Test HAVE_CPP_REGEX
#13 24.99 -- Performing Test HAVE_CPP_REGEX - Success
#13 25.00 -- FindSml check
#13 25.03 -- Found PkgConfig: /usr/bin/pkg-config (found version "1.9.3") 
#13 25.04 -- Checking for module 'sml>=1.0'
#13 25.07 --   Package 'sml', required by 'virtual:world', not found
#13 25.07 -- SML_HOME env is not set, setting it to /usr/local
#13 25.07 -- Looking for sml in /usr/local
#13 25.08 -- FindMBus check
#13 25.11 -- Checking for module 'libmbus>=0.8.0'
#13 25.24 --   Found libmbus, version 0.9.0
#13 25.63 -- Looking for libmbus in /usr/local/include
#13 25.63 libmbus found: '/usr/local/include'
#13 26.21 -- Found OpenSSL: /usr/lib/libcrypto.so (found version "3.0.7")  
#13 26.22 -- FindMicrohttpd check
#13 26.25 -- Checking for module 'microhttpd>=0.9'
#13 26.28 --   Package 'microhttpd', required by 'virtual:world', not found
#13 26.29 -- MICROHTTPD_HOME env is not set, setting it to /usr/local
#13 26.29 -- Looking for microhttpd in /usr/local
#13 26.29 -- search for libmosquitto returned /usr/lib/libmosquitto.so and /usr/include
#13 26.29 -- libmosquitto found at /usr/lib/libmosquitto.so
#13 26.30 -- Performing Test CMAKE_HAVE_LIBC_PTHREAD
#13 27.26 -- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
#13 27.27 -- Found Threads: TRUE  
#13 27.27 -- FindJson check
#13 27.31 -- Checking for module 'json-c>=0.12'
#13 27.43 --   Found json-c, version 0.16
#13 27.78 -- JSON_HOME env is not set, setting it to /usr/local
#13 27.78 -- Looking for json in /usr/local
#13 27.78 Json-c search: '/usr/local/include;/usr/local/include;/usr/local/include;/usr/include' 
#13 27.78 Json-c found: '/usr/include'
#13 28.50 -- Found CURL: /usr/lib/libcurl.so (found version "7.87.0")  
#13 28.50 -- FindGnuTls check
#13 28.55 -- Checking for module 'gnutls>=2.8'
#13 28.68 --   Found gnutls, version 3.7.8
#13 29.08 -- ==> ''
#13 29.08 -- GNUTLS_HOME env is not set, setting it to /usr/local
#13 29.08 -- Looking for gnutls in /usr/local
#13 29.10 ==> GNUTLS_LIBRARIES='-lgnutls;/usr/lib/libgmp.a;-lunistring;/usr/lib/libatomic.a;-lnettle;-lhogweed;/usr/lib/libgmp.a;-ltasn1;-lp11-kit;-lz;-lsasl2;-lgcrypt'
#13 29.10 -- Found GNUTLS: -lgnutls;/usr/lib/libgmp.a;-lunistring;/usr/lib/libatomic.a;-lnettle;-lhogweed;/usr/lib/libgmp.a;-ltasn1;-lp11-kit;-lz;-lsasl2;-lgcrypt  
#13 29.27 -- Found Git: /usr/bin/git (found version "2.38.2") 
#13 29.32 
#13 29.32         ***** Configuration parameters *****
#13 29.32              prefix: /usr/local
#13 29.32              json: -L/usr/lib/libjson-c.a;-lrt -I/usr/include
#13 29.32              sml:  -L/usr/local/lib/libsml.a;-lrt -I/usr/local/include
#13 29.32              microhttpd: -L/usr/lib/libmicrohttpd.so;-lrt -I/usr/include
#13 29.32              mqtt: -L/usr/lib/libmosquitto.so -I/usr/include
#13 29.32              libmbus: -L/usr/local/lib/libmbus.so;-lm -I/usr/local/include
#13 29.47 -- Configuring done
#13 29.67 -- Generating done
#13 29.69 -- Build files have been written to: /vzlogger
#13 29.74 /usr/bin/cmake -S/vzlogger -B/vzlogger --check-build-system CMakeFiles/Makefile.cmake 0
#13 29.89 /usr/bin/cmake -E cmake_progress_start /vzlogger/CMakeFiles /vzlogger//CMakeFiles/progress.marks
#13 29.98 make  -f CMakeFiles/Makefile2 all
#13 30.02 make[1]: Entering directory '/vzlogger'
#13 30.02 make  -f src/CMakeFiles/vz.dir/build.make src/CMakeFiles/vz.dir/depend
#13 30.05 make[2]: Entering directory '/vzlogger'
#13 30.05 cd /vzlogger && /usr/bin/cmake -E cmake_depends "Unix Makefiles" /vzlogger /vzlogger/src /vzlogger /vzlogger/src /vzlogger/src/CMakeFiles/vz.dir/DependInfo.cmake --color=
#13 30.17 make[2]: Leaving directory '/vzlogger'
#13 30.17 make  -f src/CMakeFiles/vz.dir/build.make src/CMakeFiles/vz.dir/build
#13 30.20 make[2]: Entering directory '/vzlogger'
#13 30.28 [  2%] Building CXX object src/CMakeFiles/vz.dir/Channel.cpp.o
#13 30.29 cd /vzlogger/src && /usr/bin/c++ -DHAVE_CONFIG_HPP -I/vzlogger -I/vzlogger/include -W -Wall -Wextra -Werror -Wnon-virtual-dtor -Wno-system-headers -Wno-deprecated -Wno-deprecated-declarations -Winit-self -Wmissing-include-dirs -Wno-pragmas -Wredundant-decls -Wno-unused-parameter -std=c++11 -fpermissive -Wno-error=redundant-decls -Wno-ignored-qualifiers   -g3 -MD -MT src/CMakeFiles/vz.dir/Channel.cpp.o -MF CMakeFiles/vz.dir/Channel.cpp.o.d -o CMakeFiles/vz.dir/Channel.cpp.o -c /vzlogger/src/Channel.cpp
#13 33.36 In file included from /vzlogger/include/Buffer.hpp:35,
#13 33.36                  from /vzlogger/include/Channel.hpp:32,
#13 33.36                  from /vzlogger/src/Channel.cpp:35:
#13 33.36 /vzlogger/include/Reading.hpp: In member function 'const long int& Reading::time_s() const':
#13 33.36 /vzlogger/include/Reading.hpp:182:30: error: returning reference to temporary [-Werror=return-local-addr]
#13 33.36   182 |                 return _time.tv_sec;
#13 33.36       |                        ~~~~~~^~~~~~
#13 34.43 In file included from /usr/include/c++/12.2.1/list:63,
#13 34.43                  from /vzlogger/include/Buffer.hpp:31:
#13 34.43 /usr/include/c++/12.2.1/bits/stl_list.h: In copy constructor 'std::__cxx11::list<_Tp, _Alloc>::list(const std::__cxx11::list<_Tp, _Alloc>&) [with _Tp = Option; _Alloc = std::allocator<Option>]':
#13 34.43 /usr/include/c++/12.2.1/bits/stl_list.h:814:31: note: parameter passing for argument of type 'std::_List_const_iterator<Option>' changed in GCC 7.1
#13 34.43   814 |       { _M_initialize_dispatch(__x.begin(), __x.end(), __false_type()); }
#13 34.43       |         ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#13 34.50 /usr/include/c++/12.2.1/bits/stl_list.h: In member function 'void std::__cxx11::list<_Tp, _Alloc>::_M_initialize_dispatch(_InputIterator, _InputIterator, std::__false_type) [with _InputIterator = std::_List_const_iterator<Option>; _Tp = Option; _Alloc = std::allocator<Option>]':
#13 34.50 /usr/include/c++/12.2.1/bits/stl_list.h:1929:9: note: parameter passing for argument of type 'std::_List_const_iterator<Option>' changed in GCC 7.1
#13 34.50  1929 |         _M_initialize_dispatch(_InputIterator __first, _InputIterator __last,
#13 34.50       |         ^~~~~~~~~~~~~~~~~~~~~~
#13 34.50 /usr/include/c++/12.2.1/bits/stl_list.h:1929:9: note: parameter passing for argument of type 'std::_List_const_iterator<Option>' changed in GCC 7.1
#13 34.56 /usr/include/c++/12.2.1/bits/stl_list.h: In member function 'void std::__cxx11::list<_Tp, _Alloc>::emplace_back(_Args&& ...) [with _Args = {const Option&}; _Tp = Option; _Alloc = std::allocator<Option>]':
#13 34.56 /usr/include/c++/12.2.1/bits/stl_list.h:1321:26: note: parameter passing for argument of type 'std::_List_iterator<Option>' changed in GCC 7.1
#13 34.56  1321 |           this->_M_insert(end(), std::forward<_Args>(__args)...);
#13 34.56       |           ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#13 34.60 /usr/include/c++/12.2.1/bits/stl_list.h: In member function 'void std::__cxx11::list<_Tp, _Alloc>::_M_insert(iterator, _Args&& ...) [with _Args = {const Option&}; _Tp = Option; _Alloc = std::allocator<Option>]':
#13 34.60 /usr/include/c++/12.2.1/bits/stl_list.h:2003:8: note: parameter passing for argument of type 'std::__cxx11::list<Option>::iterator' changed in GCC 7.1
#13 34.60  2003 |        _M_insert(iterator __position, _Args&&... __args)
#13 34.60       |        ^~~~~~~~~
#13 34.75 cc1plus: all warnings being treated as errors
#13 34.76 make[2]: *** [src/CMakeFiles/vz.dir/build.make:79: src/CMakeFiles/vz.dir/Channel.cpp.o] Error 1
#13 34.76 make[2]: Leaving directory '/vzlogger'
#13 34.76 make[1]: *** [CMakeFiles/Makefile2:921: src/CMakeFiles/vz.dir/all] Error 2
#13 34.76 make[1]: Leaving directory '/vzlogger'
#13 34.76 make: *** [Makefile:169: all] Error 2
------
executor failed running [/bin/sh -c cmake -DBUILD_TEST=off     && make     && make install]: exit code: 2

Anyone an idea?

CC. @isarrider

@isarrider
Copy link
Contributor

Ill have a look when I am back from holiday,
although I only use ARM v8 anymore...
So prob wont be super helpful...

@maxberger
Copy link
Contributor

Agreed, my changing of the docker image to alpine was not ready. I apologize if that came across as a pull request, that was not my intention.

That said, I am very interested in an alpine-based image for arm/v6, since I am running vzlogger on a pizero. So I may as well give this one a shot.

The fix for this error message is:

diff --git a/include/Reading.hpp b/include/Reading.hpp
index 3c72a3a..c3a6bc8 100644
--- a/include/Reading.hpp
+++ b/include/Reading.hpp
@@ -179,7 +179,8 @@ class Reading {
 
        int64_t time_ms() const { return ((int64_t)_time.tv_sec) * 1e3 + (_time.tv_usec / 1e3); };
        const long &time_s() const {
-               return _time.tv_sec;
+               static long tv_sec = _time.tv_sec;
+               return tv_sec;
        }; // return only the seconds (always rounding down)
        void time() { gettimeofday(&_time, NULL); }
        void time(struct timeval const &v) { _time = v; }

This exposes the bug in the original code: What is returned is not const, since it is based on a variable, so it can change over time. Assigning that to a static shows that everytime the function is called the newest value is taken, so the proper fix is not returning a const.

After this one, however, there are other error messages, so getting this to fully compile will require more changes.

@maxberger
Copy link
Contributor

So quick check: @StefanSchoof : Do you want to continue pursuing this? I may have some time at some point, but don't want to promise anything right now.

@StefanSchoof
Copy link
Contributor Author

I may try it. But time may a issue and my c knowledge is very limited. So if you find time, I would be grateful.

@maxberger maxberger mentioned this pull request Jan 17, 2023
@maxberger
Copy link
Contributor

Turned out to be quite easy at the end. The second issue was the only other issue. See #564

@StefanSchoof
Copy link
Contributor Author

Great, thanks @maxberger

Dockerfile Outdated Show resolved Hide resolved
@StefanSchoof
Copy link
Contributor Author

It is now building. Next step should be, to test if this image is working on an arm as expected.

@maxberger
Copy link
Contributor

Thanks! Based on the discussions in #478 I realized there should probably be 2 types of test:

  1. One that runs the included cmake tests. This should be an easy add to the docker build first phase

  2. an ldd test to ensure all libraries are there. This would be another phase with a separate image based on the final image, or with the final image.

The second one is needed because other PRs such as #525 change the needed libraries, and have a potential to easily break things.

In addition, someone needs to decide if alpine is what vzlogger wants to provide, or if you rather want to go with a pure buildroot (#486). I would recommend alpine because libary handling is easier there, but that's up to the vzlogger committers to decide.

@StefanSchoof
Copy link
Contributor Author

On my raspberry this image is running fine for several hours, without any problems.

@StefanSchoof StefanSchoof marked this pull request as ready for review January 18, 2023 09:45
@maxberger
Copy link
Contributor

Here is the diff for the ldd test. I would recommend to add that, just to ensure we don't forget any libraries. It does not add additional dependencies or build time:

@@ -69,6 +69,8 @@ RUN apk add --no-cache \
 COPY --from=builder /usr/local/bin/vzlogger /usr/local/bin/vzlogger
 COPY --from=builder /usr/local/lib/libmbus.so* /usr/local/lib/

+RUN ldd /usr/local/bin/vzlogger
+
 # without running a user context, no exec is possible and without the dialout group no access to usb ir reader possible
 RUN adduser -S vz -G dialout

@maxberger
Copy link
Contributor

I also tried to run the included test inside docker. This is currently failing because some tests use tmpnam_r, which is not available on muslc, and therefore not on alpine. I'll see if there is an easy way to fix this.

@r00t-
Copy link
Contributor

r00t- commented Jan 18, 2023

Here is the diff for the ldd test.

to ensure no libraries are missing, i would simply execute vzlogger --version

@r00t-
Copy link
Contributor

r00t- commented Jan 18, 2023

probably not a new issue, but seems worth noting:
running "docker build ." after having run cmake . on the host breaks the build.
maybe some kind of cleaning should be added.

@maxberger
Copy link
Contributor

probably not a new issue, but seems worth noting: running "docker build ." after having run cmake . on the host breaks the build. maybe some kind of cleaning should be added.

cmake is confused that it is running in different environments. The solution is to add "-B" to use a build directory. We could do this in the dockerfile. I'll try to create a patch for that as well :)

@maxberger
Copy link
Contributor

I created another PR for test fixed with alpine: #566 .

Once that is commited you can run:

RUN cmake -DBUILD_TEST=on \
    && make \
    && make install \
    && make test

This increased build time for me from about 20secs to about 110 secs (native build only, did not try on qemu/arm)

@maxberger
Copy link
Contributor

And here is the patch for not-confusing cmake. Of course if may need to be adapted with the build test and make test from above.

diff --git a/Dockerfile b/Dockerfile
index d5743b4..d8fe79f 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -38,9 +38,9 @@ RUN git clone https://github.com/rscada/libmbus.git --depth 1 \

 COPY . /vzlogger

-RUN cmake -DBUILD_TEST=off \
-    && make \
-    && make install
+RUN cmake -B indocker -DBUILD_TEST=off \
+    && make -C indocker \
+    && make -C indocker install


 #############################

@maxberger
Copy link
Contributor

Here is the diff for the ldd test.

to ensure no libraries are missing, i would simply execute vzlogger --version

Sure, that works as well:

@@ -69,6 +69,8 @@ RUN apk add --no-cache \
 COPY --from=builder /usr/local/bin/vzlogger /usr/local/bin/vzlogger
 COPY --from=builder /usr/local/lib/libmbus.so* /usr/local/lib/

+RUN vzlogger --version
+
 # without running a user context, no exec is possible and without the dialout group no access to usb ir reader possible
 RUN adduser -S vz -G dialout

@StefanSchoof
Copy link
Contributor Author

I added a build arg to switch the tests on. This allows to run a docker build --build-arg build_test=on .. I keep the default to off. If someone builds his docker image on a rpi this could a long time.

To fix the issue with running cmake on the host, I choose to add a .dockerignore with a superset of the .gitignore. This keeps the docker context small and we adding the Dockerfile, a change to Dockerfile is no longer invalidating the docker cache.

.dockerignore Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
.dockerignore Outdated Show resolved Hide resolved
@maxberger
Copy link
Contributor

Thanks. I like the idea of having the tests conditionally. I added some comments, but overall this is definitely what I will use.

@StefanSchoof
Copy link
Contributor Author

Thanks for the comments.

@maxberger
Copy link
Contributor

Awesome! Looks really good to me!

@r00t-
Copy link
Contributor

r00t- commented Jan 21, 2023

@StefanSchoof:
can you clean this up a little? (rebase to master and squash the fixup commits)?
(otherwise i can do that, if github allows me to force-push to your branch)
then we can merge.

@isarrider
Copy link
Contributor

I will do an FUP pull then which changes "make" to "make -j $(nproc --all)"...

Dockerfile Outdated Show resolved Hide resolved
@r00t-
Copy link
Contributor

r00t- commented Jan 26, 2023

would be ok to defer to a followup MR,
but it would be neat to install gtest on the builder image,
as supported since #560
to avoid pulling gtest from git when buildig the tests.
could maybe be conditional on the tests being enabled, but then leads to the boolean-parsing mess again. (for that case the != off) check should be good enough!)

@r00t-
Copy link
Contributor

r00t- commented Jan 28, 2023

thanks!

@r00t- r00t- merged commit 9b63aaa into volkszaehler:master Jan 28, 2023
@isarrider isarrider mentioned this pull request Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants