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.

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

Jump to bottom

[liblas] Fix missing dependency to boost-foreach #30950

Merged
merged 1 commit into from
May 4, 2023
Merged

[liblas] Fix missing dependency to boost-foreach #30950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions ports/liblas/fix-crosscompiling.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/cmake/liblas-config-version.cmake.in b/cmake/liblas-config-version.cmake.in
index f9b7c7cb..5dd2aba1 100644
--- a/cmake/liblas-config-version.cmake.in
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain where this change came from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this entry: #30950 (comment), where I have also noted from where I copied this solution.

Copy link
Member

Choose a reason for hiding this comment

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

@MonicaLiu0311: I could reproduce your issue. It seems to be the same issue like here: #8104 As this only happens when compiling with vcpkg it seems to be a vcpkg issue and no libLAS issue (compiling libLAS without vcpkg: CMAKE_CROSSCOMPILING is replaced by FALSE; compiling libLAS via vcpkg: CMAKE_CROSSCOMPILING is replaced by OFF). Therefore added a similar patch.

FALSE and OFF are both 'falsy' as far as CMake is concerned. It looks like this config is just broken by trying to use STREQUAL for a boolean test.

Importantly, this patch breaks what upstream is trying to do, since if CMAKE_CROSSCOMPILING is true when building the thing, the test is damaged.

It seems the correct fix is:

-elseif (NOT CMAKE_CROSSCOMPILING STREQUAL "@CMAKE_CROSSCOMPILING@")
+elseif (NOT CMAKE_CROSSCOMPILING EQUAL "@CMAKE_CROSSCOMPILING@")

and the line below should not be messed with at all since it's just logging output and it should record the real values?

Copy link
Member

Choose a reason for hiding this comment

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

Alternately, if EQUAL does not work becuase that wants them to be numbers it needs to be built out of boolean tests, like:

-elseif (NOT CMAKE_CROSSCOMPILING STREQUAL "@CMAKE_CROSSCOMPILING@")
+elseif ((CMAKE_CROSSCOMPILING AND NOT "@CMAKE_CROSSCOMPILING@")
+    OR (NOT CMAKE_CROSSCOMPILING AND "@CMAKE_CROSSCOMPILING@"))

Copy link
Contributor Author

@SunBlack SunBlack May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

EQUAL works indeed not (like expected):

set(varOff OFF)
set(varFALSE FALSE)

if(varOFF)
	message("varOff: true")
else()
	message("varOff: false")
endif()

if(varFALSE)
	message("varFALSE: true")
else()
	message("varFALSE: false")
endif()

if(OFF EQUAL FALSE)
	message("Equal")
else()
	message("Not equal")
endif()

Returned:

varOff: false
varFALSE: false
Not equal

Btw: Die real fix would be to fix vcpkg, as :

cmake_minimum_required(VERSION 3.15)
project(...)
message(${CMAKE_CROSSCOMPILING})

Returns FALSE and not OFF, but somehow vcpkg sets this variable to OFF. So we wouldn't need a fix here, when vcpkg would do it correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this particular elseif can be simplified to elseif(0). Its benefit is close to zero in a vcpkg triplet universe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BillyONeal I created a separate issue for this, as I think it is better, when this issue is fixed in vcpkg #31209

@dg0yt: As I have not set up cross compilation locally, I can't say anything about it since I can't really test it and have no experience with it.

Copy link
Member

Choose a reason for hiding this comment

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

Returns FALSE and not OFF, but somehow vcpkg sets this variable to OFF. So we wouldn't need a fix here, when vcpkg would do it correct.

CMAKE_CROSSCOMPILING is documented as being 'boolish' so all of 0, OFF, NO, FALSE, N, and IGNORE are acceptable and equivalent, see https://cmake.org/cmake/help/latest/command/if.html#basic-expressions

Copy link
Member

Choose a reason for hiding this comment

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

(I agree with @dg0yt in that making this elseif(0) could be fine. Just acting like CMAKE_CROSSCOMPILING is always false, even when it is actually true as in e.g. x64-uwp, is not the correct fix)

Copy link
Contributor Author

@SunBlack SunBlack May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Changed to the elseif(0) patch

+++ b/cmake/liblas-config-version.cmake.in
@@ -22,7 +22,7 @@ elseif (MSVC AND NOT MSVC_VERSION STREQUAL "@MSVC_VERSION@")
# Reject if there's a mismatch in MSVC compiler versions
set (REASON "_MSC_VER = @MSVC_VERSION@")
set (PACKAGE_VERSION_UNSUITABLE TRUE)
-elseif (NOT CMAKE_CROSSCOMPILING STREQUAL "@CMAKE_CROSSCOMPILING@")
+elseif (0)
# Reject if there's a mismatch in ${CMAKE_CROSSCOMPILING}
set (REASON "cross-compiling = @CMAKE_CROSSCOMPILING@")
set (PACKAGE_VERSION_UNSUITABLE TRUE)
6 changes: 4 additions & 2 deletions ports/liblas/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ vcpkg_extract_source_archive(
PATCHES
fix-boost-headers.patch
fix-cmake-config.patch
fix-crosscompiling.diff
misc-fixes.patch
remove_unnecessary_boost_dependency.diff
)

file(REMOVE_RECURSE "${SOURCE_PATH}/cmake/modules")
Expand Down Expand Up @@ -44,8 +46,8 @@ else()
endif()

vcpkg_replace_string ("${CURRENT_PACKAGES_DIR}/share/liblas/liblas-config.cmake" "_DIR}/.." "_DIR}/../..")
vcpkg_replace_string ("${CURRENT_PACKAGES_DIR}/share/liblas/liblas-config.cmake" "/lib" "$<$<CONFIG:DEBUG>:/debug>/lib")
vcpkg_replace_string ("${CURRENT_PACKAGES_DIR}/share/liblas/liblas-config.cmake" "/bin" "/tools/${PORT}")
vcpkg_replace_string ("${CURRENT_PACKAGES_DIR}/share/liblas/liblas-config.cmake" "\${PROJECT_ROOT_DIR}/lib" "\${PROJECT_ROOT_DIR}/$<$<CONFIG:DEBUG>:/debug>/lib")
vcpkg_replace_string ("${CURRENT_PACKAGES_DIR}/share/liblas/liblas-config.cmake" "\${PROJECT_ROOT_DIR}/bin" "\${PROJECT_ROOT_DIR}/tools/${PORT}")

file(REMOVE_RECURSE
"${CURRENT_PACKAGES_DIR}/debug/include"
Expand Down
22 changes: 22 additions & 0 deletions ports/liblas/remove_unnecessary_boost_dependency.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
diff --git a/CMakeLists.txt b/CMakeLists.txt
index d246a88d..634157c0 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -190,11 +190,13 @@ if(WIN32)
endif(WIN32)

find_package(Threads)
-find_package(Boost 1.42 COMPONENTS program_options thread system iostreams filesystem REQUIRED)
+find_package(Boost 1.42 COMPONENTS iostreams program_options serialization thread REQUIRED)
+
+# The following header-only and their dependencies are additionally required,
+# but cannot be explicitly requested via find_package, so make sure they exists:
+# - foreach interprocess lambda property_tree uuid

-if(Boost_FOUND AND Boost_PROGRAM_OPTIONS_FOUND)
- include_directories(${Boost_INCLUDE_DIRS})
-endif()
+include_directories(${Boost_INCLUDE_DIRS})
Copy link
Member

Choose a reason for hiding this comment

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

This is certainly wrong, it should be added by a target_link_libraries to add Boost::boost (or Boost::headers) somewhere instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libLAS requires still just CMake 2.8.12, so you imported targets are still not supported (requires CMake 3.5). They are doing later target_link_libraries(${LIBLAS_LIB_NAME} ... ${Boost_LIBRARIES}).

As the change related include_directories doesn't have any effect and is just to cleanup the code, I could remove the change and just submit this to the libLAS repo.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally for the patch we are tracking we should be making the minimum change that makes it work inside vcpkg, unless we are applying an exact change upstream did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same patch as I commited here: libLAS/libLAS#221

Copy link
Member

Choose a reason for hiding this comment

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

It is the same patch as I commited here: libLAS/libLAS#221

Upstream seems responsive so going to give them some time to consider/merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is merged there.


# make these available for the user to set.
mark_as_advanced(CLEAR Boost_INCLUDE_DIR)
2 changes: 2 additions & 0 deletions ports/liblas/usage
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ liblas provides CMake targets:
# C API
find_package(libLAS CONFIG REQUIRED)
target_link_libraries(main PRIVATE $<$<PLATFORM_ID:Windows>:lib>las_c)
target_include_directories(main SYSTEM PRIVATE ${libLAS_INCLUDE_DIRS})

# C++ API
find_package(libLAS CONFIG REQUIRED)
target_link_libraries(main PRIVATE $<$<PLATFORM_ID:Windows>:lib>las)
target_include_directories(main SYSTEM PRIVATE ${libLAS_INCLUDE_DIRS})
9 changes: 3 additions & 6 deletions ports/liblas/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
{
"name": "liblas",
"version": "1.8.1",
"port-version": 12,
"port-version": 13,
"description": "A C/C++ library for reading and writing the very common LAS LiDAR format.",
"license": null,
"supports": "!arm & !staticcrt",
"dependencies": [
"boost-detail",
"boost-filesystem",
"boost-foreach",
"boost-interprocess",
"boost-iostreams",
"boost-lambda",
"boost-multi-index",
"boost-program-options",
"boost-system",
"boost-thread",
"boost-property-tree",
"boost-uuid",
"libgeotiff",
{
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 @@ -4134,7 +4134,7 @@
},
"liblas": {
"baseline": "1.8.1",
"port-version": 12
"port-version": 13
},
"liblbfgs": {
"baseline": "1.10",
Expand Down
5 changes: 5 additions & 0 deletions versions/l-/liblas.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "a72a32ce085b262e26bcadc55efd0159fdf00c4f",
"version": "1.8.1",
"port-version": 13
},
{
"git-tree": "c8f90c515041e9c2633daee8e536e8e5e9ce0170",
"version": "1.8.1",
Expand Down