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-9475: Switch validateSourcePatterns away from Ant legacy #1806

Merged
merged 6 commits into from Aug 31, 2020

Conversation

uschindler
Copy link
Contributor

@uschindler uschindler commented Aug 30, 2020

This PR changes the legacy setp of the checkSourcePatterns task to directly execute the checker code in the running Gradle VM.

Because of Ant compatibility we preserved the old code using Ant's Groovy task. So it was executing Groovy shell inside Groovy. The code changes are minimal, further improvements are coming later.

This task has no outputs, so it's declared to run always. We may improve this, if we make the checked files and patterns a task input. Quick tests on different branches showed that task works.

This PR fixes the stupid warning caused by the groovy-inside-groovy:

> Task :validateSourcePatterns
Trying to override old definition of task fileScanner

@uschindler uschindler changed the title LUCENE-9475: Switch validateSourcePatters away from Ant legacy LUCENE-9475: Switch validateSourcePatterns away from Ant legacy Aug 30, 2020
Copy link
Contributor

@dweiss dweiss left a comment

Choose a reason for hiding this comment

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

Thanks Uwe.

@dweiss
Copy link
Contributor

dweiss commented Aug 31, 2020

Mike Drob did some work on inputs/ outputs here, it was too early though (ant compatibility).
#1441

The outputs can be empty (but declared). The inputs as per usual - file collection. Should work out of the box.

@uschindler
Copy link
Contributor Author

Oh haven't seen that PR. I was just annoyed by the warning.

@uschindler
Copy link
Contributor Author

I wanted to change the task in the same way after all. 👍🏻

@uschindler uschindler marked this pull request as draft August 31, 2020 09:18
@uschindler
Copy link
Contributor Author

Hi I converted this PR to a draft, because I am incorporating changes from #1441 (which I closed, as outdated).

@uschindler uschindler marked this pull request as ready for review August 31, 2020 10:04
@uschindler
Copy link
Contributor Author

I think it's now ready. I did not change the inner closures to be real class members. The most important thing to me was to make the inputs declared and the outputs always uptodate, so the task only runs when inputs change. The temporary file by @madrob was not needed (the trick is outputs.uptodateWhen{true} (with no outputs, gradle always executes task - well known to the forbiddenapis policeman, trick is userd there, too).

@uschindler uschindler requested a review from dweiss August 31, 2020 10:06
@uschindler
Copy link
Contributor Author

In case you ask: Making is a class extends DefaultTask allows to better declare inputs/outputs. I am not a fan of manually adding them. This code is much more readable (because of annotations).

Copy link
Contributor

@dweiss dweiss left a comment

Choose a reason for hiding this comment

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

I'm fine with it as it is - just left some hints that I think would make the separation between task class vs. task configuration better?

@uschindler
Copy link
Contributor Author

Thanks Dawid,
I will merge this soon. Robert's issue about var should be decided later. In #1802 I will add an exclude just for him.

@uschindler
Copy link
Contributor Author

uschindler commented Aug 31, 2020

By the way: Due to my changes, also gradle/** is now checked by validateSourcePatterns. I also added the .gradle file extension (you might have noticed).

Copy link
Contributor

@dweiss dweiss left a comment

Choose a reason for hiding this comment

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

Thanks Uwe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants