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

LUCENE-9977: rat task corrections (proper up-to-date checks, cleanup and rewrite of the task itself). #178

Merged
merged 4 commits into from Jun 11, 2021

Conversation

dweiss
Copy link
Contributor

@dweiss dweiss commented Jun 10, 2021

No description provided.

…ich required ASL headers and didn't have them previously (for consistency).
"build/**"
]
@InputFiles
ListProperty<ConfigurableFileTree> inputFileTrees = project.objects.listProperty(ConfigurableFileTree)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentionally left as a list of file trees. We only use a single file tree but perhaps it'll be useful in the future if we had multiple file trees as an input.

String inputFileList = inputFileTrees.get().collectMany { fileTree ->
fileTree.asList()
}.sort().join("\n")
project.file(reportFile.path.replaceAll('.xml$', '-filelist.txt')).setText(inputFileList, "UTF-8")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This generates and writes a list of files processed as a sibling of the rat report file. Easy to see what was actually included in the check.

@dweiss dweiss requested a review from uschindler June 10, 2021 08:38
Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

I think this is all fine.

I tried to do it in the same way before opening the issue, but gave up when handling the sourceSets and all specialities.

It is not so nice that we have to explicitly exclude subprojects, but OK, let's live with it! An alternative would have been to only have a top-level task like at Ant times.

rat.report(format: 'xml', reportFile: reportFile, addDefaultLicenseMatchers: true) {
// Pass all gradle file trees to the ant task (Gradle's internal adapters are used).
inputFileTrees.get().each { fileTree ->
fileTree.addToAntBuilder(ant, 'resources', FileCollection.AntType.ResourceCollection)
Copy link
Contributor

Choose a reason for hiding this comment

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

perfect!
Did you do a test by removing one of the headers in some java source file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did and it worked. I thought about a single top-level task too, actually. You could make an alias from any subproject to just depend on the root-level check... but then this would mean re-running the thing for all the files and it's working so nice and fast now that I didn't bother (you could try to make it incremental but this is additional work I don't have time for).

@@ -74,6 +74,9 @@ allprojects {
exclude ".idea"
exclude ".muse"

// Exclude github stuff (templates, workflows).
exclude ".github"
Copy link
Contributor

Choose a reason for hiding this comment

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

should we not also exclude the ".git" folder itsself, or is this done by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could. None of the default inclusion patterns match anything in there, I guess. I found that "debug" file produced by rat very useful to figure out what the checked files were. I'll add .git to excluded folders.

@dweiss dweiss merged commit 3bedc08 into apache:main Jun 11, 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants