[LibLZMA] automatic configuration

[cenit]

Different approach to the same problem as #5625
Just as curiosity, I tried to improve our own CMakeLists.txt to export targets and better deal with debug/release configurations, while trying to be as much backwards compatible as possible.

What happens now? Is everything broken? Waiting for the CI to tell the truth!

[cenit]

please when looking diff, enable the ignore whitespace.

[Neumann-A]

I like this approach. The idea is basically to provide a config and let find_package find that one first before going into the cmake internals.

The last time I defined an exportet package I used code like this (DESTINATIONs must of course be changed):

##---------------------------------------------------------------------------------------##
##----- Package definition. 
##---------------------------------------------------------------------------------------##
include(CMakePackageConfigHelpers)

#Export as Package
set_target_properties (${PROJECT_NAME} PROPERTIES EXPORT_NAME ${PROJECT_NAME})
export(TARGETS ${PROJECT_NAME} NAMESPACE ${PROJECT_NAME}:: FILE cmake/${PROJECT_NAME}Targets.cmake)
export(PACKAGE ${PROJECT_NAME})

#Config files for find_package
configure_package_config_file(
    cmake/${PROJECT_NAME}Config.cmake.in cmake/${PROJECT_NAME}Config.cmake
    INSTALL_DESTINATION "${CONFIG_INSTALL_DIR}/${PROJECT_NAME}Config.cmake"
    PATH_VARS TARGET_INSTALL_DIR INCLUDE_INSTALL_DIR
)
write_basic_package_version_file(cmake/${PROJECT_NAME}ConfigVersion.cmake COMPATIBILITY SameMinorVersion)

#Install the target and all it build outputs.
install (TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME}Targets 
    LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}" COMPONENT Runtime
    ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}" COMPONENT Development
    RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}" COMPONENT Runtime
    PUBLIC_HEADER DESTINATION "${INCLUDE_INSTALL_DIR}" COMPONENT Development
    BUNDLE DESTINATION "${CMAKE_INSTALL_BINDIR}" COMPONENT Runtime)
#Install the config files for find_package
install(FILES "${PROJECT_BINARY_DIR}/cmake/${PROJECT_NAME}ConfigVersion.cmake"
              "${PROJECT_BINARY_DIR}/cmake/${PROJECT_NAME}Config.cmake"
        DESTINATION "${CONFIG_INSTALL_DIR}")
#install the targets file included by the config
install(EXPORT ${PROJECT_NAME}Targets NAMESPACE ${PROJECT_NAME}:: DESTINATION "${TARGET_INSTALL_DIR}")

with a config.cmake.in file like this:

@PACKAGE_INIT@

include ("@PACKAGE_TARGET_INSTALL_DIR@/@PROJECT_NAME@Targets.cmake")
set_and_check(@PROJECT_NAME@_INCLUDE_DIR "@PACKAGE_INCLUDE_INSTALL_DIR@")
check_required_components("@PROJECT_NAME@")

theoretically this could be extended to arbitrary ports

[cenit]

😉
fixing these problems requires a work upstream. The best workflow would be to submit this CMakeLists.txt upstream once we are sure enough that it is working as expected.

[seanwarren]

My only concern with this approach is that if people don't realise they need to add CONFIG to their find_package(LibLZMA) calls when they use vcpkg they'll silently end up with the debug libraries. However it's certainly a cleaner approach.

[NancyLi1013]

Here are the test results from the current CI system:

x64-windows	master	test	notes
avro-c	Pass	Fail	Regression
boost-iostreams	Pass	Fail	Regression
libxml2	Pass	Fail	Regression
tiff	Pass	Fail	Regression
libgta	Pass	Fail	Regression

x64-windows-static	master	test	notes
avro-c	Pass	Fail	Regression
boost-iostreams	Pass	Fail	Regression
libxml2	Pass	Fail	Regression
tiff	Pass	Fail	Regression
libgta	Pass	Fail	Regression

x64-osx	master	test	notes
boost-iostreams	Pass	Fail	Regression
libxml2	Pass	Fail	Regression
tiff	Pass	Fail	Regression
libgta	Pass	Fail	Regression

arm64-windows	master	test	notes
avro-c	Pass	Fail	Regression
tiff	Pass	Fail	Regression
libgta	Pass	Fail	Regression
libxml2	Pass	Fail	Regression

x86-windows	master	test	notes
avro-c	Pass	Fail	Regression
boost-iostreams	Pass	Fail	Regression
libxml2	Pass	Fail	Regression
tiff	Pass	Fail	Regression
libgta	Pass	Fail	Regression

x64-linux	master	test	notes
liblzma	Pass	Fail	Regression

x64-uwp	master	test	notes
libxml2	Pass	Fail	Regression
libgta	Pass	Fail	Regression
opencv	Pass	Fail	Regression

arm-uwp	master	test	notes
libxml2	Pass	Fail	Regression
libgta	Pass	Fail	Regression

Most libs failed and it looks like this error:
c:\vsts_work\2\s\installed\arm-uwp\include\lzma.h(292): fatal error C1083: Cannot open include file: 'lzma/version.h': No such file or directory
Please refer to the attachment to look for the detail.
failureLogs.zip
As for osx, @Rastaban, could you help provide MacOS logs?

[cenit]

@NancyLi1013 thanks. I reworked the install path for the include files, it should be better now.
@seanwarren my idea is if we find that this method works "opt-in", we can force it on everybody later with a wrapper... In the end the final solution will be a merge of our two. I also still think that if this config method is becoming too cumbersome, yours is still very easy and elegant!

[seanwarren]

@cenit - ah yes of course, sounds like a good plan!

[Neumann-A]

related: #616

[vicroms]

Attached failure logs

Here are the current CI results:

x64-windows	master	test	notes
vtk	Pass	Fail	Regression
vxl	Pass	Fail	Regression

x64-windows-static	master	test	notes
libgeotiff	Pass	Fail	Regression
vtk	Pass	Fail	Regression
libspatialite	Pass	Fail	Regression
avro-c	Pass	Fail	Regression
zxing-cpp	Pass	Fail	Regression
tesseract	Pass	Fail	Regression

x64-osx	master	test	notes
opencv	Pass	Fail	Regression
itk	Pass	Fail	Regression

x86-windows	master	test	notes
vtk	Pass	Fail	Regression
vxl	Pass	Fail	Regression

x64-linux	master	test	notes
liblzma	Pass	Fail	Regression

x64-uwp	master	test	notes
opencv	Pass	Fail	Regression

arm-uwp	master	test	notes
tmx	Pass	Fail	Regression

As usual, vxl fails when OpenJPEG is rebuilt in the same CI run, so don't worry too much about it.

[Neumann-A]

Hmm currently removing the extra "d"s from conflicting ports in #5543 and noticed something interesting. rockdbs and folly define basically the same findsnappy.cmake to find snappy for debug and release. Could we not do the same, but instead having the findsnappy in the consuming ports have it be installed by the source port in an appropriate vcpkg cmake search folder? this should also solve the required call to find_package in config mode

[cenit]

i think we already discussed creating a common folder for custom cmake module files, store them there inside that single folder instead of inside each share/portname folder and add that folder to the CMAKE_MODULE_PATH directly within vcpkg toolchain. I don't remember what blocked that idea... (note that I am talking about module files, config files should and must be kept inside specific port folders)

[Neumann-A]

@cenit Does it directly mess with cmake modules files included normally within ports/packages? Would be great if you could find the issue with the discussion about it.

[cenit]

appending (and not prepending) that folder to the searched path should not mess. It would be the last resort for a find_package().
For example I have a repository containing some module files (residual from some work with cmake) and I often use it in many projects.
Not sure where the issue is, I will search it later

[cenit]

I modified a little bit the TIFF wrapper and fixed a typo in a symbol, I hope it could be enough to fix most of the issues. In fact, I also have some doubts on the logs. Just open as an example liblzma:x64-linux, everything went fine! Why regression?

[Neumann-A]

@cenit probably post build validation which is not included in the CI logs.

[cenit]

any update on the error logs?

[cenit]

any news why CI is not running anymore here?

[cenit]

@vicroms selene on arm was definitely broken, since it was using intrinsics closely related to x86 arch. Since it was definitely broken also before I doubt that was the regression marking the red cross, but since we are just working on selene now it to finish this work I don't know what could be the problem. Waiting for the logs, on monday ofc :)

[Fleischner]

Wouldn't it be better to also supply a vcpkg_cmake_wrapper.cmake for LZMA and reroute to find_package(LZMA CONFIG) so that everybody gets the improvement until a better way is found?

Replacing custom find_package logic is the better way. Any custom find logic can at best generate havok in a system as vcpkg where files have a clear default location.

I'm not a cmake expert, but it seem as if find_package has a bad design because it combines "find files" and "pass configuration options" which are tow steps that should really be separated.

[Neumann-A]

I'm not a cmake expert, but it seem as if find_package has a bad design because it combines "find files" and "pass configuration options" which are tow steps that should really be separated.

the only thing find_package does is (excluding any additional options):

• a) look for find<packename>.cmake
• b) if not a) look for <packename>-config.cmake
  c) include a) or b) dependent on what has been found and what options have been passed to find_package

The rest of the behavior is complete up to the find module or the config. The rest of the behavior is complete up to the included *.cmake file. That is also why vcpkg_cmake_wrapper.cmake is totally superflous because you got get the same behavior by defining a config or find module. The main difference between the config and the find module is: the config is generated by the project to be consumed. So it knows where it has been installed to and how it has been build. It thus encodes the paths to the libraries and required definitions within the config. The find module on the other hand is supplied by the consumer and can only rely on find_library (or others like find_path, _programm etc.) calls to discover its dependencies.
Thats why you can hook into find_library calls and correct the behavior for all libraries as done in #6555 since there should be no need to correct configs if all find_library calls are correct.

[cenit]

@Neumann-A the problem with LZMA is that it's often a second-layer dependency, unfortunately nested in shared config files for some projects instead of specific target file (a common file instead of a specific build-type file)
This means that if the LZMA_LIBRARIES (or similar names) does not contain the whole optimized;lzma.lib;debug;lzmad.lib but only one type, the second layer dependency fails at build time in static mode, or fails at runtime in dynamic linkage mode. First layer dependencies have no problems when built since they see the proper library, they just save an incomplete path in their config, relying on CMake too much.
This is especially clear in all projects using OpenCV.
I hope this can clear the problem better, otherwise you can find many and many hints and descriptions in issues and previous PRs here and elsewhere.
Fixing the build scripts that vcpkg itself provides is enough to fix the issue.

About the broad scope of this PR: it is already a second level disentanglement of a long work pursued here on vcpkg, which predates many things and people. Unfortunately dependencies work like this and it's difficult to disentangle broken ports when fixing a very basic one.

If you had read the discussion here or the code you'd know that the "CONFIG" tag is not required anymore to opt-in the new LZMA mechanism since commit 375a8bc. It was used at the beginning as an opt-in to the new system, to check step-by-step if it could even work (I was sure but I needed proof). Recently it became superfluous, since it is mandatory for every LZMA request, but it was left "on" for vcpkg-supplied build scripts and patches already in place, and discussed with Victor. I recently removed them since they were adding too much noise to this PR, being absolutely not necessary.

#5543 is a flying target and unsuitable for any production environment IMHO, so it's totally out of question for me, for now. It's already a huge risk to live on the master here, since it's not considered mandatory to keep it "sane" even on a basic level (do you remember your patch that you didn't even test and was accepted too quickly? Any people cloning vcpkg during that time found a broken project, distributed by MS, and maybe decided that it was a toy and totally not deserving any other time. Wow! If many people took their time to write an issue, just imagine how many didn't even write and just abandon it...)

Declaring vcpkg_cmake_wrapper.cmake totally superfluous means that you have not fully understood the internal mechanisms of vcpkg, since that's the only point to intercept calls for find_package resolving to an official module. How would you intercept calls to a CMake "official" module otherwise?

[cenit]

@vicroms when CI will be over on this PR, please let me know results. I hope to have fixed many regressions (thanks to public CI), I couldn't understand results anymore and so now I need your help :)
I also hope this being a green mark, apart from the usual vxl/openjpeg problem... (which I'd like to tackle soon... is it the only one or are there more?)

[Neumann-A]

they just save an incomplete path in their config

so its that issue:

Is a secondary dependency to LZMA linking against the wrong LZMA and the direct dependency is exporting a config with the wrong information? (Here the config should probably be patched)

I fully understand this issue. You could have done a local CI build of master where everything "works/compiles" and then start a search for "lzma" in the installed/share folders and find all the incomplete configs and just fix them for the time being. (That would have been a simple replace operation within all configs.)

Unfortunately dependencies work like this and it's difficult to disentangle broken ports when fixing a very basic one.

Sorry your are touching 11 ports in this PR and 8 or so could be free standing PRs correcting behavior independent of what has been done to correct LZMA. In my opinion you should just split those off and get them merged into master already. (If you do that they probably get merged in less then 2 days.)

#5543 is a flying target

yeah it is still a WIP because I spend too much time trying to fix find_package. Should have started with find_library from the beginning and currently trying to find all the corner cases which must be properly considered. But let me tell you: It will be the only way to fix all those LZMA cases within vcpkg with a reasonable amount of work which also works for multi configuration generators. There are many libraries which do not even use find_package but use find_library instead and a lot of them don't even have the names right either missing or adding debug suffixes. Furthermore all find_package implementation also rely on find_library. (The alternativ would be find modules for all affected ports.)

How would you intercept calls to a CMake "official" module otherwise?

From https://cmake.org/cmake/help/v3.14/command/find_package.html

The file is first searched in the CMAKE_MODULE_PATH, then among the Find Modules provided by the CMake installation.

so vcpkg could have just define/appended to CMAKE_MODULE_PATH to get the same behavior as vcpkg_cmake_wrapper.cmake with the benefit that people actually know how that works and how to use it. I am strongly for deprecating vcpkg_cmake_wrapper and actually introducing a module path. With good find modules we could probably deprecate the _find_package "hack" which is as far as i know undocumented cmake behavior. (yeah I know I use it myself in my PR but I am not strictly against that "hack" because it is really good for debugging issues.)

[cenit]

You could have done a local CI build of master where everything "works/compiles" and then start a search for "lzma" in the installed/share folders and find all the incomplete configs and just fix them for the time being. (That would have been a simple replace operation within all configs.)

what do you think people do to work ;) of course that's the way we use it now, it was just deserving a proper fix

Sorry your are touching 11 ports in this PR and 8 or so could be free standing PRs correcting behavior independent of what has been done to correct LZMA

it's just enabling thanks to a proper LZMA port that revealed those problems. I don't need them, but it's nice to have them fixed.

_find_package "hack"

why do you call it hack? You know that every overloaded function in CMake is kept in the original form with an underscore prepended? That's documented and well known, I'd not call it a hack

[cenit]

so vcpkg could have just define/appended to CMAKE_MODULE_PATH

I'd not mess with CMAKE_MODULE_PATH. That's the perfect way to break many projects that use it. On the contrary, I appreciate @ras0219-msft idea of vcpkg_cmake_wrapper.cmake

[cenit]

yeah it is still a WIP because I spend too much time trying to fix find_package. Should have started with find_library from the beginning and currently trying to find all the corner cases which must be properly considered. But let me tell you: It will be the only way to fix all those LZMA cases within vcpkg with a reasonable amount of work which also works for multi configuration generators

I don't think so. From my experience there can't be a solution that works for everything, and here in particular since it's a bazaar by definition.
You just have to slowly fix the mess, piece after piece... just think about the C++ developer situation 4 years ago on Windows when dealing with non-microsoft libraries and where it is now. Just tell me if it was even considered "smart" to develop on Windows instead of Linux/Mac, due to the fact that almost no library worked out of the box and the fact that it looked like nothing was going to change.

It was a long journey, made by small steps, one after the other, on many fronts (compiler, tooling, many volunteers fixing code, ...), but it wouldn't be possible in any other easier way.
I appreciate your work, I have some doubt about your method from time to time (your famous #5543 PR would have been closed long ago in many other projects, since it's clear that you're learning CMake in the meantime and you're "abusing" the system here, with a clear message of "please resubmit when ready "), but still it's clear your dedication and the time spent fixing problems, and here any help is really appreciated, so I agree with admins on letting it open because it's better so ;)

[Neumann-A]

it's just enabling thanks to a proper LZMA port that revealed those problems. I don't need them, but it's nice to have them fixed.

That was the same for my PR and that’s why I split them from the main PR. It is a way of preparing master for another PR.

why do you call it hack? You know that every overloaded function in CMake is kept in the original form with an underscore prepended? That's documented and well known, I'd not call it a hack

What happens if an external toolchain also overwrites the same function? Then _find_package will no longer be what you think it will be, infinite loop included (https://crascit.com/2018/09/14/do-not-redefine-cmake-commands/)

I'd not mess with CMAKE_MODULE_PATH. That's the perfect way to break many projects that use it.

It probably does not break as many as you think. Also it is easily checked if CMAKE_MODULE_PATH has been altered and if delegating to a different find module is necessary.
So vcpkg buildsystem has a little bit more knowledge about internals of a port and could further provide meaningful configure options (like VCPKG_WITHOUT_INTERNAL_MODULE_<PORT>) to activate and deactivate certain find modules if necessary on a general per port basis.
Furthermore I see an advantage in the fact that the filenames are all different and that they are all installed into a common folder which makes editing/working with them a lot easier. The main disadvantage of vcpkg_cmake_wrapper is that it adds a third way of how find_package works within vcpkg and at least I never know if CMake uses the Wrapper or its normal lookup by just looking at the configure logs.
And lets be real here: find_package just includes the file it finds and vcpkg_cmake_wrapper is also just a file which is included in the toolchain file.
So everything which can be done with a module can be done by the wrapper but it is also the other way around. As such the wrapper is superfluous because its just adds another way/alternative to solve the same problem which could be solved by a proper find module. As such the wrapper is just a find module which is just not called that way. Maybe we should rename them all to vcpkg_find_lowercaseportname.cmake and install them into a common shared folder instead into separate folders where they are hard to spot. (Maybe that’s my main point why I don’t like them. They are all scattered around and hard to directly spot apart from the fact that they basically offer the same functionality as find modules.....)

I don't think so. From my experience there can't be a solution that works for everything, and here in particular since it's a bazaar by definition.

And I thought you are a theoretical physicist ;). Have you given up on the theory which unifies everything? Even though it is a bazaar I see a lot of common usage patterns within all ports which can be dealt with on a general basis.

It was a long journey, made by small steps, one after the other, on many fronts (compiler, tooling, many volunteers fixing code, ...), but it wouldn't be possible in any other easier way.

A long journey which also included a lot of trial and error just like my PRs ;) and since your a speaking of small steps do you mind splitting your PR ;)

And there was probably also a lot of heated discussions on how to move forward but just how it is. People have different views on how to solve the same problem and either the maintainers or the crowed will decide how it will be solved.

your PR

The PR actually reached a point were it completly worked with just fixing find_package but I decided to restart from scratch because I could do a lot more by overwritting find_library (e.g. searching for libraries with common debug suffixes) with less changes to different ports. Unfortunally there where some side effects which had to be dealt with: like IMPORT_LOCATION not accepting generator expression under some circumstances and so on. That is not something you can figure out by reading the docs alone.

[cenit]

And I thought you are a theoretical physicist ;). Have you given up on the theory which unifies everything?

😆 good catch...

@Neumann-A Thanks for the long explanation of your point of view. I really appreciated reading it. Now I think it’s better to go back in topic. I hope also to have some feedback about regressions here (maybe only vxl? who knows :) )

[cenit]

@vicroms I merged the master. SInce the vxl problem should be gone, I'm targeting a green mark 😄
But I am sure I'll need your help to have regressions...

[vicroms]

No more regressions! 🎉

As a side effect this PR also enables some new ports:

x64-windows	test	notes
qwt	Pass
qt5	Pass
osg-qt	Fail	Previously skipped, not a regression
qt5-svg	Pass
osg	Pass
osgearth	Pass
qt5-imageformats	Pass

x64-osx	test	notes
shiva-sfml	Fail	Previously skipped, not a regression
shiva	Pass

arm64-windows	test	notes
selene	Pass

x86-windows	test	notes
qwt	Pass
qt5	Pass
osg-qt	Fail	Previously skipped, not a regression
qt5-svg	Pass
osg	Pass
osgearth	Pass
qt5-imageformats	Pass

x64-linux	test	notes
shiva-sfml	Fail	Previously skipped, not a regression
shiva	Pass

x64-uwp	test	notes
podofo	Pass

arm-uwp	test	notes
podofo	Pass
selene	Pass

[cenit]

@vicroms Thanks. To be honest (said only for debug purposes) in reality I enabled more ports than those you listed. For example OpenSSL was not working on any form of arm64, but it's not listed there (arm64-windows, arm64-uwp, ...)
That was also not the only one. I don't know what happens, maybe there's something bugged happening in result collection, which in the worst case is also letting problems slip in? @Rastaban