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

[READY] Use the archive with precompiled binaries for updating the resource dir #1741

Merged
merged 1 commit into from Apr 16, 2024

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Apr 4, 2024

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 Reviewable

Copy link
Collaborator Author

@bstaletic bstaletic left a 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

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Merging #1741 (60d43a5) into master (da9c643) will not change coverage.
The diff coverage is n/a.

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           

Copy link
Member

@puremourning puremourning left a 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?

Copy link
Member

@puremourning puremourning left a 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)

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

@bstaletic bstaletic left a 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.

@bstaletic
Copy link
Collaborator Author

Seems to be working!

Copy link
Member

@puremourning puremourning left a 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)

@puremourning
Copy link
Member

puremourning commented Apr 16, 2024

:lgtm:

@puremourning puremourning added the Ship It! Manual override to merge a PR by maintainer label Apr 16, 2024
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

Copy link
Contributor

mergify bot commented Apr 16, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit 85847ba into ycm-core:master Apr 16, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arm_neon.h missing from clang
2 participants