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

Fix_ci_0.27 #1296

Merged
merged 12 commits into from
Sep 16, 2020
Merged

Fix_ci_0.27 #1296

merged 12 commits into from
Sep 16, 2020

Conversation

clanmills
Copy link
Collaborator

@clanmills clanmills self-assigned this Sep 11, 2020
@clanmills clanmills added this to the v0.27.4 milestone Sep 11, 2020
@clanmills clanmills changed the title Fix ci Fix_ci_0.27 Sep 11, 2020
@piponazo piponazo self-requested a review September 11, 2020 08:59
.travis.yml Outdated
@@ -10,26 +10,26 @@ matrix:
dist: xenial
sudo: required
compiler: gcc
env: COVERAGE=1 CMAKE_OPTIONS="-DCMAKE_BUILD_TYPE=Debug -DEXIV2_ENABLE_VIDEO=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DBUILD_WITH_COVERAGE=ON -DEXIV2_ENABLE_CURL=ON -DCMAKE_CXX_STANDARD=98"
env: COVERAGE=1 CMAKE_OPTIONS="-DCMAKE_BUILD_TYPE=Debug -DEXIV2_ENABLE_VIDEO=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DBUILD_WITH_COVERAGE=ON -DEXIV2_ENABLE_CURL=ON cmake .. -DCMAKE_CXX_STANDARD=11 -DCMAKE_CXX_FLAGS=-Wno-deprecated"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see 2 problems here:

  • First of all CMAKE_OPTIONS is a variable with cmake options that will be passed later to CMake. The addition cmake .. does not make sense here.
  • I do not understand the change CMAKE_CXX_STANDARD=11. We are still requiring to use C++98 in 0.27-maintenance . The usage of C++11 is not possible since we are using the auto_ptr stuff. Apart from that, we are not using any c++11 stuff in this branch, so we would not gain anything by telling to the compiler to use the C++11 standard.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are quite right about C++11. The puzzle to solve is the CI overflowing with deprecation warnings and then saying "log too long" and marks the job in error.

@piponazo
Copy link
Collaborator

I can see that some of the Travis CI jobs are red in 0.27-maintenance, but it looks like it is a run-time problem when running the tests and not a problem in the configuration or compilation steps:

https://travis-ci.org/github/Exiv2/exiv2/jobs/726202125

In the case of the Appveyor builds, the problem seems to be due to a cache problem with conan:
https://ci.appveyor.com/project/piponazo/exiv2-wutfp/builds/35084088/job/u63fs365jhhf8a4s

I can take a look to the Appveyor issue if you want, I think I should manage to fix it quickly. On the other hand, the run-time issue would imply more research ... maybe the person who added those changes should take a look there.

@clanmills
Copy link
Collaborator Author

Yes. I spotted the crwtest failure and have logged that as #1297.

.travis.yml Outdated
sudo: required
compiler: gcc
env: COVERAGE=1 CMAKE_OPTIONS="-DCMAKE_BUILD_TYPE=Debug -DEXIV2_ENABLE_VIDEO=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DBUILD_WITH_COVERAGE=ON -DEXIV2_ENABLE_CURL=ON -DCMAKE_CXX_STANDARD=98"
env: COVERAGE=0 CMAKE_OPTIONS=".. -DCMAKE_BUILD_TYPE=Debug -DEXIV2_ENABLE_VIDEO=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DBUILD_WITH_COVERAGE=OFF -DEXIV2_ENABLE_CURL=ON -DCMAKE_CXX_FLAGS=-Wno-deprecated"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Robin. The .. needs to be removed from this variable. You can see in the file ci/run.sh that we are already indicating the path relatively there:

# ci/run.sh line 25
cmake ${CMAKE_OPTIONS} -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought I had dealt with this. Crap. I'll fix it.

Do you know how to get the CI to build MinGW/msys2 and/or Cygwin64?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The CI is mysteriously crashing on iotest in the new python version of bash_tests. I haven't reproduced this on my machines at home.

I'll work on that tomorrow and then the CI will work. I want to get back to the book this week as there is still a lot to be done. We're having 5 days vacation starting Tuesday 22 September.

.travis.yml Outdated
sudo: required
compiler: gcc
env: COVERAGE=1 CMAKE_OPTIONS="-DCMAKE_BUILD_TYPE=Debug -DEXIV2_ENABLE_VIDEO=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DBUILD_WITH_COVERAGE=ON -DEXIV2_ENABLE_CURL=ON -DCMAKE_CXX_STANDARD=98"
env: COVERAGE=0 CMAKE_OPTIONS="-CMAKE_CXX_STANDARD=98 -DCMAKE_BUILD_TYPE=Debug -DEXIV2_ENABLE_VIDEO=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_BUILD_UNIT_TESTS=OFF -DBUILD_WITH_COVERAGE=OFF -DEXIV2_ENABLE_CURL=ON -DCMAKE_CXX_FLAGS=-Wno-deprecated"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Robin. The problem now with this is a missing 'D' in -CMAKE_CXX_STANDARD=98.

I think today I will have some time in the evening, so that I can take a look to the existing failures at CI to help you with this.

@clanmills clanmills marked this pull request as ready for review September 15, 2020 16:50
@@ -8,22 +8,22 @@ source conan/bin/activate
if [[ "$(uname -s)" == 'Linux' ]]; then
if [ "$CC" == "clang" ]; then
# clang + Ubuntu don't like to run with UBSAN, but ASAN works
export CMAKE_OPTIONS="$CMAKE_OPTIONS -DCMAKE_CXX_FLAGS=\"-fsanitize=address\" -DCMAKE_C_FLAGS=\"-fsanitize=address\" -DCMAKE_EXE_LINKER_FLAGS=\"-fsanitize=address\" -DCMAKE_MODULE_LINKER_FLAGS=\"-fsanitize=address\""
export CMAKE_OPTIONS="$CMAKE_OPTIONS"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Robin. I am not sure it is a good idea to disable the sanitizers. In the practice, it has been helping us to identify many issues the last couple of years.

Do you intend to make this change permanent or are you just testing something?

I will take a look to the CI errors taking place in 0.27-maintenance

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The sanitisers are causing great trouble in two places (both concerning RemoteIo).

  1. calling gethostbyname() in http.cpp
    I can avoid that by using curl to handle http.
  2. in RemoteIo::mmap and RemoteIo::populateFakeData()
    I don't think curl can help with that.

For now the quick way to get the CI to turn green on 0.27-maintenance is to disable ASAN. I really appreciate you working with @neheb on the clang-tidy changes. Thank You very much. I really want to get back to working on the book. The last 3 weeks have been totally consumed by:

  1. python/bash_tests
  2. darktable/XmpProperties::propertyList()
  3. building on ARM/Xcode 6.1, stack-protector, build switches
  4. Useless community discussion about ISOBMFF, CR3, HEIF
  5. charset=Unicode
  6. I'm sure there was something else

I still have the following to do in the book:

  1. Previews
    To be researched
  2. 4 file formats are not finished:
    Olympus, RAF, GIF, CRW
  3. I/O
    including RemoteIo (which appears to have ASAN issues).
  4. XMPsdk
    How does Exiv2 interact with the Adobe code?
  5. Writing files
    Andreas uses a mysterious/undocumented "sophisticated algorithm".
  6. Complete lots of unfinished parts of the book
    security, documentation, etc..
  7. A general review of everything in the book.
  8. Undoubtably something I haven't realised or thought about!

It would be great to:

  1. Have a test harness for the three programs in the book
    tvisitor.cpp, dmpf.cpp and args.cpp
  2. Test tvisitor on more RAW images.
    https://raw.pixls.us

The scope of the book is vast. I'll get there. I always do. I'm worried that when the book is complete, I'm going to start receiving hundreds of emails about bugs, typos, omissions, questions and of course claiming that it violates unspecified patents. 3 people are demanding that I change the font and redraw all the graphics in SVG.

Alison and I will have a 5 day "mini-break" in a cottage in Wales next week. We haven't been more that 10k from home since March 1. We both need a break.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see, so it is more about ASAN detecting stuff in things that are not relevant for the Exiv2 core but useful for testing the remote part.

In that case, would be the following an option?:

  • Have one additional CI job testing the remote stuff with the basic configuration (gcc, Release, Ubuntu) and ASAN disabled.
  • The rest of the CI jobs could keep ASAN enabled without running the remote stuff.

On the other hand, with this approach we would not be running the remote stuff in all the configurations ... But I have the feeling that it is not so important to do it in every configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My priority is the book. I want to get the CI to be green because @LeoHsiao1 is working on the python/bash_tests #1215 on 0.27-maintenance. The CI will help him. And it's useful for other folks who have been sending 0.27-maintenance patches (compilerFlags etc).

I believe ASAN is already working on 'master'. If we do 0.27.4, I will restore ASAN on 0.27-maintenance and get it working perfectly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood. Let's do it like that. You had already enough pain with this 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have opened an issue about my ASAN/RemoteIo discoveries: #1307

@tester0077
Copy link
Collaborator

tester0077 commented Sep 17, 2020 via email

@clanmills
Copy link
Collaborator Author

I don't know and I'm not involved with that effort. For sure, it looks like Exiv2. There may be a way to email that project, or ask on the forum: https://discuss.pixls.us

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.

3 participants