Ide and toolchain changes #269
Conversation
Summary of ChangesHello @Mosch0512, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the project's development environment and build process. Key changes include standardizing the MSVC build to 32-bit, integrating the ImGui library for potential editor or debugging tools, and streamlining the deployment of runtime dependencies. Additionally, it modernizes debugging practices and addresses a specific issue with wide character string processing. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces several improvements for IDE integration and toolchain compatibility. The changes include updating the .gitignore for JetBrains IDEs, enhancing the CMakeLists.txt for better MSVC 32-bit build enforcement and dependency handling, and improving code portability by replacing MSVC-specific assembly with compiler intrinsics. Overall, these are positive changes. I have a couple of suggestions for the CMakeLists.txt to further improve maintainability.
There was a problem hiding this comment.
Code Review
This pull request introduces changes to the IDE and toolchain configuration, primarily focusing on setting up for a 32-bit build with MSVC and integrating the ImGui library. The changes to replace __asm { int 3 } with __debugbreak() are a good modernization step for better portability. The CMake changes are mostly good, especially the automation for copying DLLs. However, there's a potential issue in CMakeLists.txt regarding the placement of a check for the target architecture, which I've commented on.
There was a problem hiding this comment.
Code Review
This pull request is a great improvement for the project's build system and development experience. Migrating from a static Visual Studio solution to a cross-platform CMake build system is a significant step forward, making it easier for developers on different platforms and with different IDEs to contribute. The addition of IDE-specific configuration files for Visual Studio and the detailed build instructions in the README are very helpful. The automated build of the C# client library within CMake is a particularly nice touch. I have a few suggestions to fix a potential build issue and improve maintainability.
| add_custom_command( | ||
| OUTPUT "${DOTNET_DLL_PATH}" | ||
| COMMAND ${CMAKE_COMMAND} -E echo "--- C# Changes Detected: Rebuilding Client Library ---" | ||
| COMMAND dotnet publish "${DOTNET_PROJ}" -c $<CONFIG> -r win-x86 -o "${CMAKE_CURRENT_BINARY_DIR}" --nologo | ||
| DEPENDS "${DOTNET_PROJ}" ${DOTNET_SOURCES} | ||
| COMMENT "Checking for .NET Client Library updates..." | ||
| VERBATIM | ||
| ) |
There was a problem hiding this comment.
This custom command to build the C# project is a great addition. However, it will likely fail because the MUnique.Client.Library.csproj file specifies <TargetFramework>net10.0</TargetFramework>. .NET 10.0 has not been released yet. This will cause the dotnet publish command to fail for most developers. Please update the .csproj file to use a released .NET version, for example net8.0 (LTS) or net9.0. This is a critical issue that prevents the project from building.
| * **Visual Studio Build Tools 2022** (minimum requirement - older versions won't work, newer versions are not tested) | ||
| * Visual Studio 2022 with the newest update, workloads for C++ and C#, Jetbrains Rider also works | ||
| * **CMake** 3.16 or newer (bundled with Visual Studio and CLion) | ||
| * **.NET SDK 10.0** or newer (for building the Client Library) |
There was a problem hiding this comment.
The requirements list .NET SDK 10.0. As .NET 10 is not yet released, this is incorrect and will confuse users. This should be updated to match the TargetFramework used in the C# project. For example, if you change it to net8.0, this line should be updated to require .NET SDK 8.0.
| * **.NET SDK 10.0** or newer (for building the Client Library) | |
| * **.NET SDK 8.0** or newer (for building the Client Library) |
…oss-compiler debugging support.
…n = 1` for consistent wide character handling.
…e `_USE_32BIT_TIME_T`, and automate DLL copying post-build.
…eLists.txt` for editor support.
…tor-specific functionality.
… paths, and streamline DLL copying logic in `CMakeLists.txt`.
…Lists.txt`. which get included in `Include.h`
…watch for C# changes, rebuild on updates, and link DLL to `Main`.
…dio launch configuration file.
…d clarify IDE-specific steps.
…actices and improve readability.
…o detect and respond to new/removed `.cs` files dynamically.
Mosch0512
force-pushed
the
ide-and-toolchain-changes
branch
from
7b64df3 to
02eb0a6
Compare
…th cross-platform build and run instructions
There was a problem hiding this comment.
Code Review
This pull request significantly improves the project's development environment by migrating to CMake, automating the .NET client library integration, and updating build instructions. The changes enhance developer experience and maintainability. Additionally, a critical bug related to wide character handling in SeparateTextIntoLines has been fixed, and inline assembly debug breakpoints have been replaced with the more modern __debugbreak();.
| if (MSVC) | ||
| if (CMAKE_SIZEOF_VOID_P EQUAL 8) | ||
| message(FATAL_ERROR "This project requires 32-bit libraries. Please configure CMake with: -DCMAKE_GENERATOR_PLATFORM=Win32 (for Visual Studio generator) or use the x86 MSVC toolchain in CLion settings") | ||
| endif() |
There was a problem hiding this comment.
This explicit check for 64-bit MSVC builds and the FATAL_ERROR message are crucial for ensuring compatibility with existing 32-bit libraries, as mentioned in the PR description. This prevents potential build failures or runtime issues in unsupported environments.
Source Main 5.2/CMakeLists.txt
Outdated
| # 1. Define the output path using a variable CMake can understand early | ||
| set(DOTNET_DLL_PATH "${CMAKE_CURRENT_BINARY_DIR}/MUnique.Client.Library.dll") | ||
| set(DOTNET_PROJ "${CMAKE_CURRENT_SOURCE_DIR}/../ClientLibrary/MUnique.Client.Library.csproj") | ||
|
|
||
| # 2. Tell CMake to watch ALL .cs files in the ClientLibrary folder | ||
| file(GLOB_RECURSE DOTNET_SOURCES CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/../ClientLibrary/*.cs") | ||
|
|
||
| # 3. This only runs if DOTNET_SOURCES or the .csproj change | ||
| add_custom_command( | ||
| OUTPUT "${DOTNET_DLL_PATH}" | ||
| COMMAND ${CMAKE_COMMAND} -E echo "--- C# Changes Detected: Rebuilding Client Library ---" | ||
| COMMAND dotnet publish "${DOTNET_PROJ}" -c $<CONFIG> -r win-x86 -o "${CMAKE_CURRENT_BINARY_DIR}" --nologo | ||
| DEPENDS "${DOTNET_PROJ}" ${DOTNET_SOURCES} | ||
| COMMENT "Checking for .NET Client Library updates..." | ||
| VERBATIM | ||
| ) | ||
|
|
||
| # 4. Create a target for the DLL and link it to Main | ||
| add_custom_target(BuildDotNetLib DEPENDS "${DOTNET_DLL_PATH}") | ||
| add_dependencies(Main BuildDotNetLib) |
There was a problem hiding this comment.
The automated .NET Client Library integration is a significant improvement. Using file(GLOB_RECURSE ... CONFIGURE_DEPENDS ...) correctly tracks changes in C# source files, and add_custom_command with OUTPUT and DEPENDS ensures the .NET library is rebuilt only when necessary. This streamlines the build process and reduces manual steps, as highlighted in the PR description.
| // For wide characters, always 1 character per unit | ||
| iMbclen = 1; |
There was a problem hiding this comment.
This change correctly addresses a bug in SeparateTextIntoLines where _tclen was used for wchar_t strings. For wide characters, each character typically occupies one unit, so setting iMbclen = 1 ensures correct length calculation for multi-byte character sets, aligning with the PR description's bug fix.
Source Main 5.2/CMakeLists.txt
Outdated
| @@ -58,16 +85,11 @@ target_compile_definitions(Main PRIVATE | |||
| _LANGUAGE_FOREIGN | |||
| _LANGUAGE_ENG | |||
| UNICODE | |||
| $<$<CONFIG:Debug>:_DEBUG;_FOREIGN_DEBUG> | |||
| _USE_32BIT_TIME_T # Use 32-bit time_t for 32-bit builds | |||
There was a problem hiding this comment.
Adding _USE_32BIT_TIME_T is a good practice for ensuring time_t remains 32-bit, which is often necessary when dealing with older C++ codebases that might implicitly rely on its size for ABI compatibility or data structure layouts, especially in a 32-bit build environment.
Source Main 5.2/CMakeLists.txt
Outdated
| add_custom_target(run | ||
| COMMAND "$<TARGET_FILE:Main>" | ||
| WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/bin" | ||
| DEPENDS Main | ||
| COMMENT "Running Main from bin directory" | ||
| USES_TERMINAL | ||
| ) |
There was a problem hiding this comment.
The add_custom_target(run ...) is a very useful addition for developer convenience, providing a consistent way to execute the application with the correct working directory across different IDEs and command-line environments.
…c discovery and improve maintainability.
…g git submodules.
…ng, with locking to prevent parallel process issues.
…dling and environment variable overrides to fix issues with special characters in paths.
…s on build because normally they are in the user home which could contain umlauts
…, exclude `imgui` from all builds, and add `ConstantsReplacer` tool. Update `.vs/launch.vs.json` with a new configuration for `ConstantsReplacer`.
…uild output and dependencies for cleaner integration
…build settings for 32-bit and 64-bit platforms, and update .NET publishing process accordingly. Add x64 profiles to `.idea/cmake.xml` and `CMakeSettings.json`.
… architecture-specific settings, and configure `.idea/cmake.xml` to use the new toolchain.
…ed `x86` settings, improve automatic architecture inference in `CMakeLists.txt`, and update `toolchain-x64.cmake` for consistency. Update default server port.
…$(USERPROFILE) and adjust default folder path in ConstantsReplacer Form.
… builds with MSVC; improve asset and DLL copying in build setup.
…form support for x64 and x86 builds.
…lds, making it consistent with all other build methods
…files, ensuring consistency with x64 configuration
Mosch0512
force-pushed
the
ide-and-toolchain-changes
branch
from
71989ab to
bb6ac81
Compare
…e reliability, handle incomplete directories, and add missing submodules to the git index automatically.
|
Thank you!!! 👍 |
2 participants
closes #264
makes #271 irrelevant
This pull request significantly overhauls the project's development environment and build toolchain. By migrating to CMake, it streamlines the build process, automates the integration of the .NET client library, and provides clearer, more consistent build instructions across different IDEs. These changes aim to improve developer experience, reduce manual setup, and ensure better compatibility and maintainability of the build system.
Highlights
__asm { int 3 };) have been replaced with the more modern and compiler-agnostic__debugbreak();in several source files, enhancing debugging consistency.SeparateTextIntoLinesfunction related to wide character length calculation has been fixed, ensuring correct text separation for multi-byte character sets.