search

apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.68k stars 1.04k forks source link

Port documentation-lint task to Gradle build [LUCENE-9201] #10241

Closed asfimport closed 4 years ago

asfimport commented 4 years ago

Ant build's "documentation-lint" target consists of those two sub targets.

Migrated from LUCENE-9201 by Tomoko Uchida (@mocobeta), resolved May 03 2020 Parent: #10119 Attachments: javadocGRADLE.png, javadocHTML4.png, javadocHTML5.png, LUCENE-9201.patch, LUCENE-9201-ecj.patch, LUCENE-9201-ecj-2.patch, LUCENE-9201-missing-docs.patch, task_deps1.png, task_deps2.png, task_deps3.png Linked issues:

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

This is WIP branch: https://github.com/mocobeta/lucene-solr-mirror/commit/7adc390183b10ea1b64fded000a87900853cf912

To get started I'm trying to port the ECJ task. The compiler seems to work by retrieving dependencies from each sub project "configurations", but WARNINGS are suppressed (Ant build outputs a lot "ecj-lint" warnings). I twisted Gradle logger level for the STDOUT, that didn't work for me.

asfimport commented 4 years ago

Erick Erickson (@ErickErickson) (migrated from JIRA)

Tomoko:

I'm looking at the remaining python tasks, so I went about listing them all.

There are two targets in ./lucene/common-build.xml that I'm not certain about:    - check-broken-links    - check-missing-javadocs

 

Do you consider these part of this task or a separate one? If a separate one, I'll put it on my list, whereas if you expect to include it in this task I'll ignore them.

 

BTW, I ran the documentationLint task, looks good. The only way I've found to get rid of all of the compiler warnings is to edit ./gradle/defaults-java.gradle, there's got to be a better way though, and I like having them on all the time, provides incentive for us to fix them...

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

@ErickErickson I added sub-tasks equivalent to the ant targets.

And I opened a PR :)

https://github.com/apache/lucene-solr/pull/1242

I think this is almost equivalent to Ant's "documentation-lint", with some notes below. @ErickErickson @dweiss Could you review it?

Note:

For now, Python linters - checkBrokenLinks, checkMissingJavadocsClass and checkMissingJavadocsMethod - will fail because the Gradle-generated Javadocs seems to be slightly different to Ant-generated ones.

After Gradle generated Javadoc is fixed, we can return to here and complete this sub-task.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I think gradle provides slightly different options to the javadoc tool than ant, which creates the problem. For example, gradle build has only one "linkoffline" but ant build has two. Such small differences could create broken links.

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Please leave a patch or pull request. I will review and provide feedback but no earlier than Sunday/Monday.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

@mocobeta has a pull request for this issue already: https://github.com/apache/lucene-solr/pull/1242

I will do some investigation too. Maybe the actual javadocs/pythonscripts side can be cleaned up to make this less painful. For example it is not good that some python checks only can parse old javadocs format that has been removed since java 13. So even with the current ant build, things are not in great shape. Ideally java's doclint would be leaned on more (e.g. enable html check, remove jtidy crap). It will make builds faster and reduce maintenance. Gotta at least try :)

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Package summary: "ant documentation" uses "package.html" as package summary description, but "gradlew javadoc" ignores "package.html" (so some packages lacks summary description in "package-summary.html" when building javadocs by Gradle). We might be able to make Gradle Javadoc task to properly handle "package.html" files with some options. Or, should we replace all "package.html" with "package-info.java" at this time?

It is a stupid complex issue. The problem here also exists in the ant build. The underlying issue is that java 8 produced HTML4 by default. Java 9+ is doing HTML5. Java 9+ is generating HTML5 even though its manual page implies that HTML4 is still the default. This kind of shit makes our build too complicated.

Possibly in branch 8.x we can simply pass "-html4" option to javadoc processor so that it always generates html4 output, even if you happen to use java 9,10,11,12,13,etc to invoke it. Currently if you use java 9+ on this branch, javadocs are messed up, you get no overview.html, etc. Forcing html4 for this branch seems like the best way. I will investigate for sure.

On the other hand with master branch things are easier: java 11 is a minimum requirement, so let's not fight the defaults (which is HTML5). Maybe I am wrong here, if we change our minds we can just revert my commits and wire in HTML4. I fixed the syntax issues already in #10249. We must get the overviews working again, so I am not sure if package-info.java is the best solution (it addresses package.html, but what about overview.html?)

Sorry for the long answer, but at least it is no mystery.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

With java 13+, the -html4 option is removed. So we are forced to adopt html5. I feel a little better :) Fixing branch_8x is hopeless: java 8 can only generate html4 and java13 can only generate html5.

So now we should focus on fixing package.html/overview.html in the master branch. I will look into it a bit.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I looked in more on the package.html/overview.html. This seems to be purely a gradle issue of not passing all the parameters to "javadocs".

I tested 3 cases:

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

The missing overview.html is caused by bugs in the defaults-javadoc.gradle code:

      opts.overview = file("src/main/java/overview.html").toString()

It points to the wrong place for most lucene modules which are src/java not src/main/java.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I will push the obvious fix to master, but it would be great to improve the gradle code. I think we need explicit file exists check so that the build fails clearly if the overview.html is missing. It should be present for any of these artifacts.

 

asfimport commented 4 years ago

ASF subversion and git services (migrated from JIRA)

Commit a77bb1e6f57ed21d484c3927d710679166918878 in lucene-solr's branch refs/heads/master from Robert Muir https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=a77bb1e

LUCENE-9201: add overview.html from correct location to the javadocs in gradle build

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

FYI I also discovered #10250 as a part of investigations here, it is another TODO. With ant, the additional custom js/css syntax-highlights the sample code. Also some CSS classes are used for the HTML5 transition. It only impacts presentation, so it doesn't cause failures, but it would be good to fix gradle to use these.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

now that overview.html works, i tried investigating package.html problems. i can reproduce it and the problem is specific to gradle. switching to package-info.java is definitely a solution, but i can't stand unexplained mysteries.

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

Thank you @rmuir for your work and comments.

I updated the PR (refactored the gradle tasks and ported ant build details, as much as I can). I hope it is a good starting point, if not perfect. There are still not ported the ant scripts' hacks, especially around "ecj-macro" stuff, that I cannot figure out how to copy to gradle.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

OK, thanks for working on the PR. At a glance it looks good to me. But we may get better feedback from Dawid Weiss when he is back online in a few days.

I will try to investigate more of the problems that you uncovered...

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Package summary: "ant documentation" uses "package.html" as package summary description, but "gradlew javadoc" ignores "package.html" (so some packages lacks summary description in "package-summary.html" when building javadocs by Gradle). We might be able to make Gradle Javadoc task to properly handle "package.html" files with some options. Or, should we replace all "package.html" with "package-info.java" at this time?

I found the answer to this. Gradle is fundamentally broken here, its not possible to fix it.

When ant runs javadocs, we supply just a source directory (-sourcepath) and a list of packages:

javadoc -sourcepath /home/rmuir/workspace/lucene-solr/lucene/core/src/java org.apache.lucene org.apache.lucene.analysis org.apache.lucene.analysis.standard ...

When gradle runs javadocs, it does not do this, it passes each .java file individually:

javadoc '/home/rmuir/workspace/lucene-solr/lucene/core/src/java/org/apache/lucene/search/SearcherFactory.java'
'/home/rmuir/workspace/lucene-solr/lucene/core/src/java/org/apache/lucene/search/QueryCache.java' ...

it seems the whole design is to make it work with their SourceTask/FileTree crap. And you can't pass individual html files to the javadoc tool to workaround it. It takes only source files or package names.

I can't see any way to pass their task package list the way we do with ant: it REALLY wants to be based on the FileTree. Maybe we should call the ant task from gradle? They really messed this up.

The other thing that seems really broken is the missing linkoffline. There are links between the modules (e.g. lucene-analyzers and lucene-core) and the linkoffline makes that work. But it seems the gradle build is structured to make per-module output dirs which won't work here.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

If i remember, the problem with "just" converting to package-info.java is the split lucene packages across modules, something goes bad (i think doc links among other issues). I will at least assess how bad that is to fix (I am not really sure), to fight tooling less. A major release is the time to fix it.

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Gradle's built-in javadoc task is indeed pretty dumb with respect to the set of options allowed by the javadoc tool. Maybe they take the lowest denominator across all javadoc/jvm versions, I don't know.

We can change it, I don't mind. We don't even have to rely on ant - we can just run javadoc as an external tool and build the set of options required.

The pull request attached to the issue has some odd fragments in it (filtering for projects based on directory structure for example). I'm not familiar with what ant does here. Should I review the PR or should we rather focus on trying to remove python from the loop and use the built-in html validation combined with custom doclet instead?

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

maybe we should try to get the PR in and iterate from there? It find a lot of problems! Maybe we should break into issues.

as far as the pr itself:

as far as javadoc generation itself, it is buggy, and we should fix it.

as far as checkers themselves:

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

One small thing about the equivalent "ant documentation" (gradle built-in Javadoc task or our customized one), 

I think it'd be better the javadoc generation task outputs all javadocs to module-wide common directory (e.g., lucene/build/docs or solr/build/docs) just like ant build does, instead of each module's build directory. This makes things easy for succeeding "broken links check" (running checkJavadocLinks.py - or its replacement?) and release managers work that should includes updating the official documentation site (https://cwiki.apache.org/confluence/display/LUCENE/ReleaseTodo#ReleaseTodo-Pushdocs,changesandjavadocstotheCMSproductiontree).

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

Or, we would need a special task to collect all javadocs to one place for publishing documentation (if changing output dir isn't a preferred way).

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

we would need a special task to collect all javadocs to one place for publishing documentation

It is my personal preference to have a project-scope granularity. This way you can run project-scoped task (like gradlew -p lucene/core javadoc). My personal take on assembling "distributions" is to have a separate project that just takes what it needs from other projects and puts it together (with any tweaks required). This makes it easier to reason about how a distribution is assembled and from where, while each project just takes care of itself.

Again - the above isn't a convention. It's just a style I gradually developed that has been working for me in other projects. If you take a look at the current Solr packaging project it's pretty much what I have in mind:

https://github.com/apache/lucene-solr/blob/master/solr/packaging/build.gradle

Let me look at the patch again later today (digging myself out of the vacation hole).

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Dawid, have you looked at how the documentation links across modules? It is fundamentally incompatible with that preference.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

"documentation-lint" checker lints more than just javadocs. Some of the stuff is generated from markdown etc.

Look at main page: http://lucene.apache.org/core/8_4_1/ That's the start to our generated documentation page. It links to every module :)

Implicit links: These implicit links (case 1 and case 2) go to the appropriate dependency based on java package. This is the linkoffline stuff. Have a look at: http://lucene.apache.org/core/8_4_1/analyzers-icu/index.html

Case 1: Look at ICUTokenizerFactory and how e.g. ResourceLoaderAware links to analyzers-common Case 2: On the other hand look at ICUTokenizerFactory and how Tokenizer links to lucene-core

Explicit links: Have a look at http://lucene.apache.org/core/8_4_1/core/org/apache/lucene/analysis/Analyzer.html

See where it says "For some concrete implementations bundled with Lucene, look in the analysis modules" and then it links to different implementations across different modules. This uses a different type of link, not auto-generated magically based on packages but instead explicitly referenced based on docRoot. So for example, core Analyzer class links to whole "analyzers-icu" module via <a href="{@docRoot}/../analyzers-icu/overview-summary.html">ICU</a>

Currently, documentation is not structured "per-module" but really as a whole package for all of lucene. (at least that is how i think about it). There are a lot of dependencies and references implemented via links, that is why the links checker is critical. It is also why "per-module build" is a huge mismatch to how the documentation is structured.

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Yeah, I did see some of it. I didn't look deep into how difficult it'd be to make it compile independently. I'm really not against recreating the ant build structure, I just wanted to point out my opinion and personal preference on the matter :) Since we're rewriting it there is some room for improvement (like the doclet idea, which I really like).

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

It is my personal preference to have a project-scope granularity. This way you can run project-scoped task (like gradlew -p lucene/core javadoc). My personal take on assembling "distributions" is to have a separate project that just takes what it needs from other projects and puts it together (with any tweaks required). This makes it easier to reason about how a distribution is assembled and from where, while each project just takes care of itself.

I get it, have no objection. Let's keep the javadocs output as is and gather them when it's needed. We might want an independent task to gather all javadocs, though I did it in the "checking broken links" task for the present.

Let me look at the patch again later today (digging myself out of the vacation hole).

Thank you, I updated the PR according to comments from Uwe Schindler. Also I added "// FIXME" comments to some code that don't work well or need to be fixed.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I have a proposal that maybe works better. I'll write it again for the 2nd time (JIRA's bugs are super annoying, i just clicked "Visual" to preview all that i had written and all my content totally disappeared: so here i am typing it again).

Let's consider the simplest case: lucene core and demo modules.

BUILDING DEMO MODULE In the current ant build, demo's javadocs task depends on core's javadocs task. But I think strictly all that is necessary is core's package list (element-list file), which can be done simpler and faster?

org.apache.lucene
org.apache.lucene.analysis
org.apache.lucene.analysis.standard
org.apache.lucene.analysis.tokenattributes
org.apache.lucene.codecs
org.apache.lucene.codecs.blocktree
...

This means fixing the gradle offline logic for the JDK too. It needs to match ant and have JDK "package-list", so that it knows which packages should link to the JDK site, vs which ones should link somewhere else. This is also missing from the gradle build today, just another thing to fix.

STRUCTURING DOCS In the current ant build, we have:

build/docs/core/index.html
build/docs/demo/index.html

The docs folder is uploaded to the website when releasing. Instead I propose we upload a structure that reflects the source-code structure. It would instead be:

demo/build/docs/javadoc/index.html
core/build/docs/javadoc/index.html

Because generated docs would not be in one folder but instead spread across many folders, we'd need some basic tooling to help get it to the website. We would also have to adjust all the explicit links (see my comment above), but there are not that many of them.

LINTING DOCS For running the "documentation-lint" check, today this is not a per-module task.

I think it should be renamed "checkDocumentationLinks" or something like that. All it would do is validate hyperlinks between pieces of the documentation, to keep this top-level task minimal. All other checks should be moved to per-module (ideally done with the javadoc tool itself).

edit: "preview before post" does not work in JIRA without high risk of losing all of your work. Sorry for the noise, please send complaints to atlassian, not me.

edit2: OK I'm done with JIRA.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

We might want an independent task to gather all javadocs, though I did it in the "checking broken links" task for the present.

The issue is see, is if we add a independent task to gather all the docs in the existing structure, it will be mega-slow. Thats because "gathering" is a lot more expensive when its tens of thousands of files.

So I tried to work around that issue with my proposal above. But a "gather" would definitely be easier for us to implement, we'd have the same structure as today when it is done, then we could validate it. The downside is it makes the check (and hence "precommit" etc) a lot slower because of the copying.

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Can we use symbolic links from windows yet? Sorry for the dumb question, i am just trying to figure out workarounds.

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Files.createSymbolicLink on a directory attempts to create a true directory symlink which fails for me by default (requires admin permissions or explicit all-users permission on Windows). Directory junctions work without such permissions but they're not supported by Java NIO.

Gradle's sync is fairly fast on subsequent runs but it doesn't solve the first-time penalty.

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

Just from curiosity, I roughly assessed the penalty of "copying all javadocs" on my PC (Core i7-8700 cpu, fedora).

// when the javadocs is already collected into one directory, and just run the python linter

 $ ./gradlew checkBrokenLinks
 BUILD FAILED in 29s

// first collect javadocs into a directory on HDD (7200rpm), and run the python linter

 $ ./gradlew checkBrokenLinks
 BUILD FAILED in 37s

When I did same thing on NVMe SSD, there was almost no penalty, just in case.

(note: "BUILD FAILED" is an expected result for now.)

asfimport commented 4 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Tomoko thank u for measuring. my cpu is not as good but this shows it is ok. maybe we can improve later on platforms that support symlink.

we still have to address the linkoffline bugs by either building javadocs of a modules dependencies or computing their package list.

and we still have to stop using this gradle javadocs task like, yesterday, because it messes up our embedded html... especially package.html

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Hi @mocobeta. Please take a look at the patch I just attached. This has the structure and logical organisation that Uwe mentioned (tasks created per source set, aggregation task ecjLint for each project, etc.) but is still far from optimal. In general the task you chose to start from is kind of difficult - don't get discouraged! :)

The patch generally compiles the sources via ecj (main source separately from test sources; everything with an appropriate set of dependencies). I didn't add javadoc-linting options since there are still some problems I couldn't figure out (and I need a break now). Namely:

Would you mind taking another look at this? Does it help anyhow?

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

@dweiss Thank you for your guidance and detailed comments in the patch!

I applied this LUCENE-9201-ecj.patch to my branch and ran the ecjLint task, then found the task seemed to finish in one second without executing the ECJ's Main class. (I intentionally added an unused import to o.a.l.a.Analyzer but that wasn't detected.)

$ ./gradlew :lucene:core:ecjLint
BUILD SUCCESSFUL in 1s
4 actionable tasks: 4 up-to-date

I have not found any problems in the task definition (and not yet studied deeply how it works), but am I missing something?

With my previous patch https://github.com/apache/lucene-solr/pull/1242/files, for example, the :lucene:core:ecjLint takes about five seconds and (correctly) fails with this failure message if there are unused imports.

$ ./gradlew :lucene:core:ecjLint
> Task :lucene:core:ecjLint
----------
1. ERROR in /mnt/hdd/repo/lucene-solr-mirror/lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java (at line 27)
        import java.util.ArrayList;
               ^^^^^^^^^^^^^^^^^^^
The import java.util.ArrayList is never used
----------
1 problem (1 error)

> Task :lucene:core:ecjLint FAILED
FAILURE: Build failed with an exception.

* Where:
Script '/mnt/hdd/repo/lucene-solr-mirror/gradle/validation/documentation-lint.gradle' line: 130
* What went wrong:
Execution failed for task ':lucene:core:ecjLint'.
> Process 'command '/usr/local/java/adoptopenjdk/jdk-11.0.3+7/bin/java'' finished with non-zero exit value 255
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
* Get more help at https://help.gradle.org

BUILD FAILED in 4s

I will look a bit more closely at why the ecjLint task doesn't work for me...

LUCENE-9201-ecj.patch ```diff diff --git a/build.gradle b/build.gradle index 5db7b0218ee..fe69e3f9bff 100644 --- a/build.gradle +++ b/build.gradle @@ -79,6 +79,7 @@ apply from: file('gradle/validation/validate-source-patterns.gradle') apply from: file('gradle/validation/config-file-sanity.gradle') apply from: file('gradle/validation/rat-sources.gradle') apply from: file('gradle/validation/owasp-dependency-check.gradle') +apply from: file('gradle/validation/ecj-lint.gradle') // Source or data regeneration tasks apply from: file('gradle/generation/jflex.gradle') diff --git a/gradle/validation/ecj-lint.gradle b/gradle/validation/ecj-lint.gradle new file mode 100644 index 00000000000..37810f62a9d --- /dev/null +++ b/gradle/validation/ecj-lint.gradle @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// This adds 'ecjLint' task. + +configure(rootProject) { + configurations { + ecjDeps + } + + dependencies { + ecjDeps 'org.eclipse.jdt:ecj:3.19.0' + } +} + +allprojects { + plugins.withType(JavaPlugin) { + // Create a [sourceSetName]EcjLint task for each source set + // with a non-empty java.srcDirs. These tasks are then + // attached to project's "ecjLint" task. + def lintTasks = sourceSets.collect { sourceSet -> + def srcDirs = sourceSet.java.srcDirs.findAll { dir -> dir.exists() } + + tasks.create("${sourceSet.name}EcjLint", JavaExec, { + // This dependency is on a configuration; technically it causes + // all dependencies to be resolved before this task executes + // (this includes scheduling tasks that compile the + // sources from other projects for example). + dependsOn sourceSet.compileClasspath + + // We create a task for all source sets but ignore those + // that don't have any Java source directories. + enabled = !srcDirs.isEmpty() + + classpath = rootProject.configurations.ecjDeps + main = "org.eclipse.jdt.internal.compiler.batch.Main" + + // Don't emit any .class files. + // Hack around "-d none" still emitting package-info.class + // by running in a temporary directory. + def tmpDst = getTemporaryDir() + workingDir tmpDst + args += [ "-d", "none" ] + + // Compilation environment. + args += [ "-source", project.java.sourceCompatibility ] + args += [ "-target", project.java.targetCompatibility ] + args += [ "-encoding", "UTF-8"] + args += [ "-nowarn" ] + + doFirst { + // Add classpath locations at execution time (can't resolve the + // configuration at evaluation time). + if (!sourceSet.compileClasspath.isEmpty()) { + args += ["-classpath", sourceSet.compileClasspath.asPath] + } + + // Add source location(s). + // XXX: this doesn't take "excludes" into account (solr-ref-guide trips on it). + args += srcDirs + // This sadly exceeds max allowed command line size. + // args += sourceSet.java.files + } + }) + } + + task ecjLint() { + description "Lint Java sources using ECJ." + group "validation" + + dependsOn lintTasks + } + + // Attach ecjLint to check. + check.dependsOn ecjLint + } +} + +// XXX: Exclude solr-ref-guide from the check (excludes are not taken into account +// and linting of the ant-based task fails. +configure(project(":solr:solr-ref-guide")) { + afterEvaluate { + project.tasks.findByPath("mainEcjLint").enabled = false + } +} \ No newline at end of file diff --git a/gradle/validation/precommit.gradle b/gradle/validation/precommit.gradle index 7ba530e62f2..f98952dcf1d 100644 --- a/gradle/validation/precommit.gradle +++ b/gradle/validation/precommit.gradle @@ -39,6 +39,7 @@ configure(rootProject) { "licenses", "javadoc", "rat", + "ecjLint" ]} } } diff --git a/solr/solr-ref-guide/build.gradle b/solr/solr-ref-guide/build.gradle index 5a40e27ed51..7e6889414a5 100644 --- a/solr/solr-ref-guide/build.gradle +++ b/solr/solr-ref-guide/build.gradle @@ -96,6 +96,9 @@ dependencies { sourceSets { refGuide { java { + srcDirs = [] + } + resources { srcDirs = ['src'] } } @@ -107,6 +110,12 @@ sourceSets { exclude "**/asciidoctor-antlib.xml" } } + + test { + java { + srcDirs = [] + } + } } ext { ```
asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

The problem is that the task does not define an Input (sourceSet or source files). I think it should use the sourceSet as input. Now it just does nothing if it was executed previously.

asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

The problem here looks like that the JavaExec task only takes the argument line as input. But as this does not change when a file is modified, it won't reexecute the task.

To fix this we may need to tell the new task to use the sourceSet as input. I think there was a method to do this?! Maybe Dawid knows. Something like Task#addInput

asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

For debugging purposes you can force gradle to run all tasks, although it thinks that it's up-to-date: --rerun-tasks

asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

You should be able to add: task.inputs.files(srcDirs) to task definition. In many cases we might have to also add the classpath as input, otherwise it might not reexecute when dependencies change.

asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

See: https://stackoverflow.com/questions/31342386/use-inputs-file-for-javaexec-task

asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

To fix the issue completely, we also have to add some outputs.

Another workaround is specified in the stack overflow thread. Just disable up-to-date checks. This is a common problem for JavaExec. Gradle does not run it because it thinks that it's up-to-date, because it declared only opaque argument line as input (won't change) and no outputs (so there is no reason to run it).

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Hi Tomoko. Yes, the task as I included it in the patch won't run twice (it actually shows you it's "up-to-date" at the end of the input. You can run gradle with "--console=plain" then it's more obvious which tasks run and which have been skipped.

As for fixing this it really depends on what you want. Currently the only "dependency" of this task is classpath (which is not complete). I think there is no harm in precommit checks just being run all the time (at least initially) so this would work if you add it to task block definition [1]:

outputs.upToDateWhen { false }

A better solution would be indeed to declare proper task inputs (all sources involved in the check; the classpath is already a dependency). Contrary to what Uwe said you don't need any special handling for task outputs for this particular task (because we don't rely on them or cache them). I think it can just stay with inputs for precommit reasons.

I'm also sorry I didn't find the time to complete the example - too much work on other fronts and family obligations. Unless you beat me to it I'll try to complete it though and commit it in so that it shows you the full picture.

[1] https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/TaskOutputs.html#upToDateWhen-groovy.lang.Closure-

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

Thanks, I got the current status. I will try to create a patch for #10259.

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Hi @mocobeta I think you were onto something because the patch had a subtle problem (that is one of the most frustrating in gradle). The sourceSet evaluated in the ecj script had folders that were later adjusted in the ant back-compat script. In effect, the inputs for ecj were always empty (pointing at convention-based layout src/main/java etc.).

 

I corrected this, added inputs/ outputs and the remaining arguments for ECJ. Seems to be working for me now but I'd appreciate a second look (and some regression experiments against ant version).

 

There are also some odd warnings printed about Class-Path attribute invalid in one of Solr dependencies... I'm going to just ignore this for now.

asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Task looks fine, but taskname for the per SourceSet task should be created by: https://docs.gradle.org/current/dsl/org.gradle.api.tasks.SourceSet.html#org.gradle.api.tasks.SourceSet:getTaskName(java.lang.String,%20java.lang.String)

asfimport commented 4 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

There are also some odd warnings printed about Class-Path attribute invalid in one of Solr dependencies... I'm going to just ignore this for now.

Also happens in Ant.

asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Task looks fine, but taskname for the per SourceSet task should be created by:

Ah, thanks Uwe - didn't know about it! Tomoko - would you be able to modify as Uwe suggested, test it and create a pull request for this updated change?

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

Yes, the patch works for me too, with this modification. It's not explicitly documented, but sourceSet.getTaskName("ecjLint", null) generates "ecjLintMain" for main sourceSet and "ecjLintTest" for test sourceSet (on Gradle version 6.0).

 //tasks.create("${sourceSet.name}EcjLint", JavaExec, {
 tasks.create(sourceSet.getTaskName("ecjLint", null), JavaExec, {

 

Can I ask a few more questions before creating a patch?

  1. Even when I commented out those lines in gradle.build, ":solr:solr-ref-guide:ecjLint" finished without doing nothing (linting was safely skipped by the changes in solr/solr-ref-guide/build.gradle, if my understanding is correct). Is this configuration still needed?
// This excludes solr-ref-guide from the check (excludes are not taken into account
// and linting of the ant-based task fails.
configure(project(":solr:solr-ref-guide")) {
  afterEvaluate {
    project.tasks.findByPath("mainEcjLint").enabled = false
  }
}
  1. Currently all other check tasks are grouped in "Verification", so would it be better to change the task group name "validation" to "Verification"?
$ ./gradles tasks
...
Validation tasks
----------------
ecjLint - Lint Java sources using ECJ.

Verification tasks
------------------
check - Runs all checks.
checkUnusedConstraints - Ensures all versions in your versions.props correspond to an actual gradle dependency
forbiddenApis - Runs forbidden-apis checks.
owasp - Check project dependencies against OWASP vulnerability database.
rat - Runs Apache Rat checks.
test - Runs the unit tests.
verifyLocks - Verifies that your versions.lock is up to date
asfimport commented 4 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

  1. Feel free to switch to sourceSet.getTaskName.
  2. The solr-ref-guide shouldfail on linting without this block because it'll have incomplete classpath and won't be able to compile one of the source files (we don't use that one in Gradle; ant build adds ant classpath so it compiles). I think the task has been skipped because it observed no changes on inputs. Try running with --rerun-tasks and I'm sure it'll fail.
  3. Correct group name, good catch.

 

asfimport commented 4 years ago

Tomoko Uchida (@mocobeta) (migrated from JIRA)

Hi,

there remain two documentation-lint tasks to be ported. I opened for a pull request for the easy one - "check missing javadocs" task that can be defined on each sub project. https://github.com/apache/lucene-solr/pull/1267

This is functionally same to my previous pull request, but I rewrote it in a bit more declarative manner. It depends on gradle default Javadoc task for now, I think the basic logic can be applied when we switch to our custom javadoc task. Could you review it or give some comments for this?