Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThe build now passes OutputPath to the managed project via MSBuild (/p:OutputPath) from CMake, removing prior NEXO_MANAGED_OUTPUT_DIR logic and related relative-path computation. The .csproj consolidates properties and no longer hard-sets OutputPath. Clean-up targets were updated to reference artifacts under ${CMAKE_BINARY_DIR}. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
engine/src/scripting/managed/CMakeLists.txt(1 hunks)engine/src/scripting/managed/Nexo.csproj.in(1 hunks)
🔇 Additional comments (2)
engine/src/scripting/managed/Nexo.csproj.in (1)
11-13: LGTM! Clean consolidation of OutputPath handling.The changes correctly:
- Consolidate the append properties into the main PropertyGroup
- Remove the hardcoded OutputPath in favor of command-line parameter
- Document the new approach with a clear comment
This aligns with the PR objective of supporting multi-config builds by making OutputPath dynamic.
Note: If OutputPath is not provided via command line (e.g., manual
dotnet build), MSBuild will use its default output structure (bin/<config>/<framework>/). This is acceptable since the project is intended to be built via CMake.engine/src/scripting/managed/CMakeLists.txt (1)
75-82: Update clean paths for configuration-specific outputs
ADDITIONAL_CLEAN_FILES doesn’t expand generator expressions (e.g. $), so you’ll need to:
• Define a custom clean target that resolves config-specific paths at generate time
• Or explicitly list each configuration’s output directories (Debug, Release, etc.)
• Or use a supported glob pattern to match all configs
| # Build step with OutputPath passed as parameter to support multi-config builds | ||
| add_custom_target(nexoManaged ALL | ||
| COMMAND ${DOTNET_EXECUTABLE} build Nexo.csproj | ||
| ${ARCH_ARG} | ||
| -c $<CONFIG> # Matches Debug/Release configuration | ||
| /p:OutputPath=${CMAKE_BINARY_DIR}/ # Pass OutputPath dynamically based on build directory | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR} # Working directory for the build | ||
| COMMENT "Building .NET managed project (Nexo.csproj)..." | ||
| COMMENT "Building .NET managed project (Nexo.csproj) for $<CONFIG> configuration..." | ||
| ) |
There was a problem hiding this comment.
Critical: OutputPath doesn't separate configurations for multi-config generators.
The OutputPath is set to ${CMAKE_BINARY_DIR}/ without including the configuration subdirectory. For multi-config generators (Visual Studio, Ninja Multi-Config), Debug and Release builds will output to the same directory and overwrite each other's artifacts.
Apply this diff to properly separate configurations:
- /p:OutputPath=${CMAKE_BINARY_DIR}/ # Pass OutputPath dynamically based on build directory
+ /p:OutputPath=${CMAKE_BINARY_DIR}/$<CONFIG>/ # Pass OutputPath dynamically with config subdirectoryThis ensures each configuration (Debug/Release/etc.) outputs to its own subdirectory (e.g., build/Debug/, build/Release/), preventing artifacts from overwriting each other in multi-config builds.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Build step with OutputPath passed as parameter to support multi-config builds | |
| add_custom_target(nexoManaged ALL | |
| COMMAND ${DOTNET_EXECUTABLE} build Nexo.csproj | |
| ${ARCH_ARG} | |
| -c $<CONFIG> # Matches Debug/Release configuration | |
| /p:OutputPath=${CMAKE_BINARY_DIR}/ # Pass OutputPath dynamically based on build directory | |
| WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR} # Working directory for the build | |
| COMMENT "Building .NET managed project (Nexo.csproj)..." | |
| COMMENT "Building .NET managed project (Nexo.csproj) for $<CONFIG> configuration..." | |
| ) | |
| # Build step with OutputPath passed as parameter to support multi-config builds | |
| add_custom_target(nexoManaged ALL | |
| COMMAND ${DOTNET_EXECUTABLE} build Nexo.csproj | |
| ${ARCH_ARG} | |
| -c $<CONFIG> # Matches Debug/Release configuration | |
| /p:OutputPath=${CMAKE_BINARY_DIR}/$<CONFIG>/ # Pass OutputPath dynamically with config subdirectory | |
| WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR} # Working directory for the build | |
| COMMENT "Building .NET managed project (Nexo.csproj) for $<CONFIG> configuration..." | |
| ) |
🤖 Prompt for AI Agents
In engine/src/scripting/managed/CMakeLists.txt around lines 64 to 72, OutputPath
is set to ${CMAKE_BINARY_DIR}/ which causes different configurations to write to
the same folder; change the OutputPath to include the configuration subdirectory
so multi-config generators separate outputs (for example set
/p:OutputPath=${CMAKE_BINARY_DIR}/$<CONFIG>/ or equivalent) so
Debug/Release/etc. produce isolated build directories and avoid overwriting
artifacts.
iMeaNz
force-pushed
the
fix/scripting-profile
branch
from
e985807 to
b45a7c0
Compare
2 participants
Solution to #289
Pass OutputPath dynamically via MSBuild command-line parameter instead of hardcoding it in the .csproj file. This allows each build configuration to output to its correct directory.
Changes
Nexo.csproj.in:
CMakeLists.txt:
Summary by CodeRabbit