Brings the Java build in line with other clients. - #2361
Conversation
Fixes several build issues that previously allowed improperly formatted files and files with incorrect copyright headers to pass CI. - Replaces the Java-specific copyright check with the standard pre-commit check. - Adds Java and Maven dependencies to the all configuration. - Moves formatting and Spotless checks into pre-commit. - Fixes formatting in files that were not properly formatted. - Removes the remaining headers_h_1 imports. Fixes NVIDIA#2131
|
/ok to test cc91b0a |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
imotov
added
improvement
|
/ok to test 77df6c3 |
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
We should not be requiring everyone running pre-commit run --all-files on this repo to have Maven installed.
Please see my suggestions about that.
(leaving a non-blocking "comment" review because we have storms in my area today and I may lose internet... if you push changes and aren't getting a timely response from me, ask others for a adi-packaging-codeowners review)
| # spotless-fmt pre-commit hook requires Maven and a JDK | ||
| - output_types: [conda] | ||
| packages: | ||
| - maven | ||
| - openjdk=22.* |
There was a problem hiding this comment.
| # spotless-fmt pre-commit hook requires Maven and a JDK | |
| - output_types: [conda] | |
| packages: | |
| - maven | |
| - openjdk=22.* | |
| # spotless-fmt pre-commit hook requires Maven and a JDK | |
| - output_types: [conda] | |
| packages: | |
| - maven | |
| - &openjdk openjdk=22.* |
Let's please avoid duplication here. This same version range is mentioned below:
Line 594 in 3aea5d8
Make this one a YAML anchor, and replace that one with *openjdk
| language: rust | ||
| - id: spotless-fmt | ||
| name: spotless-fmt | ||
| entry: mvn --batch-mode --quiet -f java/cuvs-java/pom.xml spotless:apply |
There was a problem hiding this comment.
This is going to require everyone working on cuvs to have Maven installed locally to run pre-commit run --all-files.
I expect that most folks would not ... many RAPIDS developers and contributors to this project do not do anything involving Java.
Options I could see to avoid that:
- use a
language: condahook- example: https://github.com/rapidsai/jupyterlab-nvdashboard/blob/f0c1a2b0cec0740e84f7895490c24346676b538d/.pre-commit-config.yaml#L64-L73
- but this can be very slow, see @bdice's comments here: upgrade 'uuid', move more yarn checks into pre-commit, automatically deduplicate yarn.lock rapidsai/jupyterlab-nvdashboard#279 (comment)
- wrap this in a shell script that exists gracefully if
CIis unset /falseandmvnis not available
I think the shell-script option would be preferable, can you please do that?
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Changes for my narrow comments look great, thanks for considering those!
|
/ok to test dc401dd |
|
/ok to test 7636079 |
3 participants
Fixes several build issues that previously allowed improperly formatted files and files with incorrect copyright headers to pass CI.
Fixes #2131