Lucene - Core
  1. Lucene - Core
  2. LUCENE-5055

rat-sources target is missing build and ivy xml files, also resources folders

    Details

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

      Description

      The check for copyright notices in files isn't checking all files, only source/test files. A couple modules xml files are missing this (join, spatial, and custom-tasks.xml).

      1. LUCENE-5055.patch
        12 kB
        Uwe Schindler
      2. LUCENE-5055.patch
        8 kB
        Uwe Schindler
      3. LUCENE-5055.patch
        6 kB
        Uwe Schindler
      4. LUCENE-5055.patch
        5 kB
        Uwe Schindler
      5. LUCENE-5055.patch
        4 kB
        Ryan Ernst

        Issue Links

          Activity

          Hide
          Ryan Ernst added a comment -

          Attached is a patch which adds all xml files in each source module to the list of files checked by rat-sources, and fixes the files that were missing the copyright notice.

          Show
          Ryan Ernst added a comment - Attached is a patch which adds all xml files in each source module to the list of files checked by rat-sources, and fixes the files that were missing the copyright notice.
          Hide
          Uwe Schindler added a comment -

          Thanks Ryan. I'll take care.

          Show
          Uwe Schindler added a comment - Thanks Ryan. I'll take care.
          Hide
          Uwe Schindler added a comment - - edited

          I improved the patch a littel bit to catch further thins (optionally for some modules):

          • tools/forbiddenApis/**
            This is done by an generally empty property ${rat.additional-includes}, which is defined by those modules. Evereything in this property is a pattern applied to the module's home dir

          I found one license problem:

          • If I add "tools/prettify/**" it complains, because all those Javascript files contain no license header! We should fix this (although they are compressed&unreadable Javascript), but generally Javascript compressors keep license headers!
          Show
          Uwe Schindler added a comment - - edited I improved the patch a littel bit to catch further thins (optionally for some modules): tools/forbiddenApis/** This is done by an generally empty property ${rat.additional-includes}, which is defined by those modules. Evereything in this property is a pattern applied to the module's home dir I found one license problem: If I add "tools/prettify/**" it complains, because all those Javascript files contain no license header! We should fix this (although they are compressed&unreadable Javascript), but generally Javascript compressors keep license headers!
          Hide
          Uwe Schindler added a comment -

          The patch by Ryan and my modified one was missing to run the checks on the files in the root directory (build.xml, extra-targets.xml). I added this to the root rat-sources target. I had to tweak the rat task a bit, because root has no src dir.

          Show
          Uwe Schindler added a comment - The patch by Ryan and my modified one was missing to run the checks on the files in the root directory (build.xml, extra-targets.xml). I added this to the root rat-sources target. I had to tweak the rat task a bit, because root has no src dir.
          Hide
          Ryan Ernst added a comment - - edited

          Thanks Uwe!

          With your additional-includes property, can these checks be moved to the modules which need it?

          <!-- some modules have a src/tools/[java,test] -->
          <fileset dir="src/tools/java" excludes="${rat.excludes}" erroronmissingdir="false"/>
          <fileset dir="src/tools/test" excludes="${rat.excludes}" erroronmissingdir="false"/>
          
          Show
          Ryan Ernst added a comment - - edited Thanks Uwe! With your additional-includes property, can these checks be moved to the modules which need it? <!-- some modules have a src/tools/[java,test] --> <fileset dir= "src/tools/java" excludes= "${rat.excludes}" erroronmissingdir= "false" /> <fileset dir= "src/tools/test" excludes= "${rat.excludes}" erroronmissingdir= "false" />
          Hide
          Uwe Schindler added a comment - - edited

          With your additional-includes property, can these checks be moved to the modules which need it?

          I would keep it like it is for now. By this we automatically do the checks in all modules without taking care which actually need them. This would enforce the checks.

          I misunderstood! The extra tools check! Yes we can move those checks! Will do this.

          Show
          Uwe Schindler added a comment - - edited With your additional-includes property, can these checks be moved to the modules which need it? I would keep it like it is for now. By this we automatically do the checks in all modules without taking care which actually need them. This would enforce the checks. I misunderstood! The extra tools check! Yes we can move those checks! Will do this.
          Hide
          Ryan Ernst added a comment -

          Also, what about the resources dirs under analysis modules? All the stopwords/etc files have licence comments right?

          Show
          Ryan Ernst added a comment - Also, what about the resources dirs under analysis modules? All the stopwords/etc files have licence comments right?
          Hide
          Uwe Schindler added a comment - - edited

          Yeah, but for resources we should take care overall. The problem is, we have sometimes binary files. We have resources folders also in core (the META-INF files). So the resources check should be done globally.

          Show
          Uwe Schindler added a comment - - edited Yeah, but for resources we should take care overall. The problem is, we have sometimes binary files. We have resources folders also in core (the META-INF files). So the resources check should be done globally.
          Hide
          Uwe Schindler added a comment -

          Moved the tools checks to the 3 analysis modules that use it

          Show
          Uwe Schindler added a comment - Moved the tools checks to the 3 analysis modules that use it
          Hide
          Uwe Schindler added a comment - - edited

          I added:

          <fileset dir="${src.dir}/../resources" excludes="${rat.excludes}" erroronmissingdir="no"/>
          

          (the way with ${src.dir}/../resources is identical to the copy task in javac)

          And now we have some missing Licenses on *.rslp files. It also scans the zip files inside. The pattern might need additional checks.

          Show
          Uwe Schindler added a comment - - edited I added: <fileset dir= "${src.dir}/../resources" excludes= "${rat.excludes}" erroronmissingdir= "no" /> (the way with ${src.dir}/../resources is identical to the copy task in javac) And now we have some missing Licenses on *.rslp files. It also scans the zip files inside. The pattern might need additional checks.
          Hide
          Uwe Schindler added a comment -

          Attached is a patch that refactors the resources folder handling a little bit (adds a property for it and removes hardcoded ../resources).
          For now I only enabled license checking on META-INF/** files. We should discuss what to do with other resources files (maybe only patterns like *.txt).

          Show
          Uwe Schindler added a comment - Attached is a patch that refactors the resources folder handling a little bit (adds a property for it and removes hardcoded ../resources). For now I only enabled license checking on META-INF/** files. We should discuss what to do with other resources files (maybe only patterns like *.txt).
          Hide
          Ryan Ernst added a comment -

          Well, .rspl files are text too right? It does seem like .txt would handle the vast majority of resource files that we'd want to check.

          Show
          Ryan Ernst added a comment - Well, .rspl files are text too right? It does seem like .txt would handle the vast majority of resource files that we'd want to check.
          Hide
          Uwe Schindler added a comment -

          We should first check the license of those files. I also know that most stopword files are missing licenses, too. Especially it is sometimes unknown, so "relicensing" them might be wrong. I think we should discuss with Robert Muir, too.

          Show
          Uwe Schindler added a comment - We should first check the license of those files. I also know that most stopword files are missing licenses, too. Especially it is sometimes unknown, so "relicensing" them might be wrong. I think we should discuss with Robert Muir , too.
          Hide
          Robert Muir added a comment -

          it is never really unknown. we should not just apply random license headers though. some of these files are BSD licensed etc. Its described in NOTICE.

          Show
          Robert Muir added a comment - it is never really unknown. we should not just apply random license headers though. some of these files are BSD licensed etc. Its described in NOTICE.
          Hide
          Uwe Schindler added a comment - - edited

          OK, so should we add the whole resources folder (non binary of course) to rat-sources? Currently it fails e.g. for those *.rslp files, because they have no header and look like some programming language... But no header at all. For stop words in most cases we have some "note" (license-Like) - but if its BSD, shouldn't they have a BSD header?

          The attached patch for now only check META-INF resources for License headers.

          Show
          Uwe Schindler added a comment - - edited OK, so should we add the whole resources folder (non binary of course) to rat-sources? Currently it fails e.g. for those *.rslp files, because they have no header and look like some programming language... But no header at all. For stop words in most cases we have some "note" (license-Like) - but if its BSD, shouldn't they have a BSD header? The attached patch for now only check META-INF resources for License headers.
          Hide
          Robert Muir added a comment -

          Currently it fails e.g. for those *.rslp files, because they have no header and look like some programming language... But no header at all.

          They are a description of stemming rules, not programming (and ASL2). See RSLPStemmerBase for more documentation or also http://www.inf.ufrgs.br/~viviane/rslp/index.htm for background.

          Show
          Robert Muir added a comment - Currently it fails e.g. for those *.rslp files, because they have no header and look like some programming language... But no header at all. They are a description of stemming rules, not programming (and ASL2). See RSLPStemmerBase for more documentation or also http://www.inf.ufrgs.br/~viviane/rslp/index.htm for background.
          Hide
          Uwe Schindler added a comment -

          Robert Muir: Doesn't matter to me what they are, the question was just if we can/should enable license checks in resources folders or not. To me it looke like you are against, I just wanted your opinion.

          If we decide to not check licenses in resources folder, I will commit the attached patch, which only checks META-INF, build.xml, forbidden-api signatures and cleans up the handling of resources folder in build.xml

          Show
          Uwe Schindler added a comment - Robert Muir : Doesn't matter to me what they are, the question was just if we can/should enable license checks in resources folders or not. To me it looke like you are against, I just wanted your opinion. If we decide to not check licenses in resources folder, I will commit the attached patch, which only checks META-INF, build.xml, forbidden-api signatures and cleans up the handling of resources folder in build.xml
          Hide
          Uwe Schindler added a comment -

          In addition, how should we handle the missing license header in the prettify.js files? It is clearly a bug - see above!

          Show
          Uwe Schindler added a comment - In addition, how should we handle the missing license header in the prettify.js files? It is clearly a bug - see above!
          Hide
          Robert Muir added a comment -

          I am not against: I would prefer we are more consistent and add the missing headers!

          Show
          Robert Muir added a comment - I am not against: I would prefer we are more consistent and add the missing headers!
          Hide
          Uwe Schindler added a comment -

          Robert: I will open another issue to review the licenses of all resources files + Prettify JS

          Committing the current patch.

          Show
          Uwe Schindler added a comment - Robert: I will open another issue to review the licenses of all resources files + Prettify JS Committing the current patch.
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] uschindler
          http://svn.apache.org/viewvc?view=revision&revision=1493333

          LUCENE-5055: "rat-sources" target now checks also build.xml, ivy.xml, forbidden-api signatures, and parts of resources folders.

          Show
          Commit Tag Bot added a comment - [trunk commit] uschindler http://svn.apache.org/viewvc?view=revision&revision=1493333 LUCENE-5055 : "rat-sources" target now checks also build.xml, ivy.xml, forbidden-api signatures, and parts of resources folders.
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] uschindler
          http://svn.apache.org/viewvc?view=revision&revision=1493334

          Merged revision(s) 1493333 from lucene/dev/trunk:
          LUCENE-5055: "rat-sources" target now checks also build.xml, ivy.xml, forbidden-api signatures, and parts of resources folders

          Show
          Commit Tag Bot added a comment - [branch_4x commit] uschindler http://svn.apache.org/viewvc?view=revision&revision=1493334 Merged revision(s) 1493333 from lucene/dev/trunk: LUCENE-5055 : "rat-sources" target now checks also build.xml, ivy.xml, forbidden-api signatures, and parts of resources folders
          Hide
          Uwe Schindler added a comment -

          Thanks Ryan!

          Show
          Uwe Schindler added a comment - Thanks Ryan!
          Hide
          Steve Rowe added a comment -

          Bulk close resolved 4.4 issues

          Show
          Steve Rowe added a comment - Bulk close resolved 4.4 issues

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Ryan Ernst
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development