Feature: Use Microsoft C++ (MSVC) compiler for building and add VS support #87
Conversation
WalkthroughThe pull request introduces several modifications across various files to transition the project to a Windows-centric build environment, primarily using Microsoft tools. The Possibly related PRs
Warning Rate limit exceeded@infeo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (10)
README.md (2)
13-19: Consider enhancing the requirements sectionThe requirements are generally clear, but could benefit from some improvements:
- Specify the minimum MSVC version required
- Clarify why
makeis needed on WindowsConsider this enhancement:
### Requirements * JDK 22 * Maven -* MSVC 2022 toolset (e.g. by installing Visual Studio 2022, Workset "Desktop development with C++") -* make +* MSVC 2022 toolset (v143) or newer + * Install through Visual Studio 2022 with "Desktop development with C++" workload +* GNU Make (required for build automation) + * Can be installed via chocolatey, scoop, or msys2
20-26: Enhance build instructions for Windows usersThe build instructions could be more Windows-specific and clearer:
- Missing language specification in code block
- The Visual Studio path could vary based on edition
Consider these enhancements:
### Build Open a terminal and run -``` +```bash mvn clean verify-If building the dlls fails with "
cl.execannot be found", you have to specify the developer command file directory as a property-DdevCommandFileDir=C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\".
+If building the DLLs fails with "cl.execannot be found", specify the Visual Studio developer command file directory:
+
+bash +# For VS2022 Enterprise edition +mvn clean verify -DdevCommandFileDir="C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build" + +# For VS2022 Community edition +mvn clean verify -DdevCommandFileDir="C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\Build" +<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [grammar] ~20-~20: Please check the verb form. Context: ...evelopment with C++") * make ### Build Open a terminal and run ``` mvn clean verify... (SHE_LIVE) </details> <details> <summary>🪛 Markdownlint (0.35.0)</summary> 22-22: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>src/main/native/org_cryptomator_windows_uiappearnce_WinAppearance_Native.cpp (2)</summary><blockquote> `23-23`: **Improve error message buffer handling** While using `sprintf_s` is safer, the current implementation has room for improvement: 1. The buffer size is hardcoded 2. The size might be insufficient for complex error messages 3. The magic number "50" is repeated Consider this improvement: ```diff + #define ERROR_MSG_BUFFER_SIZE 128 + // In each error handling block: - char msg[50]; - sprintf_s(msg, 50, "Failed to read registry value (status %d)", (int) status); + char msg[ERROR_MSG_BUFFER_SIZE]; + sprintf_s(msg, ERROR_MSG_BUFFER_SIZE, "Failed to read registry value (status %d)", (int) status);Also applies to: 34-34, 40-40
Line range hint
1-44: Consider centralizing Windows registry pathsSince this codebase is becoming more Windows-centric, consider creating a central configuration file or constants for Windows-specific paths and settings. This would make maintenance easier and prevent issues like the registry path syntax errors.
Example approach:
// windows_config.h #define THEME_REGISTRY_KEY L"Software\\Microsoft\\Windows\\CurrentVersion\\Themes\\Personalize" #define THEME_REGISTRY_VALUE L"AppsUseLightTheme"src/main/native/org_cryptomator_windows_keychain_WinDataProtection_Native.cpp (4)
4-4: Consider removing redundant header inclusionThe
dpapi.hheader is already included implicitly throughwincrypt.h. While not harmful, removing redundant includes helps maintain cleaner code.-#include <dpapi.h>
Line range hint
7-24: Add error handling and prevent memory leaksSeveral critical issues need to be addressed:
- Memory leak: CryptProtectData allocates memory for ciphertextBlob that needs to be freed using LocalFree
- No error information is captured when encryption fails
- Missing input validation
Here's the suggested fix:
JNIEXPORT jbyteArray JNICALL Java_org_cryptomator_windows_keychain_WinDataProtection_00024Native_protect(JNIEnv *env, jobject thisObj, jbyteArray cleartext, jbyteArray salt) { + if (!cleartext || !salt) { + return NULL; + } + DATA_BLOB cleartextBlob; cleartextBlob.pbData = (BYTE *) env->GetByteArrayElements(cleartext, JNI_FALSE); cleartextBlob.cbData = (DWORD) env->GetArrayLength(cleartext); DATA_BLOB saltBlob; saltBlob.pbData = (BYTE *) env->GetByteArrayElements(salt, JNI_FALSE); saltBlob.cbData = (DWORD) env->GetArrayLength(salt); DATA_BLOB ciphertextBlob; BOOL success = CryptProtectData(&cleartextBlob, NULL, &saltBlob, NULL, NULL, 0, &ciphertextBlob); env->ReleaseByteArrayElements(cleartext, (jbyte*) cleartextBlob.pbData, JNI_ABORT); env->ReleaseByteArrayElements(salt, (jbyte*) saltBlob.pbData, JNI_ABORT); + jbyteArray result = NULL; if (success) { - jbyteArray ciphertext = env->NewByteArray(ciphertextBlob.cbData); - env->SetByteArrayRegion(ciphertext, 0, ciphertextBlob.cbData, (jbyte *) ciphertextBlob.pbData); - return ciphertext; + result = env->NewByteArray(ciphertextBlob.cbData); + if (result) { + env->SetByteArrayRegion(result, 0, ciphertextBlob.cbData, (jbyte *) ciphertextBlob.pbData); + } + LocalFree(ciphertextBlob.pbData); } else { - return NULL; + // Log error for debugging + DWORD error = GetLastError(); + // Consider throwing a Java exception here with the error code } + return result; }
Line range hint
26-45: Add error handling and prevent memory leaks in unprotect functionThe unprotect function has the same critical issues as the protect function:
- Memory leak: CryptUnprotectData allocates memory that needs to be freed
- No error information is captured
- Missing input validation
Apply similar fixes as protect function:
JNIEXPORT jbyteArray JNICALL Java_org_cryptomator_windows_keychain_WinDataProtection_00024Native_unprotect(JNIEnv *env, jobject thisObj, jbyteArray ciphertext, jbyteArray salt) { + if (!ciphertext || !salt) { + return NULL; + } + DATA_BLOB ciphertextBlob; ciphertextBlob.pbData = (BYTE *) env->GetByteArrayElements(ciphertext, JNI_FALSE); ciphertextBlob.cbData = (DWORD) env->GetArrayLength(ciphertext); DATA_BLOB saltBlob; saltBlob.pbData = (BYTE *) env->GetByteArrayElements(salt, JNI_FALSE); saltBlob.cbData = (DWORD) env->GetArrayLength(salt); DATA_BLOB cleartextBlob; BOOL success = CryptUnprotectData(&ciphertextBlob, NULL, &saltBlob, NULL, NULL, 0, &cleartextBlob); env->ReleaseByteArrayElements(ciphertext, (jbyte*) ciphertextBlob.pbData, JNI_ABORT); env->ReleaseByteArrayElements(salt, (jbyte*) saltBlob.pbData, JNI_ABORT); + jbyteArray result = NULL; if (success) { - jbyteArray cleartext = env->NewByteArray(cleartextBlob.cbData); - env->SetByteArrayRegion(cleartext, 0, cleartextBlob.cbData, (jbyte *) cleartextBlob.pbData); - return cleartext; + result = env->NewByteArray(cleartextBlob.cbData); + if (result) { + env->SetByteArrayRegion(result, 0, cleartextBlob.cbData, (jbyte *) cleartextBlob.pbData); + } + LocalFree(cleartextBlob.pbData); } else { - return NULL; + // Log error for debugging + DWORD error = GetLastError(); + // Consider throwing a Java exception here with the error code } + return result; }
Line range hint
7-45: Improve error handling architectureThe current error handling approach of returning NULL doesn't provide enough information to the Java side about what went wrong. Consider implementing a more robust error handling strategy:
- Create specific Java exceptions for different error cases (e.g., InvalidInputException, EncryptionFailedException)
- Use JNI's exception throwing capabilities to provide detailed error information
Here's a suggested approach for implementing exception handling:
// Helper function to throw Java exceptions void ThrowException(JNIEnv* env, const char* exceptionClass, DWORD errorCode) { char errorMsg[256]; FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM, NULL, errorCode, 0, errorMsg, sizeof(errorMsg), NULL); jclass cls = env->FindClass(exceptionClass); if (cls != NULL) { env->ThrowNew(cls, errorMsg); } }Usage in the protect/unprotect functions:
if (!success) { DWORD error = GetLastError(); switch (error) { case ERROR_INVALID_PARAMETER: ThrowException(env, "java/lang/IllegalArgumentException", error); break; default: ThrowException(env, "org/cryptomator/windows/keychain/CryptoException", error); } }integrations-win.vcxproj (1)
26-26: Consider using a more flexible SDK version specificationThe hard-coded Windows SDK version (10.0.22621.0) might cause build issues if developers have different SDK versions installed.
Consider using
10.0instead to automatically use the latest installed SDK version:- <WindowsTargetPlatformVersion>10.0.22621.0</WindowsTargetPlatformVersion> + <WindowsTargetPlatformVersion>10.0</WindowsTargetPlatformVersion>pom.xml (1)
Line range hint
1-577: Consider documenting Windows build requirementsGiven the transition to MSVC, it's important to document the build requirements and setup process for developers. This should include:
- Required Visual Studio components (C++ build tools, Windows SDK)
- Environment setup instructions
- Common troubleshooting steps
Additionally, consider:
- Adding CI checks to verify MSVC toolchain availability
- Creating a build verification script
- Documenting any platform-specific optimizations enabled by MSVC
Would you like me to help create:
- A detailed README section about build requirements?
- CI checks for the MSVC toolchain?
- A build verification script?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
.github/workflows/build.yml(1 hunks).gitignore(1 hunks)Makefile(1 hunks)README.md(1 hunks)integrations-win.sln(1 hunks)integrations-win.vcxproj(1 hunks)pom.xml(2 hunks)src/main/native/org_cryptomator_windows_keychain_WinDataProtection_Native.cpp(1 hunks)src/main/native/org_cryptomator_windows_uiappearnce_WinAppearance_Native.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- integrations-win.sln
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~20-~20: Please check the verb form.
Context: ...evelopment with C++") * make ### Build Open a terminal and run ``` mvn clean verify...
(SHE_LIVE)
🪛 Markdownlint (0.35.0)
README.md
22-22: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
README.md (1)
Line range hint 1-26: Verify references to build configurations
Let's ensure there are no outdated references to MinGW or g++ in other documentation files.
✅ Verification successful
Build configuration references are consistent with MSVC toolchain
The documentation is clean and properly aligned with the MSVC toolchain requirements. There are no outdated references to MinGW or g++, and the only build-related content is in README.md which correctly specifies MSVC 2022 as the required toolset. The build instructions are clear and up-to-date with the current toolchain choice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to MinGW or g++ in documentation files
rg -i "mingw|g\+\+" -g "*.{md,txt,rst}"
Length of output: 41
Script:
#!/bin/bash
# Let's do a more comprehensive search for build-related configurations and documentation
# 1. Find all documentation files first
fd -e md -e txt -e rst
# 2. Look for any build-related content in these files
rg -i "build|compile|toolchain|gcc|visual studio|msvc" -g "*.{md,txt,rst}"
Length of output: 505
🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: Please check the verb form.
Context: ...evelopment with C++") * make ### Build Open a terminal and run ``` mvn clean verify...
(SHE_LIVE)
🪛 Markdownlint (0.35.0)
22-22: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
pom.xml (1)
314-321: 🛠️ Refactor suggestion
Verify the build command execution
The build configuration has been updated to use Windows command prompt and Visual Studio build tools. However, there are a few concerns:
- The command concatenation using
&&might fail in certain Windows environments - The success/failure of
vcvars64.batmight not be properly propagated
Let's verify the build tools setup:
Consider using a dedicated batch script for more robust execution:
- <argument>"${devCommandFileDir}\vcvars64.bat" && make install</argument>
+ <argument>${project.basedir}/build.bat</argument>Create build.bat:
@echo off
call "%1\vcvars64.bat"
if errorlevel 1 exit /b %errorlevel%
make install✅ Verification successful
Let me gather more information about the build configuration and existing build scripts.
Let me check the contents of the Makefile and README for any build-related information.
Command concatenation with && is safe in this Windows build setup
The current build configuration is appropriate for the following reasons:
- The README explicitly documents the requirement for Visual Studio 2022 and the
vcvars64.batpath setup - The Makefile shows proper usage of MSVC compiler (
cl.exe) with correct Windows-specific flags and paths - The command concatenation using
&&is a standard Windows command prompt operator that correctly propagates errors - The
vcvars64.batsetup is required before runningmake installas the Makefile depends on MSVC environment variables
The build will fail appropriately if:
vcvars64.batfails to execute (non-zero exit code preventsmake installfrom running)make installfails during DLL compilation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Visual Studio build tools path exists in common locations
fd -t d "VC" "C:/Program Files/Microsoft Visual Studio" "C:/Program Files (x86)/Microsoft Visual Studio"
# Check if make is available in the repository
fd -t f "^make(\.exe)?$"
# Look for any existing build or compile scripts
rg -g "!target/" -g "!*.class" -g "!*.jar" "vcvars|cl\.exe|compile|build"
Length of output: 2451
Script:
#!/bin/bash
# Check for any existing batch files that might handle build setup
fd -e bat -e cmd
# Look for Makefile to understand the build requirements
fd Makefile
# Check for any error handling or build setup in pom.xml
rg -A 5 -B 5 "vcvars64\.bat" pom.xml
Length of output: 439
Script:
#!/bin/bash
# Check Makefile contents for build requirements and error handling
cat Makefile
# Check README for build instructions and requirements
rg -A 5 "build|compile|install" README.md
Length of output: 1291
src/main/native/org_cryptomator_windows_uiappearnce_WinAppearance_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_uiappearnce_WinAppearance_Native.cpp
Outdated
Show resolved
Hide resolved
Makefile
Outdated
| HEADERS := -I"src\main\headers" \ | ||
| -I"${JAVA_HOME}\include" \ | ||
| -I"${JAVA_HOME}\include\win32" \ | ||
| -I"C:\Program Files (x86)\Windows Kits\10\Include\$(WIN_SDK_VERSION)\cppwinrt" \ | ||
| -I"C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\$(MSVC_VERSION)\include" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded include paths with environment variables
Using hardcoded paths for the Windows SDK and MSVC include directories reduces portability and can cause build failures on systems where these tools are installed in different locations.
Modify the Makefile to use environment variables provided by Visual Studio's environment setup script (vcvarsall.bat). Here's how you might adjust the HEADERS variable:
-HEADERS := -I"src\main\headers" \
- -I"${JAVA_HOME}\include" \
- -I"${JAVA_HOME}\include\win32" \
- -I"C:\Program Files (x86)\Windows Kits\10\Include\$(WIN_SDK_VERSION)\cppwinrt" \
- -I"C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\$(MSVC_VERSION)\include"
+HEADERS := -I"src\main\headers" \
+ -I"$(JAVA_HOME)\include" \
+ -I"$(JAVA_HOME)\include\win32" \
+ -I"$(WindowsSdkDir)\Include\$(UCRTVersion)\um" \
+ -I"$(VCToolsInstallDir)\include"This change leverages environment variables set by Visual Studio's build tools:
WindowsSdkDirandUCRTVersionpoint to the Windows SDK directories.VCToolsInstallDirpoints to the MSVC installation directory.
Before running make, ensure that the environment is properly set up by executing the vcvarsall.bat script corresponding to your target architecture (e.g., vcvarsall.bat x64).
📝 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.
| HEADERS := -I"src\main\headers" \ | |
| -I"${JAVA_HOME}\include" \ | |
| -I"${JAVA_HOME}\include\win32" \ | |
| -I"C:\Program Files (x86)\Windows Kits\10\Include\$(WIN_SDK_VERSION)\cppwinrt" \ | |
| -I"C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\$(MSVC_VERSION)\include" | |
| HEADERS := -I"src\main\headers" \ | |
| -I"$(JAVA_HOME)\include" \ | |
| -I"$(JAVA_HOME)\include\win32" \ | |
| -I"$(WindowsSdkDir)\Include\$(UCRTVersion)\um" \ | |
| -I"$(VCToolsInstallDir)\include" |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/codeql-analysis.yml (1)
35-35: Consider using environment variables for Visual Studio pathsThe hardcoded Visual Studio path makes the workflow brittle to changes in VS installation paths or editions. Consider using environment variables or GitHub Actions environment files for better maintainability.
- run: mvn -B compile -DdevCommandFileDir="C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Auxiliary\\Build\\" + env: + VS2022_PATH: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise" + run: mvn -B compile -DdevCommandFileDir="${VS2022_PATH}\\VC\\Auxiliary\\Build\\"This approach:
- Makes it easier to update paths across all workflow files
- Improves readability and maintenance
- Allows for easier testing with different VS editions
.github/workflows/publish-github.yml (1)
24-24: Consider making the Visual Studio path configurable.While the change correctly specifies the Visual Studio build tools directory, consider making this path configurable through GitHub Actions variables or environment variables. This would improve maintainability and flexibility when Visual Studio versions or editions change.
Example improvement:
- run: mvn deploy -B -DskipTests -Psign,deploy-github --no-transfer-progress -DdevCommandFileDir="C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Auxiliary\\Build\\" + run: mvn deploy -B -DskipTests -Psign,deploy-github --no-transfer-progress -DdevCommandFileDir="${{ vars.VS_BUILD_TOOLS_PATH || 'C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Auxiliary\\Build\\' }}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/publish-central.yml(1 hunks).github/workflows/publish-github.yml(1 hunks)integrations-win.vcxproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations-win.vcxproj
🔇 Additional comments (1)
.github/workflows/publish-github.yml (1)
24-24: Verify Visual Studio build tools path in the GitHub-hosted runner.
Let's verify that the specified Visual Studio path exists in the GitHub-hosted Windows runner.
✅ Verification successful
Visual Studio path in workflow is correct and matches GitHub runner environment
The specified path C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\ is correct as it aligns with the Visual Studio Enterprise 2022 installation path (C:\Program Files\Microsoft Visual Studio\2022\Enterprise) documented in the GitHub-hosted Windows 2022 runner specifications. The VC\Auxiliary\Build subdirectory is a standard location for Visual Studio build tools.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Visual Studio build tools directory exists in recent workflow runs
# Get recent workflow runs and check their logs for the directory
gh run list --workflow=publish-github.yml --json databaseId --jq '.[0].databaseId' | \
xargs -I {} gh run view {} --log | \
grep -A 5 "C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Auxiliary\\Build\\"
# Additionally, check recent runner images changelog for any Visual Studio path changes
curl -s https://raw.githubusercontent.com/actions/runner-images/main/images/win/Windows2022-Changelog.md | \
grep -A 5 "Visual Studio Enterprise 2022"
Length of output: 557
Script:
#!/bin/bash
# Let's try a different approach to check Visual Studio installation on Windows runners
# First, let's check the workflow runs without complex path patterns
gh run list --workflow=publish-github.yml --json databaseId --jq '.[0].databaseId' | \
xargs -I {} gh run view {} --log | \
grep -i "visual studio"
# Also check if there are any environment variables or alternative paths being used
rg -i "visual studio" .github/workflows/
Length of output: 1145
Script:
#!/bin/bash
# Let's check the GitHub Actions documentation for Windows runner specifications
curl -s https://raw.githubusercontent.com/actions/runner-images/main/images/windows/Windows2022-Readme.md | \
grep -A 10 "Visual Studio Enterprise 2022"
# Also check if there are any related issues or discussions about this path
gh issue list --repo actions/runner-images --search "visual studio 2022 enterprise path" --json title,body --jq '.[]'
Length of output: 1450
infeo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
4-8: Ensure consistent path separator usageThe
HEADERSvariable mixes backslashes and forward slashes. While both work on Windows, it's better to stick to one style for consistency.HEADERS := -I"src\main\headers" \ - -I"${JAVA_HOME}\include" \ - -I"${JAVA_HOME}\include\win32" \ + -I"${JAVA_HOME}/include" \ + -I"${JAVA_HOME}/include/win32" \ -I"C:\Program Files (x86)\Windows Kits\10\Include\$(WIN_SDK_VERSION)\cppwinrt" \ -I"C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\$(MSVC_VERSION)\include"
18-18: Ensure target directory existsThe
-Fo"target/"flag specifies output directory but doesn't ensure it exists. Consider adding directory creation.install: + if not exist "target" mkdir "target" cl -EHsc -std:c++17 -LD -W4 -guard:cf \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(1 hunks)
🔇 Additional comments (1)
Makefile (1)
16-22: Verify Control Flow Guard (CFG) configuration
The -guard:cf flag enables Control Flow Guard, but it requires proper configuration in both compiler and linker flags to be fully effective.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
README.md (1)
13-19: Requirements section needs version specificationsWhile the requirements are clearly listed, specific version information would be helpful for reproducible builds:
- MSVC 2022 version number (e.g., 17.7.x)
- Minimum Make version
Consider adding version information:
### Requirements * JDK 22 * Maven -* MSVC 2022 toolset (e.g. by installing Visual Studio 2022, Workset "Desktop development with C++") -* Make (`choco install make`) +* MSVC 2022 toolset v17.7 or later (Install via Visual Studio 2022, Workload "Desktop development with C++") +* Make 4.0 or later (`choco install make`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~20-~20: Please check the verb form.
Context: ... Make (choco install make) ### Build Open a terminal and run ``` mvn clean verify...
(SHE_LIVE)
🪛 Markdownlint (0.35.0)
README.md
22-22: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
This PR changes the compiler from the GNU C++ (g++) to the Microsoft C++ compiler (cl.exe). Additionally, files for Visual Studio are added for IDE support.
Since we are on Windows anyway, we ca benefit from Windows specific optimizations/flags. Additionally, we don't have to address mingw specific quirks and can replace this dependency. The VS solution file gives proper c/c++ support to improve the developer experience.