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

devcontainer clang-format version should match RAPIDS pre-commit requirements #55

Open
harrism opened this issue Mar 22, 2023 · 8 comments

Comments

@harrism
Copy link
Member

harrism commented Mar 22, 2023

Currently just about every time I push to CI my PRs fail clang-format checks because the clang-format in the devcontainer (cuSpatial) is v16, and RAPIDS pre-commit uses something like v11. Small things like bracket wrapping and comment alignment have changed between versions.

Would be nice for these to align.

@miscco
Copy link
Collaborator

miscco commented Oct 18, 2023

I just installed clang-format-17 in our cccl containers. Maybe that would also be a solution for rapids?

@harrism
Copy link
Member Author

harrism commented Oct 21, 2023

We need to match the version used by CI.

@miscco
Copy link
Collaborator

miscco commented Oct 21, 2023

Which one is that, I installed the 17.* as a fixed version, we can any version we want through pip

@jrhemstad
Copy link
Collaborator

The problem is that RAPIDS currently uses pre-commit to apply clang-format, which requires you to point at a specific clang-format version: https://github.com/rapidsai/cudf/blob/b390bca5055aaf91ef0e6e9f8eb0f6f25cce94d0/.pre-commit-config.yaml#L65C15-L71

You can't just rely on whatever clang-format version happens to be in the container.

@miscco
Copy link
Collaborator

miscco commented Oct 24, 2023

You can specify the exact version installed, which is the exact point of the devcontainer. To create a hermetic pre-defined environment.

@jrhemstad
Copy link
Collaborator

The point is you'd have to manually sync the version specified in the pre-commit config and the devcontainer definition. There's no way to automatically have one just use the other.

@miscco
Copy link
Collaborator

miscco commented Oct 25, 2023

From my point of view a devcontainer needs to be independent. The moment I require a user to install additional tools that have an additional non-obvious dependency on the configuration of the devcontainer, then the whole idea of a devcontainer is broken.

@harrism
Copy link
Member Author

harrism commented Oct 25, 2023

The point is you'd have to manually sync the version specified in the pre-commit config and the devcontainer definition. There's no way to automatically have one just use the other.

Can't we use rapids-dependency-file-generator for this?

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

No branches or pull requests

3 participants