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.

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

Fix RUNPATHs for libraries and executables #3383

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

StefanBruens
Copy link
Contributor

The RUNPATH should be limited to the entries actually required, instead of trying to cover all possible scenarios by adding an entry for each.

Instead of modifying the default RUNPATH by repeatedly setting CMAKE_INSTALL_RPATH, set the correct minimal INSTALL_RPATH property for each executable and library.

Only tested on Linux. Windows should not be affected, MacOS may be wrong.

@StefanBruens StefanBruens changed the title Fix runpaths Fix RUNPATHs for libraries and executables Jul 15, 2022
@StefanBruens
Copy link
Contributor Author

Several unrelated failures due to the recently added python_python_line_balancing_sat test, which is broken.

@Mizux Mizux self-requested a review July 18, 2022 10:01
@Mizux Mizux self-assigned this Jul 18, 2022
@Mizux Mizux added Feature Request Missing Feature/Wrapper Build: CMake CMake based build issue labels Jul 18, 2022
@Mizux Mizux added this to the v9.5 milestone Jul 18, 2022
@Mizux
Copy link
Collaborator

Mizux commented Jul 18, 2022

Overall OK for inclusion but want to perform few tests before and I prefer to postponed it to v9.5 (~August/September ?) unless you have strong argument(s) (and confidence) to include it in the incoming v9.4.

  1. Will test on MacOS but since you now move the code after the add_executable() I fear it won't be take into account OR like on unix we should try instead to use a set_target_property
    note: Will test it ASAP !

  2. IIRC, We are using CMAKE_BUILD_WITH_INSTALL_RPATH (since libortools can be packaged in a "CMake orchestrated python wheel package")

    or-tools/CMakeLists.txt

    Lines 89 to 92 in 49b6301

    # If wrapper are built, we need to have the install rpath in BINARY_DIR to package
    if(BUILD_PYTHON OR BUILD_JAVA OR BUILD_DOTNET)
    set(CMAKE_BUILD_WITH_INSTALL_RPATH TRUE)
    endif()

    And we are also using

    or-tools/CMakeLists.txt

    Lines 46 to 72 in 49b6301

    if(UNIX)
    option(BUILD_SHARED_LIBS "Build shared libraries (.so or .dyld)." ON)
    set(CMAKE_BUILD_WITH_INSTALL_RPATH TRUE)
    set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR})
    set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR})
    set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR})
    # for multi-config build system (e.g. xcode)
    foreach(OUTPUTCONFIG IN LISTS CMAKE_CONFIGURATION_TYPES)
    string(TOUPPER ${OUTPUTCONFIG} OUTPUTCONFIG)
    set(CMAKE_LIBRARY_OUTPUT_DIRECTORY_${OUTPUTCONFIG} ${CMAKE_BINARY_DIR}/${OUTPUTCONFIG}/${CMAKE_INSTALL_LIBDIR})
    set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY_${OUTPUTCONFIG} ${CMAKE_BINARY_DIR}/${OUTPUTCONFIG}/${CMAKE_INSTALL_LIBDIR})
    set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_${OUTPUTCONFIG} ${CMAKE_BINARY_DIR}/${OUTPUTCONFIG}/${CMAKE_INSTALL_BINDIR})
    endforeach()
    else()
    # Currently Only support static build for windows
    option(BUILD_SHARED_LIBS "Build shared libraries (.dll)." OFF)
    set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR})
    set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR})
    set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR})
    # for multi-config builds (e.g. msvc)
    foreach(OUTPUTCONFIG IN LISTS CMAKE_CONFIGURATION_TYPES)
    string(TOUPPER ${OUTPUTCONFIG} OUTPUTCONFIG)
    set(CMAKE_LIBRARY_OUTPUT_DIRECTORY_${OUTPUTCONFIG} ${CMAKE_BINARY_DIR}/${OUTPUTCONFIG}/${CMAKE_INSTALL_BINDIR})
    set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY_${OUTPUTCONFIG} ${CMAKE_BINARY_DIR}/${OUTPUTCONFIG}/${CMAKE_INSTALL_BINDIR})
    set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_${OUTPUTCONFIG} ${CMAKE_BINARY_DIR}/${OUTPUTCONFIG}/${CMAKE_INSTALL_BINDIR})
    endforeach()
    endif()

    to try to have the same relative layout and directory naming between the buildir and the installdir i.e. bin/ and lib[64]/
    -> need to carefully check it is working as expected aka CMAKE_INSTALL_LIBDIR is the same than CMAKE_INSTALL_FULL_LIBDIR (relative to INSTALL_BINDIR).

@Mizux Mizux modified the milestones: v9.5, v9.6 Sep 30, 2022
@Mizux Mizux modified the milestones: v9.6, v9.7 Feb 3, 2023
@StefanBruens StefanBruens force-pushed the fix_runpaths branch 2 times, most recently from 82ba36e to 03b8139 Compare May 10, 2023 23:50
@StefanBruens
Copy link
Contributor Author

StefanBruens commented May 10, 2023

Now fails to merge due to conflicting changes done in the meantime ... 😞

fa478b8#diff-a54fa3f11d0325be89059b2ab63a3bbeffefa802bf9a66580f14694c3e428d3d

@StefanBruens StefanBruens force-pushed the fix_runpaths branch 3 times, most recently from 1e0dd27 to 1ee1e2d Compare May 11, 2023 00:45
The old code added several irrelevant, duplicate or even non-existing
directories to the RUNPATHs of the samples.

Calculate the actual relative path between binary and library install
directory (using the absolute paths, CMAKE_INSTALL_LIBDIR may be either
relative or absolute), and set it as target property.
The flatzinc library is installed to the same location (LIBDIR) as the
ortools library, so "$ORIGIN" suffices.
The executables should use the relative path between LIBDIR and BINDIR.
@StefanBruens
Copy link
Contributor Author

@Mizux Ping!

@Mizux Mizux merged commit 4633e10 into google:main Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build: CMake CMake based build issue Feature Request Missing Feature/Wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants