Build tent#2089
Conversation
Dao007forever
requested review from
alogfans,
chestnut-Q and
doujiang24
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the build system and CUDA transport logic by introducing a local CUDA build script, making unit test compilation optional, and improving the NVLink transport layer with CUDA device context management and VMM allocation support. Feedback from the review focuses on improving script portability by removing hardcoded user paths, addressing a security vulnerability in LD_LIBRARY_PATH construction, and ensuring robust error handling for CUDA driver API calls.
| CUresult retain_result = | ||
| cuMemRetainAllocationHandle(&generic_handle, (void*)desc.addr); | ||
| if (retain_result == CUDA_SUCCESS) { | ||
| cuMemRelease(generic_handle); |
There was a problem hiding this comment.
The return value of cuMemRelease is ignored. While this is a cleanup step, it is good practice to check the CUresult to ensure that the allocation handle is correctly released and to detect potential issues in the CUDA driver state.
| SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)" | ||
| REPO_ROOT="$(cd -- "${SCRIPT_DIR}/.." >/dev/null 2>&1 && pwd)" | ||
|
|
||
| DEPS_PREFIX="${MOONCAKE_DEPS:-/home/inf-daole/.local/mooncake-deps}" |
There was a problem hiding this comment.
The default value for DEPS_PREFIX contains a hardcoded path specific to a user's home directory (/home/inf-daole). This makes the script less portable and may cause build failures for other developers. Consider using a more generic default or leaving it empty to require explicit configuration.
| # ${BUILD_DIR}/ep_pg_staging when the project was built with -DWITH_EP=ON. | ||
| BUILD_DIR="${BUILD_DIR:-build}" | ||
| echo "Building wheel for Python ${PYTHON_VERSION} with output directory ${OUTPUT_DIR}" | ||
| DEPS_PREFIX="${MOONCAKE_DEPS:-/home/inf-daole/.local/mooncake-deps}" |
There was a problem hiding this comment.
The DEPS_PREFIX variable contains a hardcoded user-specific path (/home/inf-daole). Please replace this with a more generic default to ensure the script works across different environments.
|
|
||
| # Ensure LD_LIBRARY_PATH includes /usr/local/lib | ||
| export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/$(pwd)/build/mooncake-common:/usr/local/lib | ||
| export LD_LIBRARY_PATH=${LD_LIBRARY_PATH:-}:$(pwd)/${BUILD_DIR}/mooncake-common:${DEPS_PREFIX}/lib:/usr/local/lib |
There was a problem hiding this comment.
When LD_LIBRARY_PATH is empty or unset, the current construction results in a leading colon (e.g., :path/to/lib). In Linux, a leading or trailing colon in LD_LIBRARY_PATH is equivalent to including the current directory (.) in the search path. This can be a security risk if the script is executed in a directory containing untrusted shared objects.
| export LD_LIBRARY_PATH=${LD_LIBRARY_PATH:-}:$(pwd)/${BUILD_DIR}/mooncake-common:${DEPS_PREFIX}/lib:/usr/local/lib | |
| export LD_LIBRARY_PATH="${LD_LIBRARY_PATH}${LD_LIBRARY_PATH:+:}$(pwd)/${BUILD_DIR}/mooncake-common:${DEPS_PREFIX}/lib:/usr/local/lib" |
1 participant
Description
Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-pg)mooncake-rl)Type of Change
How Has This Been Tested?
Checklist
./scripts/code_format.shbefore submitting.