Skip to content

Native CMake integration#118

Open
gabrielecastellano wants to merge 3 commits intoaliyun:masterfrom
gabrielecastellano:cmake-integration
Open

Native CMake integration#118
gabrielecastellano wants to merge 3 commits intoaliyun:masterfrom
gabrielecastellano:cmake-integration

Conversation

@gabrielecastellano
Copy link
Copy Markdown
Contributor

@gabrielecastellano gabrielecastellano commented Apr 3, 2025

This creates a root CMakeLists that can be used to directly configure and build the project natively.

For instance:

$ mkdir cmake-cache && cd cmake-cache
$ cmake -DBUILD_SIM=ON -G "Unix Makefiles" ..
$ cmake --build . -j 103

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)

@tianhao909 tianhao909 requested a review from Copilot March 31, 2026 13:55
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.txt to drive builds of the simulator (and optionally analytical/phy targets).
  • Adjusts astra_ns3 subproject CMake to only default CMAKE_BUILD_TYPE when unset.
  • Updates .gitignore to ignore additional build/config artifacts (including generic **/build and **/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.

Comment on lines +22 to +29
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})
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines +41 to +42
file(REMOVE_RECURSE "${NS3_DIR}/simulation/build")
file(REMOVE_RECURSE "${NS3_DIR}/simulation/cmake-cache")
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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")

Copilot uses AI. Check for mistakes.
Comment on lines +56 to +63
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()
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +39 to +45
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()

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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()

Copilot uses AI. Check for mistakes.
Comment on lines +7 to +12
add_definitions("-Wall -g")

if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Default build type" FORCE)
endif()

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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()

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants