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

Remove and ignore .clangd file. #1376

Closed
wants to merge 1 commit into from

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Nov 9, 2023

Description

This PR removes an extraneous .clangd configuration that was added during the devcontainers PR #1328. This file was not committed to any other RAPIDS repos with devcontainers and is listed in .gitignore for several RAPIDS repos, so it's safe to remove. I also added an ignore for .nfs* to match cudf's ignore list -- this is helpful when network file systems create temporary cruft.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@harrism
Copy link
Member

harrism commented Nov 10, 2023

I rely on clangd. Is this change going to break it?

@bdice
Copy link
Contributor Author

bdice commented Nov 10, 2023

I rely on clangd. Is this change going to break it?

I don’t think so. This file seems to be autogenerated by rapids-compose (and devcontainers?), because I keep getting a diff locally in this file and have to ignore it every time I work on rmm. It is ignored by other repos that have devcontainers enabled and they seem to have clangd working fine.

@bdice bdice added tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Nov 13, 2023
@bdice
Copy link
Contributor Author

bdice commented Nov 20, 2023

Hmm. This PR might not be right. Other repos do have a cpp/.clangd file committed but have a gitignore entry for .clangd (presumably ignoring .clangd in the root). Maybe that file does need to be committed, but rmm is a special case because it doesn't have a cpp/ folder? @trxcllnt Do you have insight here?

@vyasr
Copy link
Contributor

vyasr commented Nov 20, 2023

I've observed similar behavior in multiple repos where the clangd file is committed but is modified by something in compose.

@harrism
Copy link
Member

harrism commented Nov 21, 2023

@bdice indeed it seems that in dev containers there is an included feature that adds a clangd config in /home/coder/.config/clangd/config.yaml. I think this may be the source.

Hmm, here's another one.

The contents are mostly the same as, but slightly different from the .clangd this PR aims to remove.

@bdice bdice added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Nov 22, 2023
@bdice bdice marked this pull request as draft November 22, 2023 19:16
@bdice
Copy link
Contributor Author

bdice commented Nov 28, 2023

From conversation with @trxcllnt, I think removing this file is probably wrong and will break clangd support in devcontainers. I'll close this PR.

@bdice bdice closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - DO NOT MERGE Hold off on merging; see PR for details improvement Improvement / enhancement to an existing function non-breaking Non-breaking change tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants