Skip to content

Comments

Update for https://fedoraproject.org/wiki/Changes/CMake_drop_install_vars#973

Merged
BYVoid merged 1 commit intoBYVoid:masterfrom
epico:cmake
Jan 14, 2026
Merged

Update for https://fedoraproject.org/wiki/Changes/CMake_drop_install_vars#973
BYVoid merged 1 commit intoBYVoid:masterfrom
epico:cmake

Conversation

@epico
Copy link
Contributor

@epico epico commented Aug 12, 2025

No description provided.

@BYVoid
Copy link
Owner

BYVoid commented Jan 13, 2026

Sorry, can you rebase and send the PR again?

@epico
Copy link
Contributor Author

epico commented Jan 14, 2026

No problem. I just rebased and updated the PR.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the CMake configuration to comply with Fedora's "CMake_drop_install_vars" policy, which standardizes library installation directories using CMake's GNUInstallDirs module instead of custom variables.

Changes:

  • Added include(GNUInstallDirs) to import standard CMake directory variables
  • Introduced LIB_INSTALL_DIR based on CMAKE_INSTALL_LIBDIR
  • Added logic to detect lib64 and set LIB_SUFFIX accordingly for backwards compatibility
Comments suppressed due to low confidence (2)

CMakeLists.txt:91

  • The LIB_SUFFIX variable is not initialized when CMAKE_INSTALL_LIBDIR doesn't end with 'lib64'. This will cause an error or unexpected behavior. Initialize LIB_SUFFIX to an empty string before the conditional check, e.g., set(LIB_SUFFIX \"\") before line 83.
set (DIR_LIBRARY ${DIR_PREFIX}/lib${LIB_SUFFIX})

CMakeLists.txt:107

  • The conditional check at line 105-107 will always evaluate to true since LIB_INSTALL_DIR is unconditionally set at line 82, causing DIR_LIBRARY to always be overridden to CMAKE_INSTALL_LIBDIR and ignoring the lib${LIB_SUFFIX} construction at line 91. Consider using an absolute path from GNUInstallDirs (e.g., ${CMAKE_INSTALL_FULL_LIBDIR}) instead of the relative CMAKE_INSTALL_LIBDIR, or restructure the logic to avoid this conflict.
if (DEFINED LIB_INSTALL_DIR)
  set (DIR_LIBRARY ${LIB_INSTALL_DIR})
endif (DEFINED LIB_INSTALL_DIR)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@BYVoid BYVoid merged commit 9c003c9 into BYVoid:master Jan 14, 2026
30 checks passed
frankslin pushed a commit to frankslin/OpenCC that referenced this pull request Jan 15, 2026
@epico
Copy link
Contributor Author

epico commented Jan 15, 2026

Thanks!

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.

2 participants