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

WIP - Add ARM job to Travis-CI #1235

Closed
wants to merge 6 commits into from
Closed

WIP - Add ARM job to Travis-CI #1235

wants to merge 6 commits into from

Conversation

piponazo
Copy link
Collaborator

In this PR I am trying to add a new job to travis-ci to check the compilation & test execution on ARM64 architectures.

I had to fight a bit to make the project to compile due to a difference in the CMake version between the ARM64 and AMD64 travis images (issue documented here: https://travis-ci.community/t/about-the-multi-cpu-architecture-category/5336/27?u=piponazo) .

Now, the job is failing when running the system tests. I will investigate in the next days what's the issue there (@cgilles , I am pinging you just to let you know that there might some parts in Exiv2 that behaves differently depending on the architecture).

@piponazo piponazo added CMake Configuration issues related with CMake CI Issues related with CI jobs compilers Related with compiler options, definitions, support, etc. labels Jun 10, 2020
@piponazo piponazo self-assigned this Jun 10, 2020
@clanmills
Copy link
Collaborator

Thank You for working on this Luis. I will look at this after breakfast.

I'm working on my book and been updating section 13-2 about the build. https://clanmills.com/exiv2/book/#13-2

As well as discussing your great contributions to the build, I have said something about "we can't support every possible way to build the code.". Have a look. Feedback Welcome.

clanmills
clanmills previously approved these changes Jun 10, 2020
Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Work, Luis. I don't know anything about the Rasperry Pie. We're building with the ARM processor for testing on Linux running on an ARM processor. Is that correct?

  1. CONAN_DISABLE_WEBREADY in run.sh

I didn't need anything like CONAN_DISABLE_WEBREADY in my build script on the MacMini. The values True and False are both valid CMake and Python. So I set webready=True|False and run cmake ... -DEXIV2_ENABLE_WEBREADY=${webready} and conan ... --options webready=${webready}

svn://dev.exiv2.org/svn/team/contrib/buildserver/build.sh

  1. CR-LF Endings

I see you feel down that rat-hole. Can you squash the commits, or open a new PR and delete this one or something. It's harmless and very ugly.

  1. Using the Gist Feature on GitHub

I was talking to @sridharb1 a couple of days ago and he told me about GitHub's Gist. This sounds like something very useful for dealing with "edge" conditions. If somebody wants to build in a special way, perhaps we should open a Gist. For example, if somebody wanted to use Clang in Visual Studio, we shouldn't add anything to the CI. We should publish a Gist saying how to do it, but not increase our build matrix.

I like this enough to consider bringing it to 0.27-maintenance after v0.27.3 has shipped.

@piponazo
Copy link
Collaborator Author

Hi @clanmills , thanks for your feedback.

  1. True, I should revert the change about CONAN_DISABLE_WEBREADY. At the beginning I got some failures and I thought the compilation was failing when conan was compiling the WEBREADY dependencies. But I think I misread something ...

  2. Sure, I can drop the CR-LF Endings change from this PR and create a separate one 👍

  3. I have a different opinion on this point. If we can enable support for some special configuration easily and freely, I would prefer to have it running on CI. It is true that it takes some time to create the configuration initially, but then it pays off over time since it will check that the build and tests pass automatically for each change. I actually enjoy taking responsibility about these tasks, so that I can learn about the latest trends in CI/CD tools.

In this specific case, Travis-CI enabled the ARM64 builds few months ago and I did not hear about it until now. I actually do own a raspberryPi that I can use to do local testing to debug issues locally.

@clanmills
Copy link
Collaborator

I think we're in total agreement, @piponazo. I think we should support ARM.

I think we should build MinGW/msys2 on CI. You can't use conan, however it easy to install all the dependences on with pacmac. It's documented in README.md.

Building with Cygwin on the CI could be more challenging because I don't know how to install anything into cygwin from a script (and conan isn't working there either). I don't think a basic cygwin has GCC or git. Anyway, MinGW/msys is almost identical to Cygwin today.

You've got a Rasperry Pie? Wonderful. Give me an update when you're got Exiv2 working on it. And could you build and run tvisitor.cpp from the book on it. svn://dev.exiv2.org/svn/team/book/tvisitor.cpp

The idea that we use a Gist for "This is how you do XXX" is nice because the Gist doesn't get closed (unlike issues and prs). There is a lot of useful information in closed PRs and Issues. If future when we need to update documentation, we should put it in a Gist and at release time to consolidate into README.md (or wherever).

Last point: Can you have a look at #1232 sometime (not urgent).

@clanmills
Copy link
Collaborator

I've just looked at the test failure on the CI. It's in bugfixes.github.test_CVE_2018_12265.AdditionOverflowInLoaderExifJpeg which uses safe_add. I can't remember how safe_add works. As you've suggested, it's probably a minor platform difference. Perhaps MAX_INT is 1 less on ARM64!

We shouldn't merge this PR until that's fixed. There is always more to getting these things done and ready than expected. I'm giving you encouragement to continue to hunt this down!

@mergify mergify bot dismissed clanmills’s stale review August 10, 2020 17:38

Pull request has been modified.

@tbeu
Copy link
Member

tbeu commented Aug 10, 2020

I tried to resolve the merge conflict when merging #1252. Let's see...

@piponazo
Copy link
Collaborator Author

I'll resume the work on this branch in the next days 😉

@piponazo
Copy link
Collaborator Author

Right now the job is failing at one python tests. The PR were the changes were added and the code changes is https://github.com/Exiv2/exiv2/pull/398/commits. To be investigated

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler command looks good:

args /usr/bin/x86_64-w64-mingw32-g++ --sysroot=/usr/lib/gcc/x86_64-w64-mingw32/9.3-posix  -Dexiv2lib_STATIC @CMakeFiles/exiv2lib_int.dir/includes_CXX.rsp -O3 -DNDEBUG -fvisibility=hidden -fno-keep-inline-dllexport   -Wp,-D_GLIBCXX_ASSERTIONS -Wp,-D_FORTIFY_SOURCE=2 -Wall -Wcast-align -Wpointer-arith -Wformat-security -Wmissing-format-attribute -Woverloaded-virtual -W -std=c++11 -o CMakeFiles/exiv2lib_int.dir/cr2header_int.cpp.obj -c /media/linuxDev/programming/exiv2/src/cr2header_int.cpp
1 : /usr/bin/x86_64-w64-mingw32-g++
2 : --sysroot=/usr/lib/gcc/x86_64-w64-mingw32/9.3-posix
3 : -Dexiv2lib_STATIC
4 : @CMakeFiles/exiv2lib_int.dir/includes_CXX.rsp
5 : -O3
6 : -DNDEBUG
7 : -fvisibility=hidden
8 : -fno-keep-inline-dllexport
9 : -Wp,-D_GLIBCXX_ASSERTIONS
10: -Wp,-D_FORTIFY_SOURCE=2
11: -Wall
12: -Wcast-align
13: -Wpointer-arith
14: -Wformat-security
15: -Wmissing-format-attribute
16: -Woverloaded-virtual
17: -W
18: -std=c++11
19: -o
20: CMakeFiles/exiv2lib_int.dir/cr2header_int.cpp.obj
21: -c
22: /media/linuxDev/programming/exiv2/src/cr2header_int.cpp
573 rmills@rmillsmbp:~/gnu/github/XMP-Toolkit-SDK $

There's a compiler switch to reveal header paths. Worth using that on both the good (linux) and the bad (mingw/cross).
https://stackoverflow.com/questions/11946294/dump-include-paths-from-g

Read the response file: @CMakeFiles/exiv2lib_int.dir/includes_CXX.rsp

Patience. Relax. You'll find it.

@piponazo
Copy link
Collaborator Author

Thanks @clanmills ! your feedback helpmed me to take another step. Now I know that for some reason, the compiler is looking in the wrong include folders:

(conan) luis@ryzenLinux:/media/linuxDev/programming/exiv2/buildMingw/src$ /usr/bin/x86_64-w64-mingw32-g++ -Xpreprocessor -v  --sysroot=/usr/lib/gcc/x86_64-w64-mingw32/9.3-posix  -Dexiv2lib_STATIC @CMakeFiles/exiv2lib_int.dir/includes_CXX.rsp -O3 -DNDEBUG -fvisibility=hidden -fno-keep-inline-dllexport   -Wp,-D_GLIBCXX_ASSERTIONS -Wp,-D_FORTIFY_SOURCE=2 -Wall -Wcast-align -Wpointer-arith -Wformat-security -Wmissing-format-attribute -Woverloaded-virtual -W -std=c++11 -o CMakeFiles/exiv2lib_int.dir/cr2header_int.cpp.obj -c /media/linuxDev/programming/exiv2/src/cr2header_int.cpp
ignoring nonexistent directory "/usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/../../../../x86_64-w64-mingw32/sys-include"
#include "..." search starts here:
#include <...> search starts here:
 /media/linuxDev/programming/exiv2/buildMingw
 /media/linuxDev/programming/exiv2/buildMingw/src
 /media/linuxDev/programming/exiv2/include/exiv2
 /home/luis/.conan/data/zlib/1.2.11/conan/stable/package/344886eda55829e935447d0708e3b993938b32c8/include
 /usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/include/c++
 /usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/include/c++/x86_64-w64-mingw32
 /usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/include/c++/backward
 /usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/include
 /usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/include-fixed
 /usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/../../../../x86_64-w64-mingw32/include
End of search list.
In file included from /media/linuxDev/programming/exiv2/include/exiv2/xmp_exiv2.hpp:34,
                 from /media/linuxDev/programming/exiv2/include/exiv2/image.hpp:31,
                 from /media/linuxDev/programming/exiv2/src/tiffimage_int.hpp:33,
                 from /media/linuxDev/programming/exiv2/src/cr2header_int.hpp:31,
                 from /media/linuxDev/programming/exiv2/src/cr2header_int.cpp:1:
/media/linuxDev/programming/exiv2/include/exiv2/properties.hpp:215:21: error: ‘mutex’ in namespace ‘std’ does not name a type
  215 |         static std::mutex mutex_;

I read over internet that MinGW comes with to flavors: Win32 & Posix. For some reason, it is choosing by default the Win32 one where the moder C++11 stuff is not present. I need to find a way to change these paths:

 /usr/lib/gcc/x86_64-w64-mingw32/9.3-win32/include/c++

to

/usr/lib/gcc/x86_64-w64-mingw32/9.3-posix/include/c++/

@piponazo
Copy link
Collaborator Author

piponazo commented Sep 18, 2020

I just found the answer to that:
https://stackoverflow.com/questions/55197902/cmake-cross-compiling-linux-to-windows-with-mingw-does-not-find-some-system-hea

In the cmake toolchain you have to use other compiler!

# cross compilers to use for C and C++
#set(CMAKE_C_COMPILER ${TOOLCHAIN_PREFIX}-gcc)
#set(CMAKE_CXX_COMPILER ${TOOLCHAIN_PREFIX}-g++)
set(CMAKE_C_COMPILER ${TOOLCHAIN_PREFIX}-gcc-posix)
set(CMAKE_CXX_COMPILER ${TOOLCHAIN_PREFIX}-g++-posix)

Of course, Now I am facing other problems 😛

@clanmills
Copy link
Collaborator

Team Work Works. Glad I've been able to help you. Love the work done by new guy. I hope he stays. Very pleased with the young python Chinese guy working on the bash->python code.

I'm trying to work with and understand the AdobeXMPsdk. What a mess! Those responsible for that should be shot. Then murdered and their heads displayed on Traitor's Gate.

@piponazo
Copy link
Collaborator Author

I cannot belive it ... I was almost there! and at the very last step, and old friend resurrected BasicError::setMsg()

[ 56%] Linking CXX executable ../bin/exiv2.exe
/usr/bin/x86_64-w64-mingw32-ld: CMakeFiles/exiv2.dir/objects.a(params.cpp.obj):params.cpp:(.text$_ZN5Exiv210BasicErrorIcEC1INSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEENS_9ErrorCodeERKT_[_ZN5Exiv210BasicErrorIcEC1INSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEENS_9ErrorCodeERKT_]+0x95): undefined reference to `__imp__ZN5Exiv210BasicErrorIcE6setMsgEv'
collect2: error: ld returned 1 exit status

As discussed in #663 , we have to deal with that for unblocking the support for MinGW builds. I might take a look to that issue in the next days 😉

@piponazo
Copy link
Collaborator Author

I jus noticed that I am mixing 2 topics here ... ARM & MinGW. Definitely I am not thinking straight these days ... 😓

@clanmills
Copy link
Collaborator

I thought that was solved. I haven't had to do anything about that on 0.27-maintenance. I built MinGW (native and cross-compile). The CI builds it as well Fedora or Suse/MinGW. And I've successfully run the test suite on the cross build.

Base automatically changed from master to old-master February 28, 2021 05:57
@clanmills
Copy link
Collaborator

@piponazo This is still 'work in progress. and is on branch 'old-master'.

Is there something here to port in 'main'?

@piponazo
Copy link
Collaborator Author

Hi @clanmills , I will need to revisit this topic because I can give you a good answer. I just noticed that the main branch is created, so I'll start doing the migration steps once I find some spare time.

I'll take care of closing this PR and redo the work on main.

@clanmills
Copy link
Collaborator

Thanks, that's fine, Luis (@piponazo). Only submit changes into 'main'. 0.27-maintenance should be considered read-only and old-master should be considered dead.

We might need to do a "security release" from 0.27-maintenance (CVE fixes only) in the next 12-18 months.

@neheb
Copy link
Collaborator

neheb commented Jul 19, 2023

This is obsolete I think.

@piponazo
Copy link
Collaborator Author

Yes, I will close the PR and delete the branch. Lately I do not have much time to contribute in the project :(

@piponazo piponazo closed this Jul 20, 2023
@piponazo piponazo deleted the master_ci_arm branch July 20, 2023 06:09
@tbeu tbeu removed request for D4N and cgilles August 7, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issues related with CI jobs CMake Configuration issues related with CMake compilers Related with compiler options, definitions, support, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants