Skip to content

WIP: Trying to fix linkage issues with multi configuration generators and others#7524

Closed
Neumann-A wants to merge 72 commits intomicrosoft:masterfrom
Neumann-A:multi_config_fix
Closed

WIP: Trying to fix linkage issues with multi configuration generators and others#7524
Neumann-A wants to merge 72 commits intomicrosoft:masterfrom
Neumann-A:multi_config_fix

Conversation

@Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Aug 2, 2019

Long story short see #5543

Trying to find the problems with the changes here ;) (code still requires a lot of cleanup)

The Problem
Multi configuration generators are not able to setup CMAKE_PREFIX_PATH in a way that allows proper finding of release/debug libraries if the library is not named differently in the debug build (limitations of cmakes find_library call) or vcpkg does not "correctly" name the debug library as cmake users would expect them to be. As such multi configuration generators are currently broken with vcpkg if the consumed library is not found by a <package>config.cmake or have the cmake expected naming.

Alternative solution
Provide vcpkg_cmake_wrapper.cmake for every affect library (however many that might be)

What did not work (e.g. #5543)
Lets start with what does not work: (either after find_package or after find_library)

  1. injection generator expression in the variable set by the find_library call
  2. injection of debug/optimized keyword in the variable set by the find_library call

Main failure reason: code expects a single filepath in the variable. Generator expressions work roughly 95% of the time but there are limitations like IF(EXISTS), setting the IMPORT_LOCATION or CMAKE_REQUIRED_LIBRARIES for function signature checks.

How does this solution work
Since fixing variables cannot be done in the search/find stage we have to fix the paths in the link stage/setup stage.
Explained in steps:

  1. Do a normal find_library call
  2. check if the library found is prefixed within the vcpkg path -> if not do nothing else go to 3)
  3. Since we know everything about VCPKG's layout and also know the names used for the search we ignore whatever we found and do the following:
    a) Use the search names to search explicitly in the release and debug folder (also includes a search with and without common debug suffixes)
    b) setup cache variables VCPKG_LIBTRACK_<libname_rel>_RELEASE , VCPKG_LIBTRACK_<libname_dbg>_RELEASE, VCPKG_LIBTRACK_<libname_dbg>_DEBUG , VCPKG_LIBTRACK_<libname_rel>_DEBUG to point to the expected paths
    c) return the appropriate filepath depending on the given variable name _RELEASE/_DEBUG or CMAKE_BUILD_TYPE
    d) if c) is inconclusive return whatever the initial find_library call found (current behavior)
  4. If a target is created and the property IMPORTED_LOCATION or IMPLIB_LOCATION is set do the following:
    a) check (IMPORTED|IMPLIB)_LOCATION_(RELEASE|DEBUG) for correctness
    b) if only (IMPORTED|IMPLIB)_LOCATION is set. Setup (IMPORTED|IMPLIB)_LOCATION_(RELEASE|DEBUG) using VCPKG_LIBTRACK_<libname>_(RELEASE|DEBUG)
    c) if any required VCPKG_LIBTRACK_ variable cannot be found -> ERROR
  5. Fix target_link_libraries and link_libraries calls.
    a) scan list for paths prefixed in the vcpkg directory -> if not prefixed do nothing else go to b)
    b) check if the element before the prefixed path is a keyword (debug|optimized|general) -> check path (should be correct; error otherwise)
    c) if no keyword replace the path with one including keywords and paths to the debug and release version of the library using VCPKG_LIBTRACK_<libname>_(RELEASE|DEBUG)
  6. ???
  7. Profit from correct linkage in multi configuration generators using vcpkg

Possible problems
In 3a) 3b): If the library names in debug/release are not following common conventions there might be problems in setting up VCPKG_LIBTRACK_<libname>_(RELEASE|DEBUG).

Other Benefits
Since 3a) searches with and without common debug suffixes the library naming is not that important and common debug suffixes might be dropped or added to libraries as VCPKG likes to do it (no cmake "blessed" lib names).

How to merge this PR in the future
Split it up in three different PRs:

  1. extract the functions from the toolchain file in separate files per overwritten function
  2. introduce the function_overwrite_guard/function_recurssion_guard to detect redefinition of functions in external toolchain files.
  3. add the actual fix using VCPKG_LIBTRACK.

CMake generator expressions and lists do not play nicely and
require a lot of attention to handle correctly.
For some reason cmake generates an invalid ninja build script
-> made a bug report to cmake
@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 9, 2019

TODO:

  • fluidsynth: return to ninja; target_link_libraries in tests are generating ninja build failure patch it to use $<GENEX_EVAL:expr>
  • urfdom (more precise control_bridge): fix control_bridge config. library variables are CACHE variables and not local to scope (either that or simply define find_library variables also as CACHE and LOCAL_SCOPE)
  • clblas : no plan -> some strange error
  • liblas : patch it to use geotiff config

# Conflicts:
#	ports/cpprestsdk/CONTROL
#	ports/pybind11/CONTROL
#	scripts/buildsystems/vcpkg.cmake
@Neumann-A
Copy link
Contributor Author

Neumann-A commented Aug 29, 2019

Remaining regressions:

  • osg -> no debug folder generated for some really strange reason ??? set(CMAKE_INSTALL_CONFIG_NAME "Release") in debug cmake_install.cmake seems to be the default
  • tensorflow -> build errors unrelated to this pr
  • quickfix -> Local WSL failure with failed to create symbolic link '/mnt/c/Sources/multi_config/buildtrees/quickfix/src/v1.15.1-535f98545c/include/quickfix' because existing path cannot be removed: Is a directory on CI :
/home/vcpkg/myagent/_work/4/s/buildtrees/quickfix/src/v1.15.1-535f98545c/src/C++/double-conversion/ieee.h:397:34: error: ISO C++ forbids declaration of ‘DISALLOW_COPY_AND_ASSIGN’ with no type [-fpermissive]
   DISALLOW_COPY_AND_ASSIGN(Single);
  • libmysql -> Solved by CMake update (new FindZLIB.cmake)

OSX extras (no clue since/build logs are gone currently and I don't have direct acces to an osx machine):

  • caffe2
  • sophus
  • exiv2

@Rastaban
Copy link
Contributor

Rastaban commented Aug 29, 2019

I have added tensorflow to the CI skip list on linux because it is flaky. @dan-shaw is working independently on it to address the issues. In the future it should not show up on your failed list (after the change propagates).

# Conflicts:
#	ports/duktape/CONTROL
#	ports/glew/vcpkg-cmake-wrapper.cmake
#	ports/liblas/CONTROL
#	ports/pcl/CONTROL
#	ports/qt5-base/CONTROL
#	ports/wavpack/CONTROL
#	ports/xalan-c/CONTROL
#	scripts/buildsystems/vcpkg.cmake
# Conflicts:
#	ports/arrow/CONTROL
#	ports/curl/CONTROL
@dan-shaw
Copy link
Contributor

@Neumann-A Is this PR being worked on or changes you want to see merged? If so, it might make sense to open a new PR, otherwise we might lose track of the changes.

@Neumann-A
Copy link
Contributor Author

This is still necessary for multi configuration generators like VS project files. You can live without it if you use the single configuration VS CMake integration which I personally don't like since it reruns cmake configure every time you switch configurations.
I don't think this will ever be merged since its rather intrusive but for people like me who wants to use multi configuration generators this is an escape hatch. Ninja might also become a multi configuration generator in the future.
The only way to fix the linkage mess is to overwrite all of CMakes default FindXXX.cmake files which only considers a single library by one which searches for two separate libraries for debug/release for all Platforms.

@PhoebeHui
Copy link
Contributor

Closing this PR since it seems that no progress is being made. Please reopen if work is still being done.

@PhoebeHui PhoebeHui closed this Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants