fix(build): resolve d8 path and nativeCTest execution on Windows - #112
fix(build): resolve d8 path and nativeCTest execution on Windows#112yangyws wants to merge 4 commits into
Conversation
- Add d8.bat resolution in mirrorserver/build.gradle.kts for Windows SDK compatibility - Add onlyIf check to nativeCTest task in app/build.gradle.kts to skip on Windows - Fixes stormpanda#104
|
Hey @yangeric, thanks for the PR! One remark: The PR description states that native C tests are executed on Windows via bash wrapper, however the code tells a different story: onlyIf {
val isWindows = System.getProperty("os.name").lowercase().contains("win")
!isWindows
}
commandLine("./run_native_tests.sh")
}It is necessary that those tests also execute successfully in a Windows environment to allow development for all features of the app. |
| group = "verification" | ||
| description = "Compiles and executes native C unit tests." | ||
| workingDir = rootProject.projectDir | ||
| onlyIf { |
There was a problem hiding this comment.
this contradicts the PR description and also the requirement we talked about that all tests have to be executed and pass on Windows just like on MacOS
…Gradle Configuration Cache compatibility
|
Hi @stormpanda, updated PR #112 to fix nativeCTest execution on Windows while keeping Gradle Configuration Cache compatible:
Ready for your review! 🙂 |
|
Here is the verification execution log of native C unit tests ( |
There was a problem hiding this comment.
without a change to the code, this binary should not have changes. It has to compile to the same result byte for byte on all systems, otherwise we will play ping pong with every commit
| val isWindows = System.getProperty("os.name").lowercase().contains("win") | ||
| if (isWindows) { | ||
| val gitSh = File("C:/Program Files/Git/bin/sh.exe") | ||
| val gitBash = File("C:/Program Files/Git/bin/bash.exe") |
There was a problem hiding this comment.
this is, from a robustness point of view, a naive approach since users might install Git anywhere on their drives. This needs to %PATH% resolution to be more robust and to give better feedback to the user if no shell is available.
There more I am thinking about it, there more think we should just enforce using WSL on Windows machines. Even if this works, it only fixes this script but not all the other ones that are used for Megingiard development. Have you tried using WSL? In a perfect world, the scripts should just work there if all required dependencies are properly installed (which is something that I could list in the readme)
…r dex byte consistency
|
Hi @stormpanda, thanks for the detailed review! I've updated PR #112 according to your recommendations:
Ready for your re-review! 🙂 |
|
Hey, thanks for the update. I am currently making heavy structural changes to Megingiard. Let's keep this open until I am done. I am also still considering just going the WSL route. It would be way more cohesive because most of the necessary changes would not be necessary anymore. |
Resolves #104
Fixes build and test execution errors when building Megingiard on Windows.
Changes Included
un_native_tests.sh\ via \�ash\ wrapper on Windows so native tests can start properly.