compiler-research / cppyy-backend

1 stars 6 forks source link

Implement a hacky version of GetMethodTemplate. #81

Closed vgvassilev closed 4 months ago

mcbarton commented 7 months ago

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).
https://github.com/compiler-research/cppyy-backend/blob/f648d0fa59e578dc8c00fd401ee9903bd2a5c6aa/.github/workflows/ci.yml#L111C11-L111C72

Maybe add a cling version option that is known to work like here https://github.com/compiler-research/CppInterOp/blob/aa3521b00af1aab1ae36df60865a998ceb9d1dd7/.github/workflows/ci.yml#L180C1-L180C33

and checkout that version like here https://github.com/compiler-research/CppInterOp/blob/aa3521b00af1aab1ae36df60865a998ceb9d1dd7/.github/workflows/ci.yml#L418C1-L421C16

vgvassilev commented 7 months ago

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...

mcbarton commented 7 months ago

@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.

mcbarton commented 7 months ago

@sudo-panda I think I found what the issue/bug is with the ci. Please change https://github.com/compiler-research/cppyy-backend/blob/386904d996cb6af2508b7c6482a68af48c56ad95/.github/workflows/ci.yml#L471C1-L472C60 to

    mkdir ./cppyy-backend/llvm-project/
    cp -r -v ./llvm-project/ ./cppyy-backend/

and similarly change https://github.com/compiler-research/cppyy-backend/blob/3962d76093c0e3c37eef5e2e7394955b2131d55e/.github/workflows/ci.yml#L547C1-L548C66

    mkdir .\cppyy-backend\llvm-project\
    cp -r -v .\llvm-project\ .\cppyy-backend\

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.

mcbarton commented 7 months ago

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?

vgvassilev commented 7 months ago

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?

Done.

mcbarton commented 7 months ago

@sudo-panda I have created a PR which fixes the caching issue in ci (see https://github.com/compiler-research/cppyy-backend/pull/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.

mcbarton commented 7 months ago

@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.

vgvassilev commented 4 months ago

@maximusron, after your recent changes is that needed?

aaronj0 commented 4 months ago

@maximusron, after your recent changes is that needed?

This PR is no longer needed because of the GetMethodTemplate implementation that was merged