-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CI] Update CI to include a Windows build of CppInterOp + other general CI improvements #180
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
+ Coverage 77.56% 78.68% +1.12%
==========================================
Files 8 8
Lines 2897 3050 +153
==========================================
+ Hits 2247 2400 +153
Misses 650 650 |
@alexander-penev @vgvassilev Currently I've got the Windows ci build upto the point where it needs to build CppInterOp , but it fails to find the llvm config file. I've tried a few different definitions of LLVM_DIR without success. Any ideas what the folder location should be on Windows? A separate issue which also needs to be fixed is that currently for the Windows ci build LLVM_HASH and CLING_HASH are currently hardcoded (I have an idea for a fix, but have yet to test). After these 2 issues have been fixed then this project should have a working ci which shows that CppInterOp builds on Windows, assuming no other issues arise once LLVM_DIR issue is resolved. |
Update I have now solved the LLVM_DIR and Clang_DIR issue. Now I need to solve the following issue with the cmake configure stage of building CppInterOp
|
Don't we need to use backslashes? Additionally, we can increase the 1 minute tmate session to more time here so that you can ssh into that machine and try things interactively. |
Truth be told I don't know. I never build on Windows, so most of this PR has been based on what I've read online about building on Windows. My guess is that its not supposed to be \ purely because it has no issue with any of the other path directories. |
Looking at the cmake configure stage for llvm on windows they have / not \ (example from ci shown below)
|
I did a little research and found out that although Windows paths have \ as far as cmake as is concerned the paths should always use / , as in Windows a \ is likely to interpreted as escape sequence. So to solve the current issue it needs to be determined where in the cmake files the \ path is being included, and change it to use / (references below) |
@vgvassilev I have now got past the parsing issue for the clang-repl builds of CppInterOp on Windows (worked around it by replacing \ with / in LLVM_DIR within cmakelists.txt). The issue with them now seems to be a few header files it can't find, and a few other errors . I think some more familiar with the CppInterOp code and Clang needs to fix those issues. The cling build still seems to be having issues with finding the cling config file. Not entirely sure what is causing that issue. |
We might need to include |
@vgvassilev can you delete the llvm cache from the windows build where cling was on by hand? Looking at the ci when it was last built the cling directory was incorrect, so it was never built, so that's why it can't be found. |
Done. |
I don't know how much use this, but been experimenting with getting windows to build. I have it successfully building with few things commented out. For dlfcn\dlopen i just used some of llvm support API's. You see my changes here https://github.com/fsfod/CppInterOp/commits/windows/ |
@fsfod, this looks quite nice. Are you interested in moving some of that work upstream? |
Thanks. I would also appreciate if your work is moved upstream, like @vgvassilev asks. |
What’s the plan here? Do we want to merge that PR and move to the other one with the windows changes in the codebase? |
@vgvassilev Yes. That is the plan. I thought if this PR gets merged then the changes in the other PR can be checked. I've started the process of setting myself up a build environment on a local Windows machine, so I can try and help with the other PR. This PR will help to determine when the other PR is ready for merging I believe. |
Assuming your ok for this idea then this PR is ready for review. |
Just realised looking at the ci that the patches are not applying on Windows. Looking into a possible solution. Not sure why clang-repl-16 jobs have suddenly started failing. Looking into this too. |
@alexander-penev This PR is ready for review. Applying the patches to clang is currently hardcoded for Windows, but I will fix this in a later PR. I can't explain the spaces in the patch file, except for without them the patch will fail to apply on Windows (it will output that it finds the patch file corrupt every time it meets a blank line without them). The windows ci build is not mean to pass. This PR is supposed to put in a place a ci build for Windows, with the codebase changes in #181 meant to help it pass. |
PR to add CI build infrastructure + fixes #182