[refactor]: cmake library installation#228
Merged
Conversation
…rity - Set CMAKE_POSITION_INDEPENDENT_CODE to ON for better compatibility. - Transitioned from global include directories to target-specific includes for better encapsulation. - Changed library definitions from INTERFACE to OBJECT for modular libraries to avoid transitive dependencies. - Updated target linking to use the unified PROJECT_NAME for consistency across executables. - Enhanced installation targets for unified libraries and improved include directory management. - Cleaned up unnecessary global includes and streamlined dependency management across various components.
Collaborator
|
Should we use an option to determine building the shared lib or not? |
Owner
Author
Sounds good! |
Owner
Author
|
updated |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CMake build to produce a unified libCacheSim static (and optional shared) library by compiling submodules as OBJECT libraries and updating the library installation.
- Converted each module (
cache,traceReader,profiler, etc.) into OBJECT libraries and removed individual installations. - Added a combined static
libCacheSimtarget (with optional shared variant) including propertarget_include_directories, linking, and installation rules. - Updated the
request_tstruct to nestkey_size/val_sizeinside akvsub-struct and adjusted relevant trace reader code.
Reviewed Changes
Copilot reviewed 15 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/CMakeLists.txt | Switched test executables to link against ${PROJECT_NAME} and added manual include paths. |
| libCacheSim/utils/CMakeLists.txt | Converted utils_lib to an OBJECT library. |
| libCacheSim/traceReader/*.h | Updated request_t field references from req->key_size to req->kv.key_size. |
| libCacheSim/traceReader/CMakeLists.txt | Converted traceReader_lib to an OBJECT library. |
| libCacheSim/traceAnalyzer/reuse.h | Removed vtime_granularity_ member and its constructor parameter. |
| libCacheSim/traceAnalyzer/CMakeLists.txt | Converted traceAnalyzer_lib to an OBJECT library and adjusted linking. |
| libCacheSim/profiler/CMakeLists.txt | Converted profiler_lib to an OBJECT library. |
| libCacheSim/mrcProfiler/CMakeLists.txt | Converted mrcProfiler_lib to an OBJECT library. |
| libCacheSim/include/libCacheSim/request.h | Wrapped bitfields key_size/val_size in a kv struct. |
| libCacheSim/dataStructure/CMakeLists.txt | Converted dataStructure_lib to an OBJECT library. |
| libCacheSim/cache/CMakeLists.txt | Converted cache_lib_c/cache_lib_cpp to OBJECT libraries. |
| CMakeLists.txt | Enabled CMAKE_POSITION_INDEPENDENT_CODE, added BUILD_SHARED_LIBS, centralized dependencies into ${PROJECT_NAME} static/shared library, and updated installation rules. |
Comments suppressed due to low confidence (3)
libCacheSim/include/libCacheSim/request.h:49
- The
request_tstruct now nestskey_sizeandval_sizeinsidekv, which breaks existing code references (e.g., inprint_request). Ensure all accesses (including inprint_requestand any other modules) are updated to usereq->kv.key_sizeand consider providing helper accessors if needed for clarity.
} kv;
test/CMakeLists.txt:28
- [nitpick] Since the
${PROJECT_NAME}target exports INTERFACE include directories, you can remove or reduce these manualtarget_include_directoriesentries and rely on the imported interface includes. This centralizes include paths and reduces duplication.
${CMAKE_SOURCE_DIR}/libCacheSim # Add source dir to find utils/include/mymath.h
CMakeLists.txt:283
- The
install()call for the staticlibCacheSimtarget includes aLIBRARY DESTINATION, which only applies to shared libraries. You can drop theLIBRARY DESTINATIONfor this static-only install and keep onlyARCHIVE DESTINATIONto avoid confusion.
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
haochengxia
approved these changes
Jun 25, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I am in the process of re-enabling the plugin system and noticed that the current library installation does not work. So this PR tries to fix the library installation.
Previously, we compiled each folder as a static library and installed all of them; however, we do not have a combined library. In this PR, each subfolder is compiled into an object, and we will install one library for libCacheSim. Currently we install both static and shared library.