SIM_LOG_DIR points to "${SCRIPT_DIR:?}"/log rather then /etc/astra-sim which requires sudo priviledges.#255
Conversation
…m which requires sudo priviledges.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #254 by changing SIM_LOG_DIR in Astra-Sim build scripts from a system-level path (/etc/astra-sim) to a repo-local path ("${SCRIPT_DIR:?}"/log), avoiding the need for sudo during directory creation.
Changes:
- Update
SIM_LOG_DIRin the top-levelastra-sim-alibabacloud/build.shto use a writable repo-locallog/directory. - Update
SIM_LOG_DIRinastra-sim-alibabacloud/build/astra_ns3/build.shto use a script-locallog/directory.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| astra-sim-alibabacloud/build/astra_ns3/build.sh | Changes SIM_LOG_DIR away from /etc/astra-sim to avoid sudo-required paths in the ns3 build script. |
| astra-sim-alibabacloud/build.sh | Changes SIM_LOG_DIR away from /etc/astra-sim so the build creates log/input/result directories in a user-writable location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NS3_DIR="${SCRIPT_DIR:?}"/../../extern/network_backend/ns3-interface | ||
| NS3_APPLICATION="${NS3_DIR:?}"/simulation/src/applications/ | ||
| SIM_LOG_DIR=/etc/astra-sim | ||
| SIM_LOG_DIR="${SCRIPT_DIR:?}"/log |
There was a problem hiding this comment.
SIM_LOG_DIR is now set relative to this sub-build script ("${SCRIPT_DIR}"/log), which differs from the top-level astra-sim-alibabacloud/build.sh log directory (repo-root/log). Even though SIM_LOG_DIR is currently unused here, this mismatch can cause confusion or inconsistent behavior if the commented-out copy-to-log logic is re-enabled; consider pointing both scripts at the same repo-level log directory (e.g., by computing it relative to the repo root) or passing it in from the parent script.
| SIMAI_PHY_BUILD_DIR="${SCRIPT_DIR:?}"/build/simai_phy | ||
| SIMAI_ANALYTICAL_BUILD_DIR="${SCRIPT_DIR:?}"/build/simai_analytical | ||
| SIM_LOG_DIR=/etc/astra-sim | ||
| SIM_LOG_DIR="${SCRIPT_DIR:?}"/log |
There was a problem hiding this comment.
This updates SIM_LOG_DIR to a writable repo-local path and the script creates directories under it, but parts of the simulator still appear to hardcode /etc/astra-sim for logs/results (e.g., MockNcclLog.h and SimAiMain.cc). If the goal is to avoid sudo requirements beyond just the build step, consider wiring SIM_LOG_DIR through to runtime (env/config/compile-time define) and aligning those hardcoded paths with this setting.
| SIM_LOG_DIR="${SCRIPT_DIR:?}"/log | |
| SIM_LOG_DIR="${SCRIPT_DIR:?}"/log | |
| export SIM_LOG_DIR |
This solves #254