- 
        Couldn't load subscription status. 
- Fork 35
General improvements to Emscripten build and ci #533
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
General improvements to Emscripten build and ci #533
Conversation
| These changes are super small. Let's try to put up bigger pull requests that change more things in a more uniform way. Eg. if we have 10 times to improve in the CI let's do them at once instead of generating 10 PRs. | 
| Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
+ Coverage   75.33%   75.46%   +0.13%     
==========================================
  Files           9        9              
  Lines        3620     3628       +8     
==========================================
+ Hits         2727     2738      +11     
+ Misses        893      890       -3     🚀 New features to boost your workflow:
 | 
| 
 I will do this from now on (I have the very big change in the works to have reusable actions in the workflow to stop repetition). In most cases i'll have to just put the PR title and description as general ci improvements. The reason I put this llvm cache PR (and the other 2) as small incremental changes, was so that if anything went wrong it was obvious. as to what target change or in this case remove of the folder was responsible in case any test broke. The reason I am trying to get small amounts from the cache size is that we are close to the 10GB limit, and currently we won't be able to get the llvm 20 PR in and stay under this limit (its going to be very close). If I cannot get us under this 10GB limit (I'm hoping to just scrape under it), do you have any preference as to what jobs get dropped from the ci? My suggestion would be the llvm 19 Windows job, since we aren't expecting full Windows support until at least llvm 20 I believe. | 
3c5d2a9    to
    1f180a2      
    Compare
  
    98fbe91    to
    38030de      
    Compare
  
    | @vgvassilev can you please approve this PR? I've added a few improvements to the Emscripten ci as part this PR (detailed in the PR description). If you approve I will merge when activity is quiet in the repo, since it requires me to clear some builds in the Github cache. | 
| Hi, Mostly looks good. Just a question here and there (afk currently, shall type them out soon ) | 
7151916    to
    ce98219      
    Compare
  
    | -DLLVM_INCLUDE_TESTS=OFF \ | ||
| -DLLVM_ENABLE_THREADS=OFF \ | ||
| -DLLVM_BUILD_TOOLS=OFF \ | ||
| -DLLVM_ENABLE_LIBPFM=OFF \ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you educate me on what this last flag does and why do we need it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anutosh491 going by the LLVM documentation
LLVM_ENABLE_LIBPFM:BOOL
    Enable building with libpfm to support hardware counter measurements in LLVM tools. Defaults to ON.
I got the idea for the flag from emsdks build of llvm here https://github.com/emscripten-core/emsdk/blob/b665079cb9ad9eb1704652f962281a7fa1633e2d/emsdk.py#L1124C42-L1124C66 . I thought if its good enough for the developers of Emscripten to think it can be off, then its good enough for CppInterOp.
It also makes sense that if we turn off building the tools, then we should turn off anything which builds stuff to support the tools.
| @anutosh491 if you approve, please leave it to me to merge this PR, since I need to clear some stuff from the cache first before merging, and it should be done when activity in the repo is quiet. | 
| Actually I just realized that if we build with -DLLVM_BUILD_TOOLS=OFF, it only makes sense to also use  | 
| 
 @anutosh491 I can do this additional change to the PR. Do you approve the PR subject to this change? | 
ce98219    to
    29e808a      
    Compare
  
    00d7b4d    to
    296af5c      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Maybe add the clang-enable-tools=off config in the docs too ?

Description
Please include a summary of changes, motivation and context for this PR.
This PR makes general improvements to Emscripten build and ci
Fixes # (issue)
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist