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

SunBlack · 2023-04-18T10:59:59Z

Tried to update our manifest file and got a compile issue on liblas:

...\include\liblas/dimension.hpp(53): fatal error C1083: Cannot open include file: 'boost/foreach.hpp': No such file or directory

Also removed unnecessary dependency (searched the code via #include.*boost to check which boost modules are really required).

This issue can reproduced on the current master by calling vcpkg install liblas

Changes comply with the maintainer guide
SHA512s are updated for each updated download
The "supports" clause reflects platforms that may be fixed by this new version
Any fixed CI baseline entries are removed from that file.
Any patches that are no longer applied are deleted from the port's directory.
The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
Only one version is added to each modified port's versions file.

BillyONeal · 2023-04-19T00:07:43Z

Has this been submitted upstream?

BillyONeal · 2023-04-19T00:07:57Z

ports/liblas/remove_unnecessary_boost_dependency.diff

+
+ find_package(Threads)
+-find_package(Boost 1.42 COMPONENTS program_options thread system iostreams filesystem REQUIRED)
+find_package(Boost 1.42 COMPONENTS program_options thread system iostreams REQUIRED)


Do you need something about foreach here?

Ah indeed. Tried bcp and boostdep to check what we really need here, but unfortunately the tools don't seem to be suitable for checking what is needed in foreign code or I didn't find the right options.

ok, seems not be required within the list:

CMake Warning at C:/Program Files/CMake/share/cmake-3.26/Modules/FindBoost.cmake:2218 (message): No header defined for foreach; skipping header check (note: header-only libraries have no designated component)

MonicaLiu0311 · 2023-04-19T06:26:27Z

usage:

liblas provides CMake targets:

    # C API
    find_package(libLAS CONFIG REQUIRED)
    target_link_libraries(main PRIVATE $<$<PLATFORM_ID:Windows>:lib>las_c)

    # C++ API
    find_package(libLAS CONFIG REQUIRED)
    target_link_libraries(main PRIVATE $<$<PLATFORM_ID:Windows>:lib>las)

When testing usage, the following error occurs:

1> [CMake] CMake Error at E:/liblas/scripts/buildsystems/vcpkg.cmake:852 (_find_package):
1> [CMake]   Could not find a configuration file for package "libLAS" that is compatible
1> [CMake]   with requested version "".
1> [CMake] 
1> [CMake]   The following configuration files were considered but not accepted:
1> [CMake] 
1> [CMake]     E:/liblas/installed/x64-windows/share/liblas/liblas-config.cmake, version: 1.8.1 (cross-compiling = OFF)
1> [CMake] 
1> [CMake] Call Stack (most recent call first):
1> [CMake]   CMakeFindUsage/CMakeLists.txt:16 (find_package)
1> [CMake] -- Configuring incomplete, errors occurred!
1> [CMake] See also "C:/Users/monica/source/repos/CMakeFindUsage/out/build/x64-Debug/CMakeFiles/CMakeOutput.log".
1> 'C:\Windows\system32\cmd.exe' '/c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES\MICROSOFT VISUAL STUDIO\2022\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe"  -G "Visual Studio 17 2022" -A x64  -DCMAKE_CONFIGURATION_TYPES:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\monica\source\repos\CMakeFindUsage\out\install\x64-Debug" -DCMAKE_TOOLCHAIN_FILE=E:/liblas/scripts/buildsystems/vcpkg.cmake "C:\Users\monica\source\repos\CMakeFindUsage" 2>&1"' execution failed with error: ''C:\Windows\system32\cmd.exe' '/c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES\MICROSOFT VISUAL STUDIO\2022\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe"  -G "Visual Studio 17 2022" -A x64  -DCMAKE_CONFIGURATION_TYPES:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\monica\source\repos\CMakeFindUsage\out\install\x64-Debug" -DCMAKE_TOOLCHAIN_FILE=E:/liblas/scripts/buildsystems/vcpkg.cmake "C:\Users\monica\source\repos\CMakeFindUsage" 2>&1"' returned with exit code: 1'.

CMakeFindUsage.cpp

#include <iostream>
#include "liblas.h"
using namespace std;
int main()

{

cout << "Hello CMake." << endl;

return 0;

}

CMakeLists.txt

cmake_minimum_required (VERSION 3.8)
set(CMAKE_TOOLCHAIN_FILE "E:/liblas/scripts/buildsystems/vcpkg.cmake")
project ("CMakeFindUsage")
add_executable (main "CMakeFindUsage.cpp")
find_package(libLAS CONFIG REQUIRED)
target_link_libraries(main PRIVATE $<$<PLATFORM_ID:Windows>:lib>las)

Edit: But it is not caused by this PR.

SunBlack · 2023-04-21T17:34:19Z

Has this been submitted upstream?

Not so far. I added the patch after the fact, because I first only adjusted the dependencies and only saw through the CI that in the CMake file still unneeded modules are requested. Unfortunately, the 1.8.1 tag is so old that it is probably a completely new research on the required modules.

MonicaLiu0311 · 2023-04-24T06:08:10Z

Upstream: https://github.com/libLAS/libLAS/issues

Did you notify upstream in any way?

SunBlack · 2023-04-24T21:11:03Z

Upstream: https://github.com/libLAS/libLAS/issues

Did you notify upstream in any way?

Patch submitted there now as the includes didn't changed much: libLAS/libLAS#221

@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.

Sadly it still doesn't work. I'm now getting:

CMake Error at D:/vcpkg/installed/x64-windows/share/liblas/liblas-config.cmake:24 (include):
  include could not find requested file:

    D:/vcpkg/installed/x64-windows/share/liblas$<$<CONFIG:DEBUG>:/debug>/liblas-depends.cmake

The source within libLAS (@v1.8.1):

 get_filename_component (_DIR ${CMAKE_CURRENT_LIST_FILE} PATH) 
 get_filename_component (PROJECT_ROOT_DIR "${_DIR}/@PROJECT_ROOT_DIR@" ABSOLUTE) 
 set (libLAS_INCLUDE_DIRS "${PROJECT_ROOT_DIR}/include") 
 set (libLAS_LIBRARY_DIRS "${PROJECT_ROOT_DIR}/lib") 
 set (libLAS_BINARY_DIRS "${PROJECT_ROOT_DIR}/bin") 
  
 include ("${_DIR}/liblas-depends.cmake")

This is still correct within <vcpkg_root>\buildtrees\liblas\src\libLAS-1-<hash>.clean\cmake\liblas-config.cmake.in

After applying the patch fix-cmake-config.patch you can see the file in <vcpkg_root>\buildtrees\liblas\x64-windows-rel\cmake\liblas-config.cmake.in which results in:

get_filename_component (_DIR ${CMAKE_CURRENT_LIST_FILE} PATH)
get_filename_component (PROJECT_ROOT_DIR "${_DIR}/.." ABSOLUTE)
set (libLAS_INCLUDE_DIRS "${PROJECT_ROOT_DIR}/include")
set (libLAS_LIBRARY_DIRS "${PROJECT_ROOT_DIR}/lib")
set (libLAS_BINARY_DIRS "${PROJECT_ROOT_DIR}/bin")

include(CMakeFindDependencyMacro)
find_dependency(GeoTIFF CONFIG)
include ("${CMAKE_CURRENT_LIST_DIR}/liblas-depends.cmake")

But somehow after libLAS is compiled, the file looks like:

get_filename_component (_DIR ${CMAKE_CURRENT_LIST_FILE} PATH)
get_filename_component (PROJECT_ROOT_DIR "${_DIR}/../.." ABSOLUTE)
set (libLAS_INCLUDE_DIRS "${PROJECT_ROOT_DIR}/include")
set (libLAS_LIBRARY_DIRS "${PROJECT_ROOT_DIR}$<$<CONFIG:DEBUG>:/debug>/lib")
set (libLAS_BINARY_DIRS "${PROJECT_ROOT_DIR}/tools/liblas")

include(CMakeFindDependencyMacro)
find_dependency(GeoTIFF CONFIG)
include ("${CMAKE_CURRENT_LIST_DIR}$<$<CONFIG:DEBUG>:/debug>/liblas-depends.cmake")

Seems the following call is wrong

vcpkg/ports/liblas/portfile.cmake

Line 47 in 501db0f

    
           vcpkg_replace_string ("${CURRENT_PACKAGES_DIR}/share/liblas/liblas-config.cmake" "/lib" "$<$<CONFIG:DEBUG>:/debug>/lib")

Sadly #29259 seems to be not fully tested. Added a patch for this now - not sure whether vcpkg_replace_string is really the best solution to adjust the paths there.

Now CMake runs fine - sadly libLAS doesn't define a modern imported target, therefore the usage file needed an adjustment too (tested via including #include <liblas/version.hpp>)

BillyONeal · 2023-04-25T00:26:03Z

ports/liblas/remove_unnecessary_boost_dependency.diff

+set(Boost_HeaderOnly_Required_Components foreach interprocess lambda property_tree uuid)
+foreach(COMPONENT ${Boost_HeaderOnly_Required_Components})
+  if(EXISTS "${Boost_INCLUDE_DIR}/${COMPONENT}")
+    message(FATAL_ERROR "Header-only component \"${COMPONENT}\" of Boost is required but could not be found!")


This seems like it must be wrong: if it does exist you're emitting an error saying it does not?

The fix sadly just worked for the downloaded Boost version, but would not work here. So it is now just a comment now.

BillyONeal · 2023-04-25T00:27:45Z

ports/liblas/remove_unnecessary_boost_dependency.diff

+  endif()
+endforeach()
+
+include_directories(${Boost_INCLUDE_DIRS})


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

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.

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.

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

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.

Is merged there.

BillyONeal · 2023-04-26T06:05:17Z

ports/liblas/fix-crosscompiling.diff

@@ -0,0 +1,16 @@
+diff --git a/cmake/liblas-config-version.cmake.in b/cmake/liblas-config-version.cmake.in
+index f9b7c7cb..3ecf3ef8 100644
+--- a/cmake/liblas-config-version.cmake.in


Can you explain where this change came from?

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

@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?

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@"))

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.

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

@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.

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

(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)

Changed to the elseif(0) patch

MonicaLiu0311 · 2023-04-28T02:43:10Z

The usage test passed (header files found):

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})

BillyONeal · 2023-05-01T18:36:57Z

Dropping the upstream changes tag because they merged it. I did respond about the CMAKE_CROSSCOMPILING patch because that does not look correct to me.

SunBlack force-pushed the liblas_fix_missing_boost_foreach branch from 0cdca95 to d24203b Compare April 18, 2023 12:09

BillyONeal reviewed Apr 19, 2023

View reviewed changes

jimwang118 assigned MonicaLiu0311 Apr 19, 2023

MonicaLiu0311 added category:port-bug The issue is with a library, which is something the port should already support requires:author-response labels Apr 19, 2023

MonicaLiu0311 removed the requires:author-response label Apr 24, 2023

SunBlack mentioned this pull request Apr 24, 2023

Update required Boost modules list libLAS/libLAS#221

Merged

SunBlack force-pushed the liblas_fix_missing_boost_foreach branch 2 times, most recently from b942d6a to 0d83ded Compare April 24, 2023 20:56

SunBlack force-pushed the liblas_fix_missing_boost_foreach branch 2 times, most recently from 63aada4 to 3b83dd5 Compare April 24, 2023 23:09

BillyONeal reviewed Apr 25, 2023

View reviewed changes

BillyONeal added the requires:author-response label Apr 25, 2023

SunBlack force-pushed the liblas_fix_missing_boost_foreach branch 2 times, most recently from 3d16001 to 581ea8a Compare April 25, 2023 10:23

BillyONeal reviewed Apr 26, 2023

View reviewed changes

MonicaLiu0311 removed the requires:author-response label Apr 28, 2023

BillyONeal added depends:upstream-changes Waiting on a change to the upstream project requires:author-response and removed depends:upstream-changes Waiting on a change to the upstream project labels Apr 28, 2023

SunBlack mentioned this pull request May 2, 2023

Change CMAKE_CROSSCOMPILING from ON/OFF to TRUE/FALSE #31209

Closed

[liblas] Fix missing dependency to boost-foreach

0afe350

SunBlack force-pushed the liblas_fix_missing_boost_foreach branch from 581ea8a to 0afe350 Compare May 3, 2023 09:50

BillyONeal approved these changes May 3, 2023

View reviewed changes

BillyONeal removed the requires:author-response label May 3, 2023

MonicaLiu0311 approved these changes May 4, 2023

View reviewed changes

MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label May 4, 2023

JavierMatosD merged commit 2dae865 into microsoft:master May 4, 2023

SunBlack deleted the liblas_fix_missing_boost_foreach branch May 5, 2023 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SunBlack commented Apr 18, 2023

BillyONeal commented Apr 19, 2023

BillyONeal Apr 19, 2023

SunBlack Apr 21, 2023 •

edited

Loading

SunBlack Apr 24, 2023

MonicaLiu0311 commented Apr 19, 2023 •

edited

Loading

SunBlack commented Apr 21, 2023

MonicaLiu0311 commented Apr 24, 2023

SunBlack commented Apr 24, 2023 •

edited

Loading

BillyONeal Apr 25, 2023

SunBlack Apr 25, 2023

SunBlack Apr 25, 2023

BillyONeal Apr 25, 2023

SunBlack Apr 25, 2023

BillyONeal Apr 26, 2023

SunBlack Apr 26, 2023

BillyONeal Apr 28, 2023

SunBlack Apr 29, 2023

BillyONeal Apr 26, 2023

SunBlack Apr 26, 2023

BillyONeal May 1, 2023

BillyONeal May 1, 2023

SunBlack May 2, 2023 •

edited

Loading

dg0yt May 2, 2023

SunBlack May 2, 2023

BillyONeal May 2, 2023

BillyONeal May 2, 2023

SunBlack May 3, 2023 •

edited

Loading

MonicaLiu0311 commented Apr 28, 2023

BillyONeal commented May 1, 2023

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

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

Conversation

SunBlack commented Apr 18, 2023

BillyONeal commented Apr 19, 2023

Choose a reason for hiding this comment

SunBlack Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonicaLiu0311 commented Apr 19, 2023 • edited Loading

SunBlack commented Apr 21, 2023

MonicaLiu0311 commented Apr 24, 2023

SunBlack commented Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack May 3, 2023 • edited Loading

Choose a reason for hiding this comment

MonicaLiu0311 commented Apr 28, 2023

BillyONeal commented May 1, 2023

SunBlack Apr 21, 2023 •

edited

Loading

MonicaLiu0311 commented Apr 19, 2023 •

edited

Loading

SunBlack commented Apr 24, 2023 •

edited

Loading

SunBlack May 2, 2023 •

edited

Loading

SunBlack May 3, 2023 •

edited

Loading