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-9332 validate source patterns using gradle #1441

Closed
wants to merge 1 commit into from

Conversation

madrob
Copy link
Contributor

@madrob madrob commented Apr 20, 2020

If we have a gradle task with declared InputFiles and OutputFile then Gradle can figure out when the check is still up to date and when it needs to be rerun. Most of this is just a straight copy from the groovy script. This can take up to 20s off of precommit.

TODO:

  • Figure out how to enable the ratDocument part of this, it was failing on imports for me.
  • Split into modules so that we don't run the whole task each time
  • Possibly split by file types?

The rat part is the main thing that I need help with, I haven't been able to figure out the right incantation of dependency declarations and build script declarations and whatever else we need to make it in scope. Any pointers appreciated.

@madrob madrob requested a review from dweiss April 20, 2020 14:51
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.

First and foremost: I wouldn't deviate from ant until we get rid of it, Mike. The delegation to the same script (as it is now) should be kept until we remove the ant counterpart, otherwise things will get out of sync.

Separately from the above I think the validation task (if you wish to declare it as a task) should operate at a project level and be applied to each project. Then paths and exclusions become more manageable than top-level ones currently in the script... it's a good start but I'd wait with this until there is only gradle. What do you think?

@@ -1,3 +1,5 @@
import org.gradle.plugins.ide.eclipse.model.Output
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh? Why Eclipse IDE import?

@madrob
Copy link
Contributor Author

madrob commented Apr 21, 2020

otherwise things will get out of sync
I'm hopeful that there is little change in the script until we can (soon) remove ant. Point taken, though.

should operate at a project level and be applied to each project
Totally agree. I can tackle that after I get the imports working. Do you have any idea on why I'm not able to resolve the rat dependency here or what I need for it?

I tried playing around with various parameters on the ant task to push the output to a file and allow gradle daemon to know if this check was up to date or not, but wasn't able to do that. Really, I'd much prefer that alternative for now as it is much simpler, but wasn't able to make it work either so I started going down this path.

@dweiss
Copy link
Contributor

dweiss commented Apr 22, 2020

bq. Do you have any idea on why I'm not able to resolve the rat dependency here or what I need for it?

You removed the dependencies section... perhaps it's because of this? Otherwise I have no clue - should work.

I don't think it needs to be a "task" class either. You can declare inputs/outputs from within a script (task) too; makes it simpler to experiment.

I also believe a task doesn't have to declare a physical output file to be made incremental - it will re-run when the inputs change. Look at ecj-lint.gradle - it has an example of this.

@uschindler
Copy link
Contributor

Hi, I will take some parts to my new pull request #1806. This one does not merge anymore, so it needs to be redone. The problem with RAT is already solved there (the dependencies need to be declared differently).

Closing this as #duplicate of #1806

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