Conversation
There was a problem hiding this comment.
Pull request overview
Adds a top-level CMake entrypoint to configure/build SimAI “natively” (without the existing wrapper scripts), primarily targeting the ns-3 simulator workflow and integrating Astra-sim sources into the ns-3 tree during configuration.
Changes:
- Introduces a new root
CMakeLists.txtto drive builds of the simulator (and optionally analytical/phy targets). - Adjusts
astra_ns3subproject CMake to only defaultCMAKE_BUILD_TYPEwhen unset. - Updates
.gitignoreto ignore additional build/config artifacts (including generic**/buildand**/cmake-cache).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| CMakeLists.txt | New root CMake configuration orchestrating Astra-sim + ns-3 build and creating a simulator symlink |
| astra-sim-alibabacloud/build/astra_ns3/CMakeLists.txt | Safer defaulting of CMAKE_BUILD_TYPE (doesn’t clobber user-provided value) |
| .gitignore | Adds ignores for config/build artifacts and introduces broad **/build / **/cmake-cache patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set(NS3_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/ns-3-alibabacloud/simulation") | ||
| set(ASTRA_SIM_DIR "${CMAKE_CURRENT_SOURCE_DIR}/astra-sim-alibabacloud/astra-sim") | ||
| set(BUILD_DIR "${CMAKE_CURRENT_BINARY_DIR}") | ||
| set(RESULT_DIR "${CMAKE_CURRENT_SOURCE_DIR}}/result") | ||
|
|
||
| # Create required directories | ||
| file(MAKE_DIRECTORY ${BUILD_DIR}) | ||
| file(MAKE_DIRECTORY ${RESULT_DIR}) |
There was a problem hiding this comment.
RESULT_DIR has a stray } in the path ("${CMAKE_CURRENT_SOURCE_DIR}}/result"), which will produce an invalid directory name and can break directory creation. Please fix the path string (and consider aligning the directory name with the repo’s existing results/ vs result/ conventions so outputs land in the intended ignored location).
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/build") | ||
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/cmake-cache") |
There was a problem hiding this comment.
The CLEAN_BUILD block references NS3_DIR, but this variable is never defined in this CMake file (only NS3_SRC_DIR is). This makes the clean operation incorrect and likely leaves stale ns-3 build artifacts behind (or removes the wrong path if NS3_DIR is set externally). Use the correct variable/path for the ns-3 build and cmake-cache directories.
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/build") | |
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/cmake-cache") | |
| file(REMOVE_RECURSE "${NS3_SRC_DIR}/build") | |
| file(REMOVE_RECURSE "${NS3_SRC_DIR}/cmake-cache") |
| file(GLOB_RECURSE astra_sim_files CONFIGURE_DEPENDS "${ASTRA_SIM_DIR}/*") | ||
| list(FILTER astra_sim_files EXCLUDE REGEX "/network_frontend/") | ||
| foreach(file ${astra_sim_files}) | ||
| string(REPLACE "${ASTRA_SIM_DIR}/" "astra-sim/" relative_path ${file}) | ||
| get_filename_component(output_dir ${NS3_SRC_DIR}/src/applications/${relative_path} DIRECTORY) | ||
| file(MAKE_DIRECTORY ${output_dir}) | ||
| configure_file(${file} ${NS3_SRC_DIR}/src/applications/${relative_path} COPYONLY) | ||
| endforeach() |
There was a problem hiding this comment.
file(GLOB_RECURSE ... "${ASTRA_SIM_DIR}/*") will also return directories by default, and the loop then calls configure_file(... COPYONLY) on each entry. Passing a directory to configure_file fails at configure time. Configure the glob to exclude directories (e.g., LIST_DIRECTORIES false) or filter out directories before copying.
| message(STATUS "Cleaning build directory: ${BUILD_DIR}") | ||
| file(REMOVE_RECURSE ${BUILD_DIR}) | ||
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/build") | ||
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/cmake-cache") | ||
| file(MAKE_DIRECTORY ${BUILD_DIR}) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Removing ${BUILD_DIR} (which is ${CMAKE_CURRENT_BINARY_DIR}) during the configure step is unsafe because it deletes the active build tree (including CMake’s own files) while CMake is running. If you want a “clean” option, prefer generating a separate custom target/script that runs outside the configure step, or only delete a dedicated subdirectory owned by this project (not the entire binary dir).
| message(STATUS "Cleaning build directory: ${BUILD_DIR}") | |
| file(REMOVE_RECURSE ${BUILD_DIR}) | |
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/build") | |
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/cmake-cache") | |
| file(MAKE_DIRECTORY ${BUILD_DIR}) | |
| endif() | |
| message(STATUS "Cleaning NS3 simulation build directories") | |
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/build") | |
| file(REMOVE_RECURSE "${NS3_DIR}/simulation/cmake-cache") | |
| endif() | |
| add_definitions("-Wall -g") | ||
|
|
||
| if(NOT CMAKE_BUILD_TYPE) | ||
| set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Default build type" FORCE) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Using add_definitions("-Wall -g") injects flags globally (and -g will also apply to Release unless overridden). Prefer add_compile_options / target_compile_options and make debug info conditional on the build type so Release builds aren’t forced to include -g.
| add_definitions("-Wall -g") | |
| if(NOT CMAKE_BUILD_TYPE) | |
| set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Default build type" FORCE) | |
| endif() | |
| add_compile_options(-Wall) | |
| if(NOT CMAKE_BUILD_TYPE) | |
| set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Default build type" FORCE) | |
| endif() | |
| if (CMAKE_BUILD_TYPE STREQUAL "Debug") | |
| add_compile_options(-g) | |
| endif() |
This creates a root CMakeLists that can be used to directly configure and build the project natively.
For instance:
This can be helpful for developing (code inspection features) and for future maintenance in general.
Works with PR aliyun/ns-3-alibabacloud#9 for the ns3 subproject.
(tested with simulator only)