Skip to content

[libgeotiff] Fix usage #25636

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.

Sign up for GitHub

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

Closed
JackBoosY wants to merge 13 commits into from
Closed

[libgeotiff] Fix usage #25636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a8f7294
[libgeotiff] Fix usage
Jul 8, 2022
a931966
[liblas] Fix usage
Jul 8, 2022
c3b5253
version
Jul 8, 2022
010c70f
typo
Jul 8, 2022
d2a744d
version
Jul 8, 2022
b505ea6
Add license
Jul 8, 2022
64ba0d6
version
Jul 8, 2022
fbe44df
apply suggestions
Jul 8, 2022
7bc887e
version
Jul 8, 2022
022e74b
typo
Jul 8, 2022
75f6d32
version
Jul 8, 2022
e026ea7
Revert liblas changes
Jul 11, 2022
2f82119
version
Jul 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ports/libgeotiff/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,5 @@ endif()

file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include" "${CURRENT_PACKAGES_DIR}/debug/share")

file(INSTALL "${CMAKE_CURRENT_LIST_DIR}/usage" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}")
file(INSTALL "${SOURCE_PATH}/libgeotiff/LICENSE" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}" RENAME copyright)
26 changes: 21 additions & 5 deletions ports/libgeotiff/public-dependencies.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
diff --git a/libgeotiff/CMakeLists.txt b/libgeotiff/CMakeLists.txt
index 1840258..fcf2dd0 100644
index bd59682..5c80b9d 100644
--- a/libgeotiff/CMakeLists.txt
+++ b/libgeotiff/CMakeLists.txt
@@ -311,17 +311,18 @@ endif()
@@ -332,17 +332,18 @@ endif()
SET_TARGET_PROPERTIES(${GEOTIFF_LIBRARY_TARGET} PROPERTIES
OUTPUT_NAME ${GEOTIFF_LIB_NAME})

Expand All @@ -20,15 +20,25 @@ index 1840258..fcf2dd0 100644
if(TARGET PROJ::proj)
set(PROJ_LIBRARIES PROJ::proj)
- string(APPEND CONFIG_DEPENDENCIES " find_dependency(PROJ CONFIG)\n")
+ string(APPEND CONFIG_PRIVATE_DEPENDENCIES " find_dependency(PROJ CONFIG)\n")
+ string(APPEND CONFIG_PUBLIC_DEPENDENCIES " find_dependency(PROJ CONFIG)\n")
Copy link
Contributor

@dg0yt dg0yt Jul 11, 2022

Choose a reason for hiding this comment

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

I still think this change is wrong/not needed. It is a private dependency. It is not part of INTERFACE_LINK_LIBRARIES.
However, CMake starts to export a new property IMPORTED_LINK_DEPENDENT_LIBRARIES. If this is going to need find_dependency, you will have a lot of more ports to update.

Copy link
Contributor Author

@JackBoosY JackBoosY Jul 11, 2022

Choose a reason for hiding this comment

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

Anyway, this port requires to link with proj: #25636 (comment)

Copy link
Contributor

@dg0yt dg0yt Jul 11, 2022

Choose a reason for hiding this comment

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

This port does link with proj. That's how proj.dll comes into the list in your comment.
The question is if downstream consumers are forced to explicitly link to proj, too (public dependency).

Copy link
Contributor Author

@JackBoosY JackBoosY Jul 11, 2022

Choose a reason for hiding this comment

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

No ideas with this. Maybe we also should fix them.

Copy link
Contributor

@dg0yt dg0yt Jul 11, 2022

Choose a reason for hiding this comment

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

You have to show what is actually broken. If it ain't broken, don't fix it.

Copy link
Contributor Author

@JackBoosY JackBoosY Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In geotiff-depends-debug.cmake:

set_target_properties(geotiff_library PROPERTIES
  IMPORTED_IMPLIB_DEBUG "${_IMPORT_PREFIX}/debug/lib/geotiff_d_i.lib"
  IMPORTED_LINK_DEPENDENT_LIBRARIES_DEBUG "PROJ::proj"
  IMPORTED_LINK_INTERFACE_LIBRARIES_DEBUG "TIFF::TIFF"
  IMPORTED_LOCATION_DEBUG "${_IMPORT_PREFIX}/debug/bin/geotiff_d.dll"
  )

Doc: https://cmake.org/cmake/help/latest/prop_tgt/IMPORTED_LINK_DEPENDENT_LIBRARIES.html

Copy link
Contributor

@dg0yt dg0yt Jul 11, 2022

Choose a reason for hiding this comment

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

Yes. That's what I'm saying: PROJ::proj is not in INTERFACE_LINK_LIBRARIES. Either we don't need to deal with this property (because we don't deal with rpath), or you will have to fix another 200 ports. AFAIU, CMake "just ignores entries of IMPORTED_LINK_DEPENDENT_LIBRARIES that cannot be found", cf. https://discourse.cmake.org/t/use-cases-for-imported-link-dependent-libraries/5662.

endif()
target_link_libraries(${GEOTIFF_LIBRARY_TARGET} PRIVATE
${PROJ_LIBRARIES})
diff --git a/libgeotiff/cmake/project-config.cmake.in b/libgeotiff/cmake/project-config.cmake.in
index 3690489..87de991 100644
index 3690489..774de2d 100644
--- a/libgeotiff/cmake/project-config.cmake.in
+++ b/libgeotiff/cmake/project-config.cmake.in
@@ -22,13 +22,14 @@ set (@PROJECT_NAME@_BINARY_DIRS "${_ROOT}/bin")
@@ -13,6 +13,9 @@
# @PROJECT_NAME_UPPER@_LIBRARY
# @PROJECT_NAME_UPPER@_LIBRARIES

+cmake_policy(PUSH)
+cmake_policy(SET CMP0012 NEW)
Copy link
Contributor

@dg0yt dg0yt Jul 11, 2022

Choose a reason for hiding this comment

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

I often add the policy in wrappers, but I'm not sure if this should be extended to CMake config. Ports which use CMake config hardly run with such low cmake_minimum_required.

Copy link
Contributor Author

@JackBoosY JackBoosY Jul 11, 2022

Choose a reason for hiding this comment

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

I think this is a upstream bug, so it's not appropriate to place it in a wrapper.

Copy link
Contributor

@dg0yt dg0yt Jul 11, 2022

Choose a reason for hiding this comment

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

I didn't mean that it is to be placed in a wrapper for libgeotiff.
I mean that we should not add patches for config consumers which don't do at least cmake_minimum_required(VERSION 2.8). It is not an upstream bug if they don't care for stone-age consumers.
(And liblas had the extra issue that cmake_minimum_required appeared after project.)

+
# Tell the user project where to find our headers and libraries
get_filename_component (_DIR ${CMAKE_CURRENT_LIST_FILE} PATH)
get_filename_component (_ROOT "${_DIR}/@PROJECT_ROOT_DIR@" ABSOLUTE)
@@ -22,13 +25,14 @@ set (@PROJECT_NAME@_BINARY_DIRS "${_ROOT}/bin")
unset (_ROOT)
unset (_DIR)

Expand All @@ -45,3 +55,9 @@ index 3690489..87de991 100644
endif()

if(NOT @PROJECT_NAME@_FIND_QUIETLY)
@@ -52,3 +56,5 @@ set (@PROJECT_NAME_UPPER@_FOUND 1)
set (@PROJECT_NAME_UPPER@_LIBRARIES ${@PROJECT_NAME@_LIBRARIES})
set (@PROJECT_NAME_UPPER@_INCLUDE_DIR ${@PROJECT_NAME@_INCLUDE_DIRS})
set (@PROJECT_NAME_UPPER@_LIBRARY ${@PROJECT_NAME@_LIBRARIES})
+
+cmake_policy(POP)
2 changes: 1 addition & 1 deletion ports/libgeotiff/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "libgeotiff",
"version": "1.7.1",
"port-version": 1,
"port-version": 2,
"description": "Libgeotiff is an open source library on top of libtiff for reading and writing GeoTIFF information tags.",
"homepage": "https://github.com/OSGeo/libgeotiff",
"license": "MIT",
Expand Down
2 changes: 1 addition & 1 deletion versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3630,7 +3630,7 @@
},
"libgeotiff": {
"baseline": "1.7.1",
"port-version": 1
"port-version": 2
},
"libgit2": {
"baseline": "1.4.2",
Expand Down
5 changes: 5 additions & 0 deletions versions/l-/libgeotiff.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "7504b7420c23d466eb212fef1e4801d6731fe775",
"version": "1.7.1",
"port-version": 2
},
{
"git-tree": "aa303b0481fcc35024bae8af620ab2271ca9b5b2",
"version": "1.7.1",
Expand Down