compiler-research / CppInterOp

A Clang-based C++ Interoperability Library
Other
38 stars 20 forks source link

Fix issue with wasm build cache missing #228

Closed mcbarton closed 2 months ago

mcbarton commented 2 months ago

@vgvassilev This PR should fix the cache issue with the one wasm job. Not sure why it has only just started failing, but every job should pass now,

mcbarton commented 2 months ago

@vgvassilev @maximusron This PR restored the llvm build from the cache built on the main branch, not building its own (the one which is building right now is due to a change of key for that cache). I am not sure what change helped this happen in the last few PRs, but PRs should run much quicker now.

mcbarton commented 2 months ago

@vgvassilev can you merge this PR?

maximusron commented 2 months ago

@mcbarton Looks good, CI is working great now, really like the update! The 2 stage separation and the cache usage makes it easier to look at and faster compared to earlier. If I understand correctly the staged builds for cppyy and xeus show up right after the LLVM build/cache restore job.

I was wondering if we can add the InterOp build to those jobs (tagged without cppyy or xeus) and then use that library instance for all the cppyy/xeus/wasm builds. In theory with any pull request we only need a single platform and clang specific build for InterOp. What I mean to say is that with one ubuntu clang 18 InterOp build we can run all 3 cppyy/xeus/wasm jobs since they use the same library installation (we may have to set some paths), and similarly for 17 and 16. @vgvassilev is this feasible?

I can try to implement this but it might take some time...

mcbarton commented 2 months ago

@mcbarton Looks good, CI is working great now, really like the update! The 2 stage separation and the cache usage makes it easier to look at and faster compared to earlier. If I understand correctly the staged builds for cppyy and xeus show up right after the LLVM build/cache restore job.

This is correct.

I was wondering if we can add the InterOp build to those jobs (tagged without cppyy or xeus) and then use that library instance for all the cppyy/xeus/wasm builds. In theory with any pull request we only need a single platform and clang specific build for InterOp. What I mean to say is that with one ubuntu clang 18 InterOp build we can run all 3 jobs since cppyy, xeus and wasm can use the same library installation (we may have to set some paths). @vgvassilev is this feasible?

@maximusron I would suggest against dropping down to only one llvm for the cppyy tests, due to issues like https://github.com/compiler-research/xeus-clang-repl/issues/78 which only appear on one llvm.

I have more ideas on how to speed up the CI further, such as having a stage to build CppInterOp for each llvm, between the cache build and the xeus-clang-repl/cppyy builds so that this work isn't duplicated. I will put these PRs in over the coming week.

maximusron commented 2 months ago

@maximusron I would suggest against dropping down to only one llvm for the cppyy tests, due to issues like https://github.com/compiler-research/xeus-clang-repl/issues/78 which only appear on one llvm.

Yeah I meant a single build per clang version. I think we are converging on the same idea that is to build CppInterOp on all unique combinations of platforms and LLVM versions only once for reuseability

a stage to build CppInterOp for each llvm, between the cache build and the xeus-clang-repl/cppyy builds so that this work isn't duplicated. I will put these PRs in over the coming week.

Right, this sounds like a good solution. I think we can just add them to the same job that builds LLVM since the number of jobs would be identical. This would make stage 1 - Unique LLVM + InterOp for each platform and stage 2 - cppyy/xeus/wasm where the number of jobs would be 3x stage 1

mcbarton commented 2 months ago

It should be relatively easy to separate the non wasm CppInterOp build, so that we don't duplicate that work more than once for the same clang version. We can't combine that with the wasm build version though. I'll cc you in on the PR when its ready.

vgvassilev commented 2 months ago

Shall we merge this one?

mcbarton commented 2 months ago

Yes this PR is ready to merge.