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

CI: rescue appveyor config from old-master and backup others #1588

Merged
merged 9 commits into from
May 3, 2021

Conversation

piponazo
Copy link
Collaborator

@piponazo piponazo commented Apr 24, 2021

In this PR I am recovering the Appveyor configuration from old-master which is covering more cases than the previous one. Also, the Github PR page is now pointing to the correct Appveyor instance (before it was pointing to some Robin's personal instance).

Currently, the Appveyor configuration only had 3 jobs with different versions of Visual Studio and always the same configuration:

  • Visual Studio 2019 - x86_64 - Shared Lib - Release
  • Visual Studio 2017 - x86_64 - Shared Lib - Release
  • Visual Studio 2015 - x86_64 - Shared Lib - Release

I think it is more interesting to have different configurations but use always the latest Visual Studio since we are sure all these versions of Visual Studio fully implement C++11 (The previous configuration was saved at ci/appveyor_all_vs_versions.yml as a backup):

  • Visual Studio 2019 - x86_64 - Shared Lib - Release
  • Visual Studio 2019 - x86_64 - Shared Lib - Debug
  • Visual Studio 2019 - x86_64 - Static Lib - Release
  • Visual Studio 2019 - x86_64 - Static Lib - Debug
  • Visual Studio 2019 - x86 - Shared Lib - Release
  • Visual Studio 2019 - x86 - Shared Lib - Debug
  • Visual Studio 2019 - x86 - Static Lib - Release
  • Visual Studio 2019 - x86 - Static Lib - Debug

Thanks to this change I could fix few deffects:

  • Conan: When using gtest on debug mode, I had to remove the debug_postfix, otherwise CMake was not finding the library provided by conan.
  • Conan: Keep the usage of the old Expat recipe. On windows there are problems to find the libraries.
  • CMake: Ignore LINK warnings 4099 about missing PDBs on 3rd party dependencies. We are not interested at the moment in generating fully working PDBs on Windows / Debug releases.
  • Code: fix some warnings detected in debug mode.
  • Testing code: Increased timeout. Windows Debug releases are very slow and they were timing out.

@@ -133,7 +133,8 @@ set_target_properties( exiv2lib_int PROPERTIES
COMPILE_DEFINITIONS exiv2lib_EXPORTS
)

target_link_libraries(exiv2lib_int PRIVATE ZLIB::ZLIB)
# NOTE: Cannot use target_link_libraries on OBJECT libraries with old versions of CMake
Copy link
Member

Choose a reason for hiding this comment

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

Which CMake version are we stuck with due to older distributions?
Does it maybe make sense to move to a newer one, and simply have the older distributions download a newer version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On Debian:9 the default CMake version is 3.7 and on CentOS:8 it is 3.11.

I am the first one I would like to use the latest CMake version possible, but in practice I think this would make more difficult for distributions to include the latest Exiv2 version (need to create patches, and so on). But being honest, I am just guessing, I do not know how distribution maintainers consume and generate binaries for each of the distribution packages.

Most of the times (like this one) it is only about tiny details, and the quality of the CMake code does not get affected (too much). If we find a good excuse/reason to require a CMake higher version, we can always discuss it in a separate issue/PR 😁

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable to me! It's always a trade-off :)
As long as it's not impacting our CMake too much I think you are right, no reason to upgrade just yet :)

@piponazo piponazo changed the title WIP - ci: rescue appveyor config from old-master and backup others CI: rescue appveyor config from old-master and backup others Apr 26, 2021
@piponazo piponazo merged commit 294372f into main May 3, 2021
@piponazo piponazo deleted the main-AppveyorNewConfig branch May 3, 2021 04:40
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.

2 participants