Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.2, 6.0
    • Component/s: general/build
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Update forbidden-apis plugin to 1.8:

      • Initial support for Java 9 including JIGSAW
      • Errors are now reported sorted by line numbers and correctly grouped (synthetic methods/lambdas)
      • Package-level forbids: Deny all classes from a package: org.hatedpkg.** (also other globs work)
      • In addition to file-level excludes, forbiddenapis now supports fine granular excludes using Java annotations. You can use the one shipped, but define your own, e.g. inside Lucene and pass its name to forbidden (e.g. using a glob: **.SuppressForbidden would any annotation in any package to suppress errors). Annotation need to be on class level, no runtime annotation required.

      This will for now only update the dependency and remove the additional forbid by Shalin Shekhar Mangar for MessageFormat (which is now shipped with forbidden). But we should review and for example suppress forbidden failures in command line tools using @SuppressForbidden (or similar annotation). The discussion is open, I can make a patch.

      1. LUCENE-6420.patch
        2 kB
        Uwe Schindler
      2. LUCENE-6420-anno.patch
        82 kB
        Uwe Schindler
      3. LUCENE-6420-anno.patch
        80 kB
        Uwe Schindler
      4. LUCENE-6420-anno.patch
        74 kB
        Uwe Schindler
      5. LUCENE-6420-anno.patch
        75 kB
        Uwe Schindler

        Activity

        Hide
        Uwe Schindler added a comment -

        Patch with version update

        Show
        Uwe Schindler added a comment - Patch with version update
        Hide
        ASF subversion and git services added a comment -

        Commit 1673077 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1673077 ]

        LUCENE-6420: Update forbiddenapis to v1.8

        Show
        ASF subversion and git services added a comment - Commit 1673077 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1673077 ] LUCENE-6420 : Update forbiddenapis to v1.8
        Hide
        ASF subversion and git services added a comment -

        Commit 1673078 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1673078 ]

        Merged revision(s) 1673077 from lucene/dev/trunk:
        LUCENE-6420: Update forbiddenapis to v1.8

        Show
        ASF subversion and git services added a comment - Commit 1673078 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1673078 ] Merged revision(s) 1673077 from lucene/dev/trunk: LUCENE-6420 : Update forbiddenapis to v1.8
        Hide
        Dawid Weiss added a comment -

        Thanks Uwe, these are great changes.

        Show
        Dawid Weiss added a comment - Thanks Uwe, these are great changes.
        Hide
        Uwe Schindler added a comment -

        Simon Willnauer did include the new Annotation support of forbiddenapis 1.8 into elasticsearch: https://github.com/elastic/elasticsearch/pull/10560/files
        We can do the same in Lucene, so we have a more fine-granular exclusion pattern than the current file-level exclusions. I also like his "reason" on his annotation, so you can/have to give a reason why you apply @SuppressForbidden.
        I would suggest to add this annotation to lucene-core.jar as class level, non runtime annotation. I can work on that next week.

        Show
        Uwe Schindler added a comment - Simon Willnauer did include the new Annotation support of forbiddenapis 1.8 into elasticsearch: https://github.com/elastic/elasticsearch/pull/10560/files We can do the same in Lucene, so we have a more fine-granular exclusion pattern than the current file-level exclusions. I also like his "reason" on his annotation, so you can/have to give a reason why you apply @SuppressForbidden . I would suggest to add this annotation to lucene-core.jar as class level, non runtime annotation. I can work on that next week.
        Hide
        Hoss Man added a comment -

        uwe: should probably split this out into a distinct issue yeah?

        I think an annotation approach is a great idea ... as long as the txt file based approach is still also supported correct? – that way we won't have to introduce a lucene-core.jar dependency on stuff that doesn't already depend on it (biggest concern: solrj)

        Show
        Hoss Man added a comment - uwe: should probably split this out into a distinct issue yeah? I think an annotation approach is a great idea ... as long as the txt file based approach is still also supported correct? – that way we won't have to introduce a lucene-core.jar dependency on stuff that doesn't already depend on it (biggest concern: solrj)
        Hide
        Uwe Schindler added a comment -

        You can just place the Annotation definition into solrj's internals, too. Foibrddenapis supports stuff like forbiddenAnnotation=**.SuppressForbidden, so it accepts any annotation from any package named @SuppressAnoontation to filter. It can also be package private!

        But sure, the file-based exclude still works.

        Show
        Uwe Schindler added a comment - You can just place the Annotation definition into solrj's internals, too. Foibrddenapis supports stuff like forbiddenAnnotation=**.SuppressForbidden , so it accepts any annotation from any package named @SuppressAnoontation to filter. It can also be package private! But sure, the file-based exclude still works.
        Hide
        Hoss Man added a comment -

        ... forbiddenAnnotation=**.SuppressForbidden ...

        Ahhhhh, very cool sir. very cool.

        Show
        Hoss Man added a comment - ... forbiddenAnnotation=**.SuppressForbidden ... Ahhhhh, very cool sir. very cool.
        Hide
        Uwe Schindler added a comment -

        Patch that uses the annotation support to suppress (mostly sysout) violations.

        This also cleans up the Maven build, which got much easier now:

        • add missing signatures which were added inthe meantime (lucene.txt, solr.txt)
        • Enable to not fail on broken signatures (so we don't need to configure every module depending of its use of servlet-api/commons-io)

        Steve Rowe: Can you chekc the Maven changes?

        Show
        Uwe Schindler added a comment - Patch that uses the annotation support to suppress (mostly sysout) violations. This also cleans up the Maven build, which got much easier now: add missing signatures which were added inthe meantime (lucene.txt, solr.txt) Enable to not fail on broken signatures (so we don't need to configure every module depending of its use of servlet-api/commons-io) Steve Rowe : Can you chekc the Maven changes?
        Hide
        Steve Rowe added a comment -

        Steve Rowe: Can you chekc the Maven changes?

        Yes, I've started looking, will finish tomorrow.

        Show
        Steve Rowe added a comment - Steve Rowe: Can you chekc the Maven changes? Yes, I've started looking, will finish tomorrow.
        Hide
        Uwe Schindler added a comment -

        Path with some improvements:

        • Add Javadocs for "reason" attribute on annotation (to make Javadoc checker happy)
        • More elegant way to the DocSetPerf command line pseudo-tester.
        Show
        Uwe Schindler added a comment - Path with some improvements: Add Javadocs for "reason" attribute on annotation (to make Javadoc checker happy) More elegant way to the DocSetPerf command line pseudo-tester.
        Hide
        Uwe Schindler added a comment -

        I just wanted to mention: This patch currently still uses a file-based exclude, used for the forked commons-csv tests, which don't extend LuceneTestCase. We should fix this in a separate issue.

        Show
        Uwe Schindler added a comment - I just wanted to mention: This patch currently still uses a file-based exclude, used for the forked commons-csv tests, which don't extend LuceneTestCase. We should fix this in a separate issue.
        Hide
        Robert Muir added a comment -

        +1

        Show
        Robert Muir added a comment - +1
        Hide
        Steve Rowe added a comment -

        About the maven config:

        lucene/test-framework/pom.xml.template and solr/core/src/test/pom.xml.template aren't modified, but likely should be - I think the specializations there can be removed.

        lucene/benchmark/pom.xml.template and lucene/demo/pom.xml.template should probably have lucene.txt added to their <signaturesFiles>.

        Also, if I understand how things are setup, the new annotation suppresses all forms of forbiddenapi checking, as compared to the previous configuration, where there were multiple executions, and exceptions were targetted at a particular check (e.g. sysout), but didn't prevent other checks from running. In the maven build this represents a loss of coverage everywhere the annoatations are used, doesn't it? Not sure about the Ant build.

        Show
        Steve Rowe added a comment - About the maven config: lucene/test-framework/pom.xml.template and solr/core/src/test/pom.xml.template aren't modified, but likely should be - I think the specializations there can be removed. lucene/benchmark/pom.xml.template and lucene/demo/pom.xml.template should probably have lucene.txt added to their <signaturesFiles> . Also, if I understand how things are setup, the new annotation suppresses all forms of forbiddenapi checking, as compared to the previous configuration, where there were multiple executions, and exceptions were targetted at a particular check (e.g. sysout), but didn't prevent other checks from running. In the maven build this represents a loss of coverage everywhere the annoatations are used, doesn't it? Not sure about the Ant build.
        Hide
        Uwe Schindler added a comment -

        New patch with Steve Rowe's suggestions. Explanation follows!

        Show
        Uwe Schindler added a comment - New patch with Steve Rowe 's suggestions. Explanation follows!
        Hide
        Uwe Schindler added a comment -

        lucene/test-framework/pom.xml.template and solr/core/src/test/pom.xml.template aren't modified, but likely should be - I think the specializations there can be removed.

        They cannot be completely removed:

        • test-frameworks executes test-checks on the standard src/mainfolder (this is also special in Ant build). But the solution is much easier here: It just inherits the shared execution, but changes the goal to "check" instead of "testCheck". The config is inherited, so it executes the test configuration on the src/main folder (like in Ant)
        • solr/core/src/test/pom.xml is special, because it still excludes the imported-commons-csv tests. I simplified this by also inheriting the config, just adding the exclude. This is similar to what Ant does.

        lucene/benchmark/pom.xml.template and lucene/demo/pom.xml.template should probably have lucene.txt added to their <signaturesFiles>.

        • I solved this in a similar way by inheriting the parent configuration and just "overriding" the bundledSignatures config (without jdk-system-out).

        Also, if I understand how things are setup, the new annotation suppresses all forms of forbiddenapi checking, as compared to the previous configuration, where there were multiple executions, and exceptions were targetted at a particular check (e.g. sysout), but didn't prevent other checks from running. In the maven build this represents a loss of coverage everywhere the annoatations are used, doesn't it? Not sure about the Ant build.

        Yes and No You are right, we miss some coverage (also in Ant build), but we get more coverage on the other side, because we can exclude in a more fine-granular way (on method level). I thought about this already, one solution might be (but lets keep this for later): For the very common sysout-stuff, we can add a separate @SuppressForbiddenSysout so we can scan in 2 executions. We should discuss this in a separate issue. I did not want to add too many annotations yet.

        On the other hand, because we can now work more fine-granular, I would suggest to refactor the code a bit and move the "violations" to separate methods (like I did in the DocSetPerf tester) and only exclude those, so we don't have to exclude the whole method. For the command line tools, we might add a private method to the class containing the main method called "printout(String)" that is suppressed.

        Show
        Uwe Schindler added a comment - lucene/test-framework/pom.xml.template and solr/core/src/test/pom.xml.template aren't modified, but likely should be - I think the specializations there can be removed. They cannot be completely removed: test-frameworks executes test-checks on the standard src/mainfolder (this is also special in Ant build). But the solution is much easier here: It just inherits the shared execution, but changes the goal to "check" instead of "testCheck". The config is inherited, so it executes the test configuration on the src/main folder (like in Ant) solr/core/src/test/pom.xml is special, because it still excludes the imported-commons-csv tests. I simplified this by also inheriting the config, just adding the exclude. This is similar to what Ant does. lucene/benchmark/pom.xml.template and lucene/demo/pom.xml.template should probably have lucene.txt added to their <signaturesFiles>. I solved this in a similar way by inheriting the parent configuration and just "overriding" the bundledSignatures config (without jdk-system-out). Also, if I understand how things are setup, the new annotation suppresses all forms of forbiddenapi checking, as compared to the previous configuration, where there were multiple executions, and exceptions were targetted at a particular check (e.g. sysout), but didn't prevent other checks from running. In the maven build this represents a loss of coverage everywhere the annoatations are used, doesn't it? Not sure about the Ant build. Yes and No You are right, we miss some coverage (also in Ant build), but we get more coverage on the other side, because we can exclude in a more fine-granular way (on method level). I thought about this already, one solution might be (but lets keep this for later): For the very common sysout-stuff, we can add a separate @SuppressForbiddenSysout so we can scan in 2 executions. We should discuss this in a separate issue. I did not want to add too many annotations yet. On the other hand, because we can now work more fine-granular, I would suggest to refactor the code a bit and move the "violations" to separate methods (like I did in the DocSetPerf tester) and only exclude those, so we don't have to exclude the whole method. For the command line tools, we might add a private method to the class containing the main method called "printout(String)" that is suppressed.
        Hide
        Uwe Schindler added a comment -

        More improvements and simplifications:

        • Created a separate execution for sysout checks (like in lucene). I also compared what happens in Lucene and Solr it is now identical
        • I just suppressed the sysout checks in demo and benchmark
        • in lucene and solr test-framework it both runs the test config, but using goal "check", so it chooses right source folder.

        At the end we can now easily add a new annotation for sysout excludes.

        I think its ready. Steve Rowe if you could have a look.

        Show
        Uwe Schindler added a comment - More improvements and simplifications: Created a separate execution for sysout checks (like in lucene). I also compared what happens in Lucene and Solr it is now identical I just suppressed the sysout checks in demo and benchmark in lucene and solr test-framework it both runs the test config, but using goal "check", so it chooses right source folder. At the end we can now easily add a new annotation for sysout excludes. I think its ready. Steve Rowe if you could have a look.
        Hide
        Steve Rowe added a comment -

        I think its ready. Steve Rowe if you could have a look.

        +1, LGTM, ant clean-maven-build get-maven-poms && cd maven-build && mvn -DskipTests install succeeds.

        Thanks Uwe!

        Show
        Steve Rowe added a comment - I think its ready. Steve Rowe if you could have a look. +1, LGTM, ant clean-maven-build get-maven-poms && cd maven-build && mvn -DskipTests install succeeds. Thanks Uwe!
        Hide
        Uwe Schindler added a comment -

        OK, I will commit this in a minute. While sitting in the train to JAX2015, I ran "ant run-maven-build -DskipTests=true", works (this does basically the same as your command line).

        I analyzed the build log and checked the output of forbidden and I was happy, too.

        Show
        Uwe Schindler added a comment - OK, I will commit this in a minute. While sitting in the train to JAX2015, I ran "ant run-maven-build -DskipTests=true", works (this does basically the same as your command line). I analyzed the build log and checked the output of forbidden and I was happy, too.
        Hide
        ASF subversion and git services added a comment -

        Commit 1674926 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1674926 ]

        LUCENE-6420: Use forbidden-apis annotation @SuppressForbidden; cleanup maven build

        Show
        ASF subversion and git services added a comment - Commit 1674926 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1674926 ] LUCENE-6420 : Use forbidden-apis annotation @SuppressForbidden; cleanup maven build
        Hide
        ASF subversion and git services added a comment -

        Commit 1674931 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1674931 ]

        Merged revision(s) 1674926 from lucene/dev/trunk:
        LUCENE-6420: Use forbidden-apis annotation @SuppressForbidden; cleanup maven build

        Show
        ASF subversion and git services added a comment - Commit 1674931 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1674931 ] Merged revision(s) 1674926 from lucene/dev/trunk: LUCENE-6420 : Use forbidden-apis annotation @SuppressForbidden; cleanup maven build
        Hide
        Uwe Schindler added a comment -

        Thanks Steve!

        Show
        Uwe Schindler added a comment - Thanks Steve!
        Hide
        ASF subversion and git services added a comment -

        Commit 1674939 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1674939 ]

        LUCENE-6420: Add missing suppressAnnotation declaration on forbiddenapis ANT task in test-frameworks

        Show
        ASF subversion and git services added a comment - Commit 1674939 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1674939 ] LUCENE-6420 : Add missing suppressAnnotation declaration on forbiddenapis ANT task in test-frameworks
        Hide
        ASF subversion and git services added a comment -

        Commit 1674941 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1674941 ]

        Merged revision(s) 1674939 from lucene/dev/trunk:
        LUCENE-6420: Add missing suppressAnnotation declaration on forbiddenapis ANT task in test-frameworks

        Show
        ASF subversion and git services added a comment - Commit 1674941 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1674941 ] Merged revision(s) 1674939 from lucene/dev/trunk: LUCENE-6420 : Add missing suppressAnnotation declaration on forbiddenapis ANT task in test-frameworks
        Hide
        Anshum Gupta added a comment -

        Bulk close for 5.2.0.

        Show
        Anshum Gupta added a comment - Bulk close for 5.2.0.

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Uwe Schindler
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development