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

Back-port actions and fuzzer to 0.27-maintenance #1854

Merged

Conversation

kevinbackhouse
Copy link
Collaborator

I would like to run all the automated checks that we have developed for the main branch on the 0.27-maintenance branch before we release 0.27.5. This PR mostly just copies the contents of the .github and fuzz sub-directories from the main branch onto the 0.27-maintenance branch. There are a few other minor changes :

  • CMakeLists.txt, cmake/compilerFlags.cmake: add the fuzzer to cmake.
  • src/basicio.cpp, src/jpgimage.cpp, src/pngimage.cpp: don't call memcpy or memcmp with a zero size argument to avoid a UBSAN false positive (complaining about one of the pointer arguments being NULL).
  • src/image.cpp, src/jpgimage.cpp: some debug messages were accidentally switched on by default, which was spamming the fuzzer logs.
  • The build commands are the same as the main branch, except BMFF is off by default.
  • I disabled all "schedule" triggers in the actions because we don't need to build the 0.27-maintenance branch on a daily basis.

@kevinbackhouse kevinbackhouse added this to the v0.27.5 milestone Aug 6, 2021
@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #1854 (df011fc) into 0.27-maintenance (58fb9f8) will decrease coverage by 19.31%.
The diff coverage is 40.28%.

Impacted file tree graph

@@                  Coverage Diff                  @@
##           0.27-maintenance    #1854       +/-   ##
=====================================================
- Coverage             65.47%   46.16%   -19.32%     
=====================================================
  Files                   159      145       -14     
  Lines                 23104    22460      -644     
  Branches                  0    11514    +11514     
=====================================================
- Hits                  15128    10368     -4760     
+ Misses                 7976     6647     -1329     
- Partials                  0     5445     +5445     
Impacted Files Coverage Δ
include/exiv2/basicio.hpp 75.00% <ø> (ø)
include/exiv2/error.hpp 62.96% <ø> (-28.35%) ⬇️
include/exiv2/exif.hpp 100.00% <ø> (ø)
include/exiv2/image.hpp 100.00% <ø> (ø)
include/exiv2/iptc.hpp 100.00% <ø> (ø)
include/exiv2/metadatum.hpp 40.00% <ø> (-60.00%) ⬇️
include/exiv2/pgfimage.hpp 0.00% <ø> (-100.00%) ⬇️
include/exiv2/rwlock.hpp 100.00% <ø> (ø)
include/exiv2/slice.hpp 89.83% <ø> (-8.32%) ⬇️
include/exiv2/xmp_exiv2.hpp 83.33% <ø> (-8.98%) ⬇️
... and 257 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13c4b14...df011fc. Read the comment docs.

@kevinbackhouse
Copy link
Collaborator Author

All the builds are passing now, except "Win Matrix on PRs / Win10 Arch: x64 BuildType:Debug - SHARED:OFF".
It looks like the solution is in commit 582edd3, but I'm not sure if I need all of that or only the change in samples/CMakeLists.txt.

- name: Build
run: |
mkdir build && cd build
cmake -GNinja -DCMAKE_BUILD_TYPE=${{matrix.build_type}} -DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=OFF -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" -DCMAKE_CXX_STANDARD=11 ..
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmake -GNinja -DCMAKE_BUILD_TYPE=${{matrix.build_type}} -DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=OFF -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" -DCMAKE_CXX_STANDARD=11 ..
cmake -GNinja -DCMAKE_BUILD_TYPE=${{matrix.build_type}} -DBUILD_SHARED_LIBS=${{matrix.shared_libraries}} -DEXIV2_ENABLE_PNG=ON -DEXIV2_ENABLE_WEBREADY=ON -DEXIV2_ENABLE_CURL=ON -DEXIV2_BUILD_UNIT_TESTS=ON -DEXIV2_ENABLE_BMFF=OFF -DEXIV2_TEAM_WARNINGS_AS_ERRORS=ON -DCMAKE_INSTALL_PREFIX=install -DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" -DCMAKE_CXX_STANDARD=98 ..

on 0.27 it should be c++98 right?

Copy link
Collaborator Author

@kevinbackhouse kevinbackhouse Aug 6, 2021

Choose a reason for hiding this comment

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

I couldn't get the build to work with conan on macOS, so I tried removing the conan step. But that meant that I had to add brew install googletest to the installation steps and that seems to have given me a version of gtest that will only build with C++11. I figure that it's fairly harmless to use C++11 here, because I'm just passing it on the command-line rather than modifying any of the CMakeLists.txt files.

Do you know the best way to build on macOS?

Copy link
Member

Choose a reason for hiding this comment

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

Probably fine, considering that we are just trying to run these checks.

I unfortunately also don't know much about the build config, especially for MacOS and Win.
@piponazo was the one who set most of this up AFAIK

@kevinbackhouse kevinbackhouse marked this pull request as ready for review August 7, 2021 15:48
@kevinbackhouse
Copy link
Collaborator Author

I think this is ready now. The first 4 commits are cherry-picked from main to fix various build problems. The 5th commit adds the Actions and fuzz directory.

The Actions are currently building with -DEXIV2_ENABLE_BMFF=OFF because I mistakenly thought that BMFF was not enabled 0.27. I will enable BMFF after #1855 is merged.

@kevinbackhouse kevinbackhouse merged commit 1588af3 into Exiv2:0.27-maintenance Aug 8, 2021
@clanmills clanmills mentioned this pull request Aug 9, 2021
@kevinbackhouse kevinbackhouse deleted the ActionsBackPort branch August 11, 2021 14:58
@clanmills clanmills mentioned this pull request Sep 8, 2021
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