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

meson changes #2712

Merged
merged 14 commits into from
Aug 8, 2023
Merged

meson changes #2712

merged 14 commits into from
Aug 8, 2023

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Jul 20, 2023

No description provided.

@ghost
Copy link

ghost commented Jul 20, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@neheb
Copy link
Collaborator Author

neheb commented Jul 20, 2023

@eli-schwartz iconv change sensible?

meson.build Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #2712 (51e76cb) into main (6ea6e2c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2712   +/-   ##
=======================================
  Coverage   63.91%   63.91%           
=======================================
  Files         103      103           
  Lines       22312    22312           
  Branches    10805    10805           
=======================================
  Hits        14260    14260           
  Misses       5830     5830           
  Partials     2222     2222           
Files Changed Coverage Δ
src/convert.cpp 52.35% <100.00%> (ø)

@neheb
Copy link
Collaborator Author

neheb commented Jul 28, 2023

@Ryanf55 can you test the cmake changes?

@neheb
Copy link
Collaborator Author

neheb commented Jul 28, 2023

@kmilos plz approve this PR as it contains a fix for meson CI build failures.

@Ryanf55
Copy link
Contributor

Ryanf55 commented Jul 28, 2023

yeah, #2703 broke it.

@Ryanf55 plz fix.

Let's follow up in the original PR?

@neheb
Copy link
Collaborator Author

neheb commented Jul 28, 2023

sure

@Ryanf55
Copy link
Contributor

Ryanf55 commented Jul 30, 2023

Yes. I added CMake config support to meson

Ok, so should I try to build/install with meson, then check I can still consume exiv2 with CMake?

Results not successful, the meson install seems to be missing the exiv2Export.cmake file.

@Ryanf55 can you test the cmake changes?

meson setup meson_build
cd meson_build
meson compile
meson install --destdir mesoninstall

Then, a simple test project that tries to find it in CONFIG mode.

ryan@ryan-B650-970:~/Development/ryanf55/exiv2/test_build$ cmake -S . -B build -DCMAKE_PREFIX_PATH=../meson_build/mesoninstall/usr/local/lib/x86_64-linux-gnu/cmake/
-- Found ZLIB: /usr/lib/x86_64-linux-gnu/libz.so (found version "1.2.13")  
CMake Error at /home/ryan/Development/ryanf55/exiv2/meson_build/mesoninstall/usr/local/lib/x86_64-linux-gnu/cmake/exiv2/exiv2Config.cmake:25 (include):
  include could not find requested file:

    /home/ryan/Development/ryanf55/exiv2/meson_build/mesoninstall/usr/local/lib/x86_64-linux-gnu/cmake/exiv2/exiv2Export.cmake
Call Stack (most recent call first):
  CMakeLists.txt:4 (find_package)


CMake Error at /home/ryan/Development/ryanf55/exiv2/meson_build/mesoninstall/usr/local/lib/x86_64-linux-gnu/cmake/exiv2/exiv2Config.cmake:27 (check_required_components):
  Unknown CMake command "check_required_components".
Call Stack (most recent call first):
  CMakeLists.txt:4 (find_package)


-- Configuring incomplete, errors occurred!

Although ZLIB is found, the export file is not found.

The generated config file is looking for the export file named like so.
include("${CMAKE_CURRENT_LIST_DIR}/exiv2Export.cmake")

Just a note: The cmake generated config file looks like this:

####### Expanded from @PACKAGE_INIT@ by configure_package_config_file() #######
####### Any changes to this file will be overwritten by the next CMake run ####
####### The input file was exiv2Config.cmake.in                            ########

get_filename_component(PACKAGE_PREFIX_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" ABSOLUTE)

macro(set_and_check _var _file)
  set(${_var} "${_file}")
  if(NOT EXISTS "${_file}")
    message(FATAL_ERROR "File or directory ${_file} referenced by variable ${_var} does not exist !")
  endif()
endmacro()

macro(check_required_components _NAME)
  foreach(comp ${${_NAME}_FIND_COMPONENTS})
    if(NOT ${_NAME}_${comp}_FOUND)
      if(${_NAME}_FIND_REQUIRED_${comp})
        set(${_NAME}_FOUND FALSE)
      endif()
    endif()
  endforeach()
endmacro()

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

cmake_minimum_required(VERSION 3.5)
include(CMakeFindDependencyMacro)

if(ON) # if(EXIV2_ENABLE_PNG)
  find_dependency(ZLIB REQUIRED)
endif()

include("${CMAKE_CURRENT_LIST_DIR}/exiv2Export.cmake")

check_required_components(exiv2)

While the meson one looks different, because it is missing the definition of check_required_components.


####### Expanded from @PACKAGE_INIT@ by configure_package_config_file() #######
####### Any changes to this file will be overwritten by the next CMake run ####
####### The input file was exiv2Config.cmake.in ########

get_filename_component(PACKAGE_PREFIX_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../.." ABSOLUTE)

macro(set_and_check _var _file)
  set(${_var} "${_file}")
  if(NOT EXISTS "${_file}")
    message(FATAL_ERROR "File or directory ${_file} referenced by variable ${_var} does not exist !")
  endif()
endmacro()

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


cmake_minimum_required(VERSION 3.5)
include(CMakeFindDependencyMacro)

if(ON) # if(EXIV2_ENABLE_PNG)
  find_dependency(ZLIB REQUIRED)
endif()

include("${CMAKE_CURRENT_LIST_DIR}/exiv2Export.cmake")

check_required_components(exiv2)

@neheb
Copy link
Collaborator Author

neheb commented Jul 30, 2023

hrm I'll remove it then.

@Ryanf55
Copy link
Contributor

Ryanf55 commented Jul 30, 2023

Seems like this PR is missing the other macro, check_required_components: mesonbuild/meson#5949

Even if that's fixed, there doesn't seem to be a way to get meson to generate the export set.

Edit: There's a draft PR to do exports, but it's not merged: mesonbuild/meson#9978

@neheb
Copy link
Collaborator Author

neheb commented Jul 31, 2023

alright, removed. just kept the version file, which does I have no idea what.

neheb added 14 commits August 7, 2023 23:52
Signed-off-by: Rosen Penev <[email protected]>
MSVCRT requires wide string APIs, which were removed. Instead of dealing
with this, just error out on it. MSVCRT is deprecated anyway.

Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Moved files in normal headers for simplicity.

Signed-off-by: Rosen Penev <[email protected]>
Makes the main one smaller.

Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Also make video support properly optional.

Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
CMake can't install properly for some reason. Nuke.

Signed-off-by: Rosen Penev <[email protected]>
apt is having some reliability issues. Cache the packages to speed up
workflow and fix this.

Signed-off-by: Rosen Penev <[email protected]>
The update to 3.11 seems to have fixed meson crashing.

Signed-off-by: Rosen Penev <[email protected]>
Reduces verbosity when installing packages.

Signed-off-by: Rosen Penev <[email protected]>
@neheb
Copy link
Collaborator Author

neheb commented Aug 8, 2023

@kmilos please review. Note I got rid of one CI here.

Copy link
Collaborator

@kmilos kmilos left a comment

Choose a reason for hiding this comment

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

LGTM

@neheb neheb merged commit 0831c81 into Exiv2:main Aug 8, 2023
104 checks passed
@1div0
Copy link
Collaborator

1div0 commented Nov 5, 2023

LG2M++

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.

5 participants