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

MINOR: Switch to using the Gradle RAT plugin #10491

Merged
merged 5 commits into from Apr 13, 2021
Merged

Conversation

jthurne
Copy link
Contributor

@jthurne jthurne commented Apr 6, 2021

The Gradle RAT plugin properly declares inputs and outputs and is also
cachable. This also relieves the Kafka developers from maintaining the build
integration with RAT.

The generated RAT report is identical to the one generated previously. The only
difference is the RAT report name: the RAT plugin sets the HTML report name to
index.html (still under build/rat).

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

The Gradle RAT plugin properly declares inputs and outputs and is also
cachable. This also relieves the Kafka developers from maintaining the build
integration with RAT.

The generated RAT report is identical to the one generated previously. The only
difference is the RAT report name: the RAT plugin sets the HTML report name to
`index.html` (still under `build/rat`).
@jthurne
Copy link
Contributor Author

jthurne commented Apr 6, 2021

@ijuma another one for you.

@jthurne
Copy link
Contributor Author

jthurne commented Apr 6, 2021

Full disclosure: I'm an engineer that works for Gradle (on Gradle Enterprise, and I found these optimizations while using the project to test out Gradle Enterprise features.

I put this change in a separate PR from the other build optimizations since it is a slightly larger change, and since the HTML report name changes.

Copy link

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

LGTM

@jthurne
Copy link
Contributor Author

jthurne commented Apr 7, 2021

I'm looking into the build failures on this one.

At first glance, the pr-merge build failed due to one of the test tasks crashing. Presumably that's not related at all to this change, which doesn't touch the test tasks.

Some of the other failures site a RAT license check failure. I'm looking into this to make sure it isn't something new introduced by switching to the RAT plugin.

* origin/trunk:
  MINOR: move NoOpSnapshotWriter to main (apache#10496)
  KAFKA-10769 Remove JoinGroupRequest#containsValidPattern as it is dup… (apache#9851)
  KAFKA-12384: stabilize ListOffsetsRequestTest#testResponseIncludesLeaderEpoch (apache#10389)
  KAFKA-5146: remove Connect dependency from Streams module (apache#10131)
  KAFKA-12602: Fix LICENSE file (apache#10474)
  MINOR: Enable scala/java joint compilation consistently for `core` module (apache#10485)
@jthurne
Copy link
Contributor Author

jthurne commented Apr 7, 2021

I believe the license failures were caused by a different change to trunk that has since been fixed. I merged the latest trunk HEAD into this PR to re-trigger CI.

The file encoding property can affect the Apache RAT ant task, which the RAT
plugin uses internally.
@ijuma
Copy link
Contributor

ijuma commented Apr 12, 2021

Started another build (the previous got canceled due to environmental issues).

@ijuma
Copy link
Contributor

ijuma commented Apr 12, 2021

@jthurne This change causes new failures:

[2021-04-12T03:23:02.541Z] > A failure occurred while executing org.nosphere.apache.rat.RatWork

[2021-04-12T03:23:02.541Z] > Apache Rat audit failure - 3 unapproved licenses

[2021-04-12T03:23:02.541Z] See file:///home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-10491/build/rat/index.html

@jthurne
Copy link
Contributor Author

jthurne commented Apr 12, 2021

@jthurne This change causes new failures:

Indeed. Unfortunately, I have been unable to reproduce them locally. One downside to the RAT plugin is that it doesn't output the files that fail the check to the build log. That makes this hard to diagnose since it seems to only happens on the CI server.

My suspicion, however, is that this has something to do with file encoding. I'll keep digging.

@ijuma
Copy link
Contributor

ijuma commented Apr 12, 2021

This is the output:

/home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-10491/clients/src/test/resources/serializedData/topicPartitionSerializedfile
/home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-10491/clients/src/test/resources/serializedData/offsetAndMetadataWithLeaderEpoch
/home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-10491/clients/src/test/resources/serializedData/offsetAndMetadataBeforeLeaderEpoch

The RAT plugin appears to be identifying these as text files on some platforms
(instead of identifying them as binary files which should be ignored).

Explicitly excluding the files in this directory is a workaround, but it should
be good enough for now.
@jthurne
Copy link
Contributor Author

jthurne commented Apr 12, 2021

Interesting. When I run the rat task locally, those three files are identified (correctly) as binary files and RAT skips them.

I'm not sure why they are being detected as text files on CI. But an easy workaround is to explicitly exclude that directory. I've pushed up a commit that does just that.

This reverts commit e923380.

Setting the file encoding did not appear to have any effect on the RAT plugin.
And it may have had unintended consiquences. So it's better to roll this change
back for now.
Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM. I also verified that ./gradlew rat succeeds when there is no .git folder.

@ijuma ijuma merged commit 1e8a7c4 into apache:trunk Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants