Skip to content
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

Fix issue with wasm build cache missing #228

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

mcbarton
Copy link
Collaborator

@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
Copy link
Collaborator Author

mcbarton commented Apr 10, 2024

@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
Copy link
Collaborator Author

@vgvassilev can you merge this PR?

@aaronj0
Copy link
Collaborator

aaronj0 commented Apr 10, 2024

@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
Copy link
Collaborator Author

@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 compiler-research/xeus-clang-repl#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.

@aaronj0
Copy link
Collaborator

aaronj0 commented Apr 10, 2024

@maximusron I would suggest against dropping down to only one llvm for the cppyy tests, due to issues like compiler-research/xeus-clang-repl#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
Copy link
Collaborator Author

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
Copy link
Contributor

Shall we merge this one?

@mcbarton
Copy link
Collaborator Author

Yes this PR is ready to merge.

@vgvassilev vgvassilev merged commit 8dd6b87 into compiler-research:main Apr 10, 2024
56 checks passed
@mcbarton mcbarton deleted the fix-cache branch May 22, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants