Generalize install locations #1791
Open
+14
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On Fedora the install location is dependent on the arch ex/ for x86_64 libraries install to /usr/lib64 So copy how llama-cpp does its install, replacing 'llama' with 'whisper' in the cmake variables. Signed-off-by: Tom Rix <[email protected]>
Contributor
|
The change seem to break Windows builds. Any hope to fix it? |
LecrisUT
reviewed
Jun 26, 2024
Comment on lines
+552
to
+557
| set(WHISPER_INCLUDE_INSTALL_DIR ${CMAKE_INSTALL_INCLUDEDIR} | ||
| CACHE PATH "Location of header files") | ||
| set(WHISPER_LIB_INSTALL_DIR ${CMAKE_INSTALL_LIBDIR} | ||
| CACHE PATH "Location of library files") | ||
| set(WHISPER_BIN_INSTALL_DIR ${CMAKE_INSTALL_BINDIR} | ||
| CACHE PATH "Location of binary files") |
There was a problem hiding this comment.
Any reason for exposing these as variables?
Comment on lines
+560
to
+564
| LIBRARY DESTINATION "${WHISPER_LIB_INSTALL_DIR}" | ||
| ARCHIVE DESTINATION "${WHISPER_LIB_INSTALL_DIR}/static" | ||
| RUNTIME DESTINATION "${WHISPER_BIN_INSTALL_DIR}" | ||
| RESOURCE DESTINATION "${WHISPER_BIN_INSTALL_DIR}" | ||
| PUBLIC_HEADER DESTINATION "${WHISPER_INCLUDE_INSTALL_DIR}" |
There was a problem hiding this comment.
Suggested change
| LIBRARY DESTINATION "${WHISPER_LIB_INSTALL_DIR}" | |
| ARCHIVE DESTINATION "${WHISPER_LIB_INSTALL_DIR}/static" | |
| RUNTIME DESTINATION "${WHISPER_BIN_INSTALL_DIR}" | |
| RESOURCE DESTINATION "${WHISPER_BIN_INSTALL_DIR}" | |
| PUBLIC_HEADER DESTINATION "${WHISPER_INCLUDE_INSTALL_DIR}" |
If the custom directories are removed, you can use the defaults, which are already compatible with windows (at least through vcpkg)
Contributor
|
This patch is now in conflict with the latest master branch. @ggerganov Is there any hope to get install rules included in the cmake setup? I guess Windows compatibility is required? |
Member
@petterreinholdtsen I thought they are already included. If it does not work on Windows, please propose a patch to fix it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Fedora the install location is dependent on the arch ex/ for x86_64 libraries install to /usr/lib64
So copy how llama-cpp does its install, replacing 'llama' with 'whisper' in the cmake variables.