-
Notifications
You must be signed in to change notification settings - Fork 7
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
Implement a hacky version of GetMethodTemplate. #81
Conversation
3b01b91
to
44cd4a9
Compare
765718f
to
f648d0f
Compare
based on my experience getting the ci working on CppInterOp the reason llvm is probably failing to build is because cling is not set to clone a tagged version (the head of the repo seems broken at the moment). Maybe add a cling version option that is known to work like here and checkout that version like here |
Yes, we probably need to update the CI rules. I am starting to think that we should somehow chain these workflows to reuse code rather than cut and paste... |
386904d
to
8a11db8
Compare
@sudo-panda It's entirely possible its a cache issue (this has happened to me in the past). Are you able to clear the cache, so it will build the llvm cache from scratch? If you can't clear the cache, I suggest commenting out https://github.com/compiler-research/cppyy-backend/blob/386904d996cb6af2508b7c6482a68af48c56ad95/.github/workflows/ci.yml#L396C1-L404C1 for one run, so you can build a new llvm cache, and have it not retrieve the cache, and it should hopefully override the old cache. |
8a11db8
to
3962d76
Compare
@sudo-panda I think I found what the issue/bug is with the ci. Please change
and similarly change
and perform one run to replace the cache (so keep code which restores the cache commented out for this new run) . The verbose flag isn't strictly necessary, but it will be nice to see for certain where llvm-project is being copied over to for caching. It looks from tests on my local machine that it was creating a folder called llvm-project inside llvm-project during the recursive copy, so the llvm_dir, clang_dir etc referenced by CppInterOp didn't exist. |
I just realised that without the restore part of the ci then the key never gets defined, so can't overwrite the cache, but the run that just happened should be able to tell you whether your PR does what it intended to do. @vgvassilev can you clear the cache, so that it be fixed with the changes listed above? |
3962d76
to
bc7d660
Compare
Done. |
@sudo-panda I have created a PR which fixes the caching issue in ci (see #92 ). Before making anymore commits to the PR, please check that this PR has been merged, and that your ci is up to date with the master branch. If you commit before doing this your saved llvm cache will be broke, and you will not be able to get all the jobs to pass. I do not know the reason why recursive copy is different for osx and linux on the Github runners (as it's the same on my local machine). I only know that this PR fixes it. |
@sudo-panda The caching issue with the ci has been fixed. If you make sure your ci matches the one the master branch has, before you make any other changes, then you'll be able to test your changes. |
@maximusron, after your recent changes is that needed? |
This PR is no longer needed because of the GetMethodTemplate implementation that was merged |
No description provided.