szip: conan v2 support #13551

paulharris · 2022-10-17T15:16:56Z

Please check the patch - there are a few questions around the targets.

The old recipe generated targets for szip-static and szip-shared.
It also generated an alias target for szip::szip but only for the older finders.

I want to generate the szip::szip target, and am wondering if we should continue to generate the other plain ones (and how would it be done).

Also, I get a weird error message when trying to build locally from test_package:

-- Using Conan toolchain: K:/env/conan3/deps/cci-fixes/szip/recipes/szip/all/test_package/build/generators/conan_toolchain.cmake
-- Selecting Windows SDK version 10.0.22621.0 to target Windows 10.0.19044.
-- Conan: Target declared 'szip::szip'
CMake Error at build/generators/szip-Target-release.cmake:34 (conan_package_library_targets):
  conan_package_library_targets Function invoked with incorrect arguments for
  function named: conan_package_library_targets
Call Stack (most recent call first):
  build/generators/szipTargets.cmake:26 (include)
  build/generators/szip-config.cmake:76 (include)
  CMakeLists.txt:5 (find_package)

What does it mean and how do I fix it?

github-actions · 2022-10-17T15:38:13Z

Hooks produced the following warnings for commit 49a83d9

szip/2.1.1

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libszip_debug.so' links to system library 'm' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libszip.so' links to system library 'm' but it is not in cpp_info.system_libs.

github-actions · 2022-10-17T15:51:22Z

Hooks produced the following warnings for commit a11570c

szip/2.1.1

post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libszip_debug.so' links to system library 'm' but it is not in cpp_info.system_libs.
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library './lib/libszip.so' links to system library 'm' but it is not in cpp_info.system_libs.

recipes/szip/all/conanfile.py

prince-chrismc

Same feedback as the question on Slack and the issue you created :)

recipes/szip/all/conanfile.py

paulharris · 2022-10-18T13:15:49Z

I've fixed things up and squashed the commits so the diff is more straight forward.
I kept build() above generate() as that means less movement in the code, hopefully easier to see the actual changes.

paulharris · 2022-10-18T13:46:42Z

I have no idea why the static build works fine, but the shared build fails -- the test can't find the header file...

prince-chrismc

installing this from scratch... I noticed the shared export is missing a public definition

set_target_properties(szip-shared PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "SZ_BUILT_AS_DYNAMIC_LIB=1"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "m"
)

full cmake for reference

cat /home/cmcarthur/szip-2.1.1/./share/cmake/szip-targets.cmake
# Generated by CMake

if("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" LESS 2.6)
   message(FATAL_ERROR "CMake >= 2.6.0 required")
endif()
cmake_policy(PUSH)
cmake_policy(VERSION 2.6...3.20)
#----------------------------------------------------------------
# Generated CMake target import file.
#----------------------------------------------------------------

# Commands may need to know the format version.
set(CMAKE_IMPORT_FILE_VERSION 1)

# Protect against multiple inclusion, which would fail when already imported targets are added once more.
set(_targetsDefined)
set(_targetsNotDefined)
set(_expectedTargets)
foreach(_expectedTarget szip-static szip-shared)
  list(APPEND _expectedTargets ${_expectedTarget})
  if(NOT TARGET ${_expectedTarget})
    list(APPEND _targetsNotDefined ${_expectedTarget})
  endif()
  if(TARGET ${_expectedTarget})
    list(APPEND _targetsDefined ${_expectedTarget})
  endif()
endforeach()
if("${_targetsDefined}" STREQUAL "${_expectedTargets}")
  unset(_targetsDefined)
  unset(_targetsNotDefined)
  unset(_expectedTargets)
  set(CMAKE_IMPORT_FILE_VERSION)
  cmake_policy(POP)
  return()
endif()
if(NOT "${_targetsDefined}" STREQUAL "")
  message(FATAL_ERROR "Some (but not all) targets in this export set were already defined.\nTargets Defined: ${_targetsDefined}\nTargets not yet defined: ${_targetsNotDefined}\n")
endif()
unset(_targetsDefined)
unset(_targetsNotDefined)
unset(_expectedTargets)


# Compute the installation prefix relative to this file.
get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "")
endif()

# Create imported target szip-static
add_library(szip-static STATIC IMPORTED)

set_target_properties(szip-static PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "m"
)

# Create imported target szip-shared
add_library(szip-shared SHARED IMPORTED)

set_target_properties(szip-shared PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "SZ_BUILT_AS_DYNAMIC_LIB=1"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "m"
)

if(CMAKE_VERSION VERSION_LESS 2.8.12)
  message(FATAL_ERROR "This file relies on consumers using CMake 2.8.12 or greater.")
endif()

# Load information for each installed configuration.
get_filename_component(_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
file(GLOB CONFIG_FILES "${_DIR}/szip-targets-*.cmake")
foreach(f ${CONFIG_FILES})
  include(${f})
endforeach()

# Cleanup temporary variables.
set(_IMPORT_PREFIX)

# Loop over all imported files and verify that they actually exist
foreach(target ${_IMPORT_CHECK_TARGETS} )
  foreach(file ${_IMPORT_CHECK_FILES_FOR_${target}} )
    if(NOT EXISTS "${file}" )
      message(FATAL_ERROR "The imported target \"${target}\" references the file
   \"${file}\"
but this file does not exist.  Possible reasons include:
* The file was deleted, renamed, or moved to another location.
* An install or uninstall procedure did not complete successfully.
* The installation package was faulty and contained
   \"${CMAKE_CURRENT_LIST_FILE}\"
but not all the files it references.
")
    endif()
  endforeach()
  unset(_IMPORT_CHECK_FILES_FOR_${target})
endforeach()
unset(_IMPORT_CHECK_TARGETS)

# This file does not depend on other imported targets which have
# been exported from the same project but in a separate export set.

# Commands beyond this point should not need to know the version.
set(CMAKE_IMPORT_FILE_VERSION)
cmake_policy(POP)

recipes/szip/all/conanfile.py

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

paulharris · 2022-10-19T06:34:50Z

installing this from scratch... I noticed the shared export is missing a public definition
set_target_properties(szip-shared PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "SZ_BUILT_AS_DYNAMIC_LIB=1"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"

Great eyes, thanks, I've added it

paulharris · 2022-10-19T07:47:57Z

Some kind of cross-compile problem?
The recipe has:

        if can_run(self) and self.options.enable_large_file:
            # Assume it works, otherwise raise in 'validate' function
            tc.variables["TEST_LFS_WORKS_RUN"] = True
            tc.variables["TEST_LFS_WORKS_RUN__TRYRUN_OUTPUT"] = True

And then the error is:

Make Error: TRY_RUN() invoked in cross-compiling mode, please set the following cache variables appropriately:
   TEST_LFS_WORKS_RUN (advanced)
   TEST_LFS_WORKS_RUN__TRYRUN_OUTPUT (advanced)
For details see /Users/jenkins/w/prod/BuildSingleReference/.conan/data/szip/2.1.1/_/_/build/a507cd2ddb8bd6879372578d0db199ba72df440f/build/Release/TryRunResults.cmake

The recipe used tools.cross_building(self, skip_x64_x86=True), I thought it was correct to switch that to can_run() ... no?

… like cross-building mac -> mac arm

prince-chrismc · 2022-10-20T16:24:06Z

For HDF5 and other existing cmake projects, they are linking to szip like:
target_link_libraries(thing ${SZIP_LIBRARIES})

@paulharris if downstream projects are not using the correct CMake targets we should open PRs to help them improve their CMake

as for the variables being the wrong casing

a0a72a3

there are cases thats also true the openssl recipes has some examples for adding it for the new generators as well

(sorry for the spam)

paulharris · 2022-10-21T00:09:25Z

Call me crazy but their CMake never actually set it to true thinking

This is what drives me to the edge, I just want to get it working and move back to my own work!
Conan-v2 and modern CMake is such a big step in the direction of keeping my sanity!

Including legacy finders. See new target_legacy_package and target_v1_legacy_package

paulharris · 2022-10-21T01:21:51Z

@paulharris if downstream projects are not using the correct CMake targets we should open PRs to help them improve their CMake

I've done that in the past. HDF5 in particular is active, and there are some recent github issues related to their cmake.
But as mentioned here, #13550 (comment)
we don't really want to set the future direction here first...

and, if we aim to support the older versions of HDF5, etc, we still need this recipe to work with the older versions of that library.
We can patch what we have in CCI, but we can't patch other projects outside of CCI.

(sorry for the spam)

More spam = more I can get some understanding. I still feel like I'm in the dark most of the time.

So, how do I write package_info() if I would like to support for both Conan v2 and v1:

find_package(szip CONFIG) and target_link_libraries(thingy szip::szip) (because it seems right)
find_package(szip CONFIG) and target_link_libraries(thingy szip-shared) (or szip-static, depending on what was built)
find_package(SZIP) and target_link_libraries(thingy ${SZIP_LIBRARIES}) (support legacy)
find_package(SZIP) and target_link_libraries(thingy szip-shared) (or szip-static, depending on what was built)

The first one is the only new item in the list.

I'm pushing a commit that does all of these, AND adds some extra tests - I wonder if we can ask the CI to run them?
They had to be separate to the existing tests as I wanted to test find_package(SZIP) as opposed to find_package(szip) and I don't think they can be both in the same cmake file.
So now there is

test_package
test_v1_package
test_legacy_package
test_v1_legacy_package

Where "legacy" refers to find_package(SZIP) and SZIP_FOUND and SZIP_LIBRARIES etc ... so that we have find_package(szip) as the primary, as per @prince-chrismc suggestion above.

What needs to change?

paulharris · 2022-10-21T01:58:22Z

I can't figure it how to support ${SZIP_LIBRARIES} in test_legacy_package (ie conan v2 mode).
Is it something we can support?

If not, then that means the HDF5 (and other) packages will stop working during the conan v1->v2 switch.

prince-chrismc · 2022-10-21T15:40:39Z

recipes/szip/all/conanfile.py

-        tools.save(module_file, content)
+        content += """\
+                set (SZIP_FOUND TRUE)
+                set (SZIP_LIBRARIES szip::szip)


this does not exist 😕

This is my attempt to tackle this problem: #13551 (comment)
would appreciate some guidance ...

So, how do I write package_info() if I would like to support for both Conan v2 and v1:

find_package(szip CONFIG) and target_link_libraries(thingy szip::szip) (because it seems right)

find_package(szip CONFIG) and target_link_libraries(thingy szip-shared) (or szip-static, depending on what was built)

find_package(SZIP) and target_link_libraries(thingy ${SZIP_LIBRARIES}) (support legacy)

find_package(SZIP) and target_link_libraries(thingy szip-shared) (or szip-static, depending on what was built)

Out of all of these only the second one is "official"

As you've pointed out, we do not want to set the direction, szip is unmainteed AFAIK so there's no way to add to it at this point

Ok, so just patch HDF5 (and others) to link to whichever target is available with the if (TARGET szip-shared) trick in the test_package

prince-chrismc · 2022-10-21T16:34:54Z

Call me crazy but their CMake never actually set it to true thinking

This is what drives me to the edge, I just want to get it working and move back to my own work! Conan-v2 and modern CMake is such a big step in the direction of keeping my sanity!

This is exactly why we need to match szip targets (and not HDF5's custom variables) lol -- I couldn't agree more 🤣

It's a matter of perspective, since you require HDF5 your concern is around that library, making sure it works. And that's totally okay (most likely a good thing). I don't use either of these so I have a very broad CCI contributor view -- so there's a very good middle ground to make both of us happy.
But HDF5 has "bad" (in quotes since this is my perspective) CMake scripts doing weird hacky stuff which works in isolation for their project. It does not work though at "the C++ package manager" level and it's a huge challenge that we have here in CCI.

I know those are bad because Conan does expose imaginary variables, there's a bunch of small issues with the old generators, relying on those is bad too. That's why we've landed on focusing on exposing the official targets. And this we agree on

But as mentioned here, #13550 (comment)
we don't really want to set the future direction here first...

We already have to patch out everything you are trying to get to work

conan-center-index/recipes/hdf5/all/patches/conanize-link-szip-1.12.2+.patch

Line 15 in f2a80c1

- set (LINK_COMP_LIBS ${LINK_COMP_LIBS} ${SZIP_LIBRARIES})

for the oldest of the generators, so it never really worked from the Conan side.

conan-center-index/recipes/hdf5/all/patches/conanize-link-szip-1.8.21.patch

Line 39 in f2a80c1

+ set (LINK_COMP_LIBS ${LINK_COMP_LIBS} "CONAN_PKG::${CONAN_SZIP_LIBNAME}")

this line needs to be fixes with the same code that's in the test packages if target ... else static stuff. (so it works in both v1 and v2).

We can not afford to add extras to szip solely for HDF5, its a dangerous slippery slope because then we'd need to add tons of wrong targets to every recipe for all the other downstream projects. It would get out of hand quickly. Take a look at the folly patches, those are horrible - we can not patch all those deps for that one project.

If not, then that means the HDF5 (and other) packages will stop working during the conan v1->v2 switch.

That's exactly why we have a test_v1_package thats using the cmake_find_package[_multi] and so the targets are correctly exposed. If recipes correctly use their dependencies targets then both new and old generators will have the exact same behavior.

What will need to happen is this needs to be updated to match szip's build script and installation.

Then in the HDF5 recipe we will need the toolchain to set the values in their custom find szip module https://github.com/HDFGroup/hdf5/blob/c8fdd92cd4d53e5343b475b85c013de60ba13555/config/cmake/FindSZIP.cmake
and maybe we need a wrapper to make sure we alias the targets since that project alone needs

quickly searching github https://github.com/search?l=CMake&q=FindSZIP&type=Code, it's only the HDF5 project that's using this (or projects using it) so it does not make sense to expose this to everything that does not need it

prince-chrismc · 2022-10-21T16:38:51Z

TL;DR

Dont waste time in this recipe trying to fix HDF5 - there no way to "not invent/start things here first" and make HDF5 work

Let's work on fixing the HDF5 recipe, with toolchains we have a lot more options about passing in values which should be more then enough to get (at least) less patches in HDF5 (and hopefully none)

paulharris · 2022-10-22T00:16:10Z

Thanks @prince-chrismc I appreciate all your work and text here.
Perhaps this can be in a policy document somewhere in the "how to make packages" and "how to upgrade for v2", because there must be quite a few of these situations.

prince-chrismc · 2022-10-22T03:41:54Z

Perhaps this can be in a policy document somewhere in the "how to make packages" and "how to upgrade for v2", because there must be quite a few of these situations.

Thats my job to write those!

This has been something on my mind for a while, but I've struggled with a title. Maybe along side #13673 I will try to bring this in 💟

paulharris · 2022-10-25T03:56:33Z

Ok I simplified the recipe a bunch, please review.

prince-chrismc · 2022-10-25T20:22:52Z

recipes/szip/all/conanfile.py

        cmake.install()
-        # TODO: to remove in conan v2 once cmake_find_package* generators removed
-        self._create_cmake_module_alias_targets(


Quickly this should be required for the test_v1_package 🤔

You should check the https://github.com/conan-io/conan-center-index/tree/master/docs/package_templates/cmake_package for a reference about the 2.0 migration

Theres extras in here I am not sure about

If you'd like we can do a zoom to finish this up and it will be easier to work instead of the horrible time zone gap

Chris was awesome in the zoom, and so now I'm a little bit wiser than before :)

conan-center-bot · 2022-10-26T06:36:42Z

All green in build 22 (aabb127e34a9eea5e06d2029a8594fdd90c57298):

szip/2.1.1@:
All packages built successfully! (All logs)

github-actions bot mentioned this pull request Oct 17, 2022

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

conan-center-bot added the Failed label Oct 17, 2022

This comment has been minimized.

Sign in to view

paulharris mentioned this pull request Oct 17, 2022

[question] why do we use _create_cmake_module_alias_targets so much? #13550

Closed

This comment has been minimized.

Sign in to view

prince-chrismc reviewed Oct 17, 2022

View reviewed changes

recipes/szip/all/conanfile.py Outdated Show resolved Hide resolved

prince-chrismc reviewed Oct 17, 2022

View reviewed changes

uilianries reviewed Oct 18, 2022

View reviewed changes

recipes/szip/all/conanfile.py Outdated Show resolved Hide resolved

szip - modernize

0c395e5

paulharris force-pushed the szip-plain-target branch from e6dceb8 to 0c395e5 Compare October 18, 2022 13:09

This comment has been minimized.

Sign in to view

szip - fixes suggested by linter, and test other target

2d42be9

This comment has been minimized.

Sign in to view

prince-chrismc reviewed Oct 19, 2022

View reviewed changes

recipes/szip/all/conanfile.py Outdated Show resolved Hide resolved

Update recipes/szip/all/conanfile.py

cf83cd4

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

This comment has been minimized.

Sign in to view

Added public definition, removed szip::szip

d79b0f7

This comment has been minimized.

Sign in to view

Fixup test_package

484306c

This comment has been minimized.

Sign in to view

Switch back from can_run() to cross_building(), the make file doesn't…

cba8987

… like cross-building mac -> mac arm

conan-center-bot removed the Failed label Oct 19, 2022

conan-center-bot removed the Failed label Oct 20, 2022

This comment has been minimized.

Sign in to view

Tweak aliases and module-alias, try and support everything

2ff3cd6

Including legacy finders. See new target_legacy_package and target_v1_legacy_package

Fixed tests - one won't pass

44b3533

This comment has been minimized.

Sign in to view

prince-chrismc reviewed Oct 21, 2022

View reviewed changes

prince-chrismc mentioned this pull request Oct 24, 2022

Implement CMakeDeps configuration to override dependencies information conan-io/conan#12367

Closed

Remove cmake_find_package lines

6bfa03b

conan-center-bot added the Failed label Oct 25, 2022

This comment has been minimized.

Sign in to view

prince-chrismc reviewed Oct 25, 2022

View reviewed changes

paulharris added 2 commits October 26, 2022 06:42

Put finder module back in

87754ca

Remove legacy tests

aabb127

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Oct 26, 2022

conan-center-bot requested a review from franramirez688 October 26, 2022 12:03

prince-chrismc approved these changes Oct 26, 2022

View reviewed changes

uilianries approved these changes Oct 28, 2022

View reviewed changes

conan-center-bot merged commit 2a4b936 into conan-io:master Oct 28, 2022

paulharris deleted the szip-plain-target branch October 28, 2022 07:43

szip: conan v2 support #13551

szip: conan v2 support #13551

Conversation

paulharris commented Oct 17, 2022

This comment has been minimized.

github-actions bot commented Oct 17, 2022

This comment has been minimized.

github-actions bot commented Oct 17, 2022

This comment has been minimized.

This comment has been minimized.

prince-chrismc left a comment

Choose a reason for hiding this comment

paulharris commented Oct 18, 2022

This comment has been minimized.

paulharris commented Oct 18, 2022

This comment has been minimized.

prince-chrismc left a comment

Choose a reason for hiding this comment

This comment has been minimized.

paulharris commented Oct 19, 2022

This comment has been minimized.

This comment has been minimized.

paulharris commented Oct 19, 2022 • edited Loading

prince-chrismc commented Oct 20, 2022

This comment has been minimized.

paulharris commented Oct 21, 2022 • edited Loading

paulharris commented Oct 21, 2022

paulharris commented Oct 21, 2022

This comment has been minimized.

prince-chrismc Oct 21, 2022

Choose a reason for hiding this comment

paulharris Oct 21, 2022

Choose a reason for hiding this comment

prince-chrismc Oct 21, 2022

Choose a reason for hiding this comment

paulharris Oct 22, 2022

Choose a reason for hiding this comment

prince-chrismc commented Oct 21, 2022

prince-chrismc commented Oct 21, 2022

paulharris commented Oct 22, 2022

prince-chrismc commented Oct 22, 2022

paulharris commented Oct 25, 2022

This comment has been minimized.

prince-chrismc Oct 25, 2022

Choose a reason for hiding this comment

paulharris Oct 27, 2022

Choose a reason for hiding this comment

This comment has been minimized.

conan-center-bot commented Oct 26, 2022

paulharris commented Oct 19, 2022 •

edited

Loading

paulharris commented Oct 21, 2022 •

edited

Loading