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
[READY] Use the archive with precompiled binaries for updating the resource dir #1741
Conversation
d9d04a4
to
60d43a5
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.
Here's when and why the change to clang resource directory was introduced.
llvm/llvm-project@c17d9b4
In short, ARM intrinsics are 1MB and RISC-V are 10MB.
Now I wonder if we should commit these headers or maybe package them with clangd
and libclang
and have them only be available if user enables clangd/libclang completer.
I'm definitely leaning towards making these things optional.
Additional benefit of that approach would be always using the resource dir that comes from the same archive as clangd
and libclang
.
Reviewable status: 0 of 2 LGTMs obtained
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1741 +/- ##
=======================================
Coverage 95.85% 95.85%
=======================================
Files 84 84
Lines 8318 8318
Branches 163 163
=======================================
Hits 7973 7973
Misses 295 295
Partials 50 50 |
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.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
update_clang_headers.py
line 69 at r1 (raw file):
def UpdateClangHeaders( version, temp_dir ): # The headers do not seem to differ between archives, so pick the smallest archive_name = 'clang+llvm-18.1.1-arm64-apple-darwin.tar.xz'
Don’t hard-code version?
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.
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
60d43a5
to
5ab785a
Compare
While most headers in the resource directory are present in the clang source archive, some are generated during compiling. As a bonus, non-header files do not exist in the resource directory, in the archives containing binaries. Previously we have manually removed a CMakeLists.txt, but have been leaving behind some READMEs. HLSL headers are not installed, because the feature is highly experimental. openmp_wrappers/{stdlib.h,time.h} are not intended to be included in resource directory.
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.
Assuing the CI passes, this is ready.
Not vendoring the headers is a bit of a hassle. Part of download and extract should be done in cpp/CMakeLists.txt
, because of libclang completer. The resource directory should be done by both clangd and libclang, but such that they don't step on one another's toes. Should the header archives be cached somewhere? It's a bit annoying to deal with and it is not fixing a user-facing problem.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
update_clang_headers.py
line 69 at r1 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Don’t hard-code version?
Fixed.
Seems to be working! |
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.
Reviewed 236 of 236 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
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.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
Thanks for sending a PR! |
While most headers in the resource directory are present in the clang source archive, some are generated during compiling.
As a bonus, non-header files do not exist in the resource directory, in the archives containing binaries. Previously we have manually removed a CMakeLists.txt, but have been leaving behind some READMEs.
HLSL headers are not installed, because the feature is highly experimental.
openmp_wrappers/{stdlib.h,time.h} are not intended to be included in resource directory.
Closes #1740
This change is