Skip to content

Conversation

@danepitkin
Copy link
Member

@danepitkin danepitkin commented Jul 17, 2024

Rationale for this change

Now that Java 8 is deprecated, we can bump checkstyle to latest version.

What changes are included in this PR?

  • Bump checkstyle to 10.17.0
  • Update checkstyle.xml to fix backwards-breaking changes
  • Fix new check errors

Are these changes tested?

CI

Are there any user-facing changes?

No

@danepitkin danepitkin requested a review from lidavidm as a code owner July 17, 2024 21:39
@danepitkin
Copy link
Member Author

@github-actions crossbow submit -g java

@github-actions
Copy link

Revision: 6583c9266e9bda47f29547acea3bf21a63a062f3

Submitted crossbow builds: ursacomputing/crossbow @ actions-3d2568a501

Task Status
java-jars GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@danepitkin danepitkin marked this pull request as draft July 17, 2024 21:42
@danepitkin danepitkin force-pushed the danepitkin/checkstyle-10-17-0 branch from 6583c92 to 92fdef9 Compare July 24, 2024 15:50
@vibhatha
Copy link
Contributor

@danepitkin might need explicit formatting?

@danepitkin danepitkin force-pushed the danepitkin/checkstyle-10-17-0 branch from 92fdef9 to f417a2d Compare July 26, 2024 16:09
@danepitkin
Copy link
Member Author

@github-actions crossbow submit -g java

@github-actions
Copy link

Revision: e390e0b7307b3183446126e3f7484b3b3ee001b6

Submitted crossbow builds: ursacomputing/crossbow @ actions-83f5abdb26

Task Status
java-jars GitHub Actions
test-conda-python-3.11-spark-master GitHub Actions
verify-rc-source-java-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-java-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-java-macos-amd64 GitHub Actions

@danepitkin danepitkin marked this pull request as ready for review July 26, 2024 19:12
@danepitkin danepitkin requested a review from vibhatha July 26, 2024 19:13
@vibhatha
Copy link
Contributor

@github-actions crossbow submit java-jars

@vibhatha
Copy link
Contributor

Oh probably this may need #43431

@github-actions
Copy link

Revision: e390e0b7307b3183446126e3f7484b3b3ee001b6

Submitted crossbow builds: ursacomputing/crossbow @ actions-0e19636c2e

Task Status
java-jars GitHub Actions

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jul 29, 2024
@lidavidm
Copy link
Member

[INFO] Scanning for projects...
Error: ] Some problems were encountered while processing the POMs:
[FATAL] Non-readable POM /Users/runner/work/crossbow/crossbow/arrow/java/maven: /Users/runner/work/crossbow/crossbow/arrow/java/maven (No such file or directory) @ 
 @ 
[INFO] 0 goals, 0 executed

needs a rebase?

@lidavidm
Copy link
Member

ah I thought the PR Vibhatha linked was merged already

@lidavidm
Copy link
Member

Ok, now it just needs a rebase!

@danepitkin danepitkin force-pushed the danepitkin/checkstyle-10-17-0 branch from e390e0b to a5c2bd1 Compare July 29, 2024 13:53
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing the property here? Wouldn't that confusion for people using Windows line separator?

Copy link
Member Author

@danepitkin danepitkin Jul 29, 2024

Choose a reason for hiding this comment

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

Good call, I should fix this properly.

The windows build was always generating windows line separators. There was a bug in the old version of checkstyle that couldn't differentiate between lf (\n) and crlf (\r\n), so this was never caught before. checkstyle/checkstyle#4074

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed! I just had to update java/.gitattributes

Copy link
Contributor

Choose a reason for hiding this comment

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

does it mean that all files are to be considerered as text files (vs binary)? Would'nt that cause issues when adding binary files (for testing for example?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should be * text=auto eol=lf?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lidavidm
Copy link
Member

@github-actions crossbow submit java-jars

@github-actions
Copy link

Revision: 4c3f929515acc1f4e119646ec36005cecc647bd9

Submitted crossbow builds: ursacomputing/crossbow @ actions-36a3d85d67

Task Status
java-jars GitHub Actions

@vibhatha
Copy link
Contributor

@lidavidm this seems to be a new failure in java-jars.

Copy link
Contributor

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

@danepitkin Thanks for working on this one. PR LGTM!

@danepitkin danepitkin force-pushed the danepitkin/checkstyle-10-17-0 branch from 4c3f929 to 24a762b Compare July 30, 2024 13:02
@danepitkin
Copy link
Member Author

@github-actions crossbow submit java-jars

@github-actions
Copy link

Revision: 24a762b

Submitted crossbow builds: ursacomputing/crossbow @ actions-40a559e941

Task Status
java-jars GitHub Actions

@danepitkin danepitkin merged commit 48782e7 into apache:main Jul 30, 2024
@danepitkin danepitkin removed the awaiting merge Awaiting merge label Jul 30, 2024
@vibhatha
Copy link
Contributor

@danepitkin have we already identified the failure in java-jars? It seems like a centOS related thing?

@danepitkin
Copy link
Member Author

Yeah, looks like CentOS 7 causing issues again!

@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 48782e7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 14 possible false positives for unstable benchmarks that are known to sometimes produce them.

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
….0 in /java (apache#43311)

### Rationale for this change

Now that Java 8 is deprecated, we can bump checkstyle to latest version.

### What changes are included in this PR?

* Bump checkstyle to 10.17.0
* Update checkstyle.xml to fix backwards-breaking changes
* Fix new check errors

### Are these changes tested?

CI

### Are there any user-facing changes?

No

Authored-by: Dane Pitkin <[email protected]>
Signed-off-by: Dane Pitkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants