• 5.0.0-beta2

      Rather than holding parsers within warnings-plugin, move them to an external library, so that the same parser can be used for multiple plugins.

      There is https://github.com/tomasbjerre/violations-lib that already duplicates some of the parsers (plus might add a few extra). Would you consider merging the parser logic into that library (and moving it into jenkinsci-organization in Github)?

      See https://github.com/jenkinsci/violations-plugin/issues/88#issuecomment-266990121. I would assume some sort of a merge would be nice here.

          [JENKINS-40439] Move parsers into external library

          Tuukka Mustonen created issue -
          Ulli Hafner made changes -
          Link New: This issue duplicates JENKINS-17434 [ JENKINS-17434 ]

          Ulli Hafner added a comment - - edited

          That would make sense, yes. Before we can do this step, we need to define a new API for this plugin (or library).

          Questions to address:

          1. Does the library depend on Jenkins core? Or is it a plain Java library? Currently the parsers implement ExtensionPoint... Also the serialisation is based on Jenkins XStream. Additionally, the library must be part of a plug-in to get the class loading done.
          2. How does the object graph of the result look like? I would like to change the current structure of Warning, ParserResult, etc.
          3. Is there a way to obtain only the number of warnings rather than the actual warnings?

          PS for a bachelor thesis of one of my students I created a prototype module to see what is actually required: https://github.com/jenkinsci/analysis-model

          Ulli Hafner added a comment - - edited That would make sense, yes. Before we can do this step, we need to define a new API for this plugin (or library). Questions to address: Does the library depend on Jenkins core? Or is it a plain Java library? Currently the parsers implement ExtensionPoint... Also the serialisation is based on Jenkins XStream. Additionally, the library must be part of a plug-in to get the class loading done. How does the object graph of the result look like? I would like to change the current structure of Warning, ParserResult, etc. Is there a way to obtain only the number of warnings rather than the actual warnings? PS for a bachelor thesis of one of my students I created a prototype module to see what is actually required: https://github.com/jenkinsci/analysis-model

          Tomas Bjerre added a comment - - edited

          1. No. Just SLF4J https://github.com/tomasbjerre/violations-lib/blob/master/build.gradle#L43 .
          2. Its a list of Violations https://github.com/tomasbjerre/violations-lib/blob/master/src/main/java/se/bjurr/violations/lib/model/Violation.java
          3. No.

          When it comes to defining an API I am pretty much happy with what I have. But I am open for suggestions!

          I took a quick look at analysis-model. The amount of dependencies are a show stopper for me. I would rather spend time on making the parsing algorithms use only functionality from the standard library (1.7).

          Tomas Bjerre added a comment - - edited 1. No. Just SLF4J https://github.com/tomasbjerre/violations-lib/blob/master/build.gradle#L43 . 2. Its a list of Violations https://github.com/tomasbjerre/violations-lib/blob/master/src/main/java/se/bjurr/violations/lib/model/Violation.java 3. No. When it comes to defining an API I am pretty much happy with what I have. But I am open for suggestions! I took a quick look at analysis-model. The amount of dependencies are a show stopper for me. I would rather spend time on making the parsing algorithms use only functionality from the standard library (1.7).

          Ulli Hafner added a comment -

          1a. No Jenkins dependencies means a lot of duplication: a wrapper for each violation model class in analysis-core (otherwise serialization/classloading will not work: maybe this works when exporting the violation-lib classes in analysis-core). Reimplementation of extension point pattern (using a registry), etc. It is possible going this way but this will require a lot of extra work (and testing). The question is what you loose if we have a dependency to Jenkins.
          1b. No other dependencies means a lot of rework: several parsers require different libraries. What is the reason behind limiting the dependencies to just logging (why is this a show stopper)? Why Java 7 and not 8?
          2. Currently this is almost the same in analysis-core (plus a lot of additional utility classes). This is one thing I would change if I could, this model is quite limited when used in conjunction with DB backend, UI filtering and paging.
          3. This is something requested by some users, would be nice if it could be integrated somehow if I totally change the underlying model

          Ulli Hafner added a comment - 1a. No Jenkins dependencies means a lot of duplication: a wrapper for each violation model class in analysis-core (otherwise serialization/classloading will not work: maybe this works when exporting the violation-lib classes in analysis-core). Reimplementation of extension point pattern (using a registry), etc. It is possible going this way but this will require a lot of extra work (and testing). The question is what you loose if we have a dependency to Jenkins. 1b. No other dependencies means a lot of rework: several parsers require different libraries. What is the reason behind limiting the dependencies to just logging (why is this a show stopper)? Why Java 7 and not 8? 2. Currently this is almost the same in analysis-core (plus a lot of additional utility classes). This is one thing I would change if I could, this model is quite limited when used in conjunction with DB backend, UI filtering and paging. 3. This is something requested by some users, would be nice if it could be integrated somehow if I totally change the underlying model

          Tomas Bjerre added a comment -

          1a I use the library also in Gradle and Maven plugins. Others may use it in whatever application they want, perhaps another CI server. Having a dependency to Jenins just does not make sense for a library whose only task is to read and parse text-files.
          1b
          I prefer to make it Java 7 compatible because its still used by alot of people.
          Unnecessary libraries on the classpath will be a problem for users of the library. For example, they may already have another version of Apache commons already on the classpath. It may be there because of another dependency or because they need it themselves. They would have to exclude dependencies and sometimes it will be impossible to resolve the situation and find a version that is compatible with all libraries using it.
          I have been able to implement alot of parsers this way, the library is very clean, very easy to work with and understand for anyone familiar with Java. I'd like to keep it that way.

          2 That to me sounds like something other then reading and parsing text files. Not a problem that should be solved by the library.

          Tomas Bjerre added a comment - 1a I use the library also in Gradle and Maven plugins. Others may use it in whatever application they want, perhaps another CI server. Having a dependency to Jenins just does not make sense for a library whose only task is to read and parse text-files. 1b I prefer to make it Java 7 compatible because its still used by alot of people. Unnecessary libraries on the classpath will be a problem for users of the library. For example, they may already have another version of Apache commons already on the classpath. It may be there because of another dependency or because they need it themselves. They would have to exclude dependencies and sometimes it will be impossible to resolve the situation and find a version that is compatible with all libraries using it. I have been able to implement alot of parsers this way, the library is very clean, very easy to work with and understand for anyone familiar with Java. I'd like to keep it that way. 2 That to me sounds like something other then reading and parsing text files. Not a problem that should be solved by the library.

          I'm not author of anything here, but here's my two cents:

          1. I interpret Tomas' statement so that he's not exactly against dependency libraries (that would be weird, as violations-lib is a dependency for other projects).

          The problem of conflicting requirements versions between multiple libraries is common. I not user of NodeJS, but as I've understood it solves that by having isolated requirements for each library (and also means lots of "wasted" disk space and potentially higher memory consumption... just assuming). In other languages you typically specify minimum version instead of pinning to an exact one. Of course, newer versions may break compatibility, so it's not fool proof, but you can constraint maximum version also. Would that work as a compromise, tomasbjerre?

          I do agree that this library should not depend on jenkins libraries (analysis-core, violations, script-security). In addition to them, there's only commons-digester3 and I don't see it bad (commons and guava libraries are useful, if they cut the amount of code needed, just use them, imo). Other dependencies are test-scope only, and would probably mostly vanish if you get rid of the jenkins-related libraries.

          3. Is there a way to obtain only the number of warnings rather than the actual warnings?

          Can't you just .size() the results? You have to run the full analysis anyway. Maybe I'm missing the point here...

          Tuukka Mustonen added a comment - I'm not author of anything here, but here's my two cents: 1. I interpret Tomas' statement so that he's not exactly against dependency libraries (that would be weird, as violations-lib is a dependency for other projects). The problem of conflicting requirements versions between multiple libraries is common. I not user of NodeJS, but as I've understood it solves that by having isolated requirements for each library (and also means lots of "wasted" disk space and potentially higher memory consumption... just assuming). In other languages you typically specify minimum version instead of pinning to an exact one. Of course, newer versions may break compatibility, so it's not fool proof, but you can constraint maximum version also. Would that work as a compromise, tomasbjerre ? I do agree that this library should not depend on jenkins libraries ( analysis-core, violations, script-security ). In addition to them, there's only commons-digester3 and I don't see it bad (commons and guava libraries are useful, if they cut the amount of code needed, just use them, imo). Other dependencies are test-scope only, and would probably mostly vanish if you get rid of the jenkins-related libraries. 3. Is there a way to obtain only the number of warnings rather than the actual warnings? Can't you just .size() the results? You have to run the full analysis anyway. Maybe I'm missing the point here...

          Tomas Bjerre added a comment -

          1. Sort of yes.

          If you take Guava for example. I have been using this method alot:
          https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Objects.html#firstNonNull(T, T)
          As you can read there it says "scheduled for removal in June 2016". If the library was using that method then anyone trying to use the library in an application with latest Guava would get ClassNotFoundException if they have latest Guava on classpath, or they would have to downgrade their Guava to the older one used by the library.

          Also commons-digester3 (https://repo1.maven.org/maven2/org/apache/commons/commons-digester3/3.2/commons-digester3-3.2.pom) adds cglib, commons-beanutils, commons-logging + whatever libraries they depend on.

          A good library does not force the user of the library into using a specific version of a utility library like this. I dont see the need to use anything else but the standard library here. Show me an example and I may change my mind =)

          3. Yes, .size() would work.

          Tomas Bjerre added a comment - 1. Sort of yes. If you take Guava for example. I have been using this method alot: https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Objects.html#firstNonNull(T , T) As you can read there it says "scheduled for removal in June 2016". If the library was using that method then anyone trying to use the library in an application with latest Guava would get ClassNotFoundException if they have latest Guava on classpath, or they would have to downgrade their Guava to the older one used by the library. Also commons-digester3 ( https://repo1.maven.org/maven2/org/apache/commons/commons-digester3/3.2/commons-digester3-3.2.pom ) adds cglib, commons-beanutils, commons-logging + whatever libraries they depend on. A good library does not force the user of the library into using a specific version of a utility library like this. I dont see the need to use anything else but the standard library here. Show me an example and I may change my mind =) 3. Yes, .size() would work.

          Ulli Hafner added a comment -

          I think it is possible to remove the Jenkins dependencies, it just will require some additional work: currently the model is part of analysis-core, the parsers are in the warnings plug-in. If we move the parsers to a separate library then the model needs to be duplicated (and the extension point needs to be replaced by a registry) and after the parsing the model needs to be transformed to the Jenkins model.

          On the other hand I'm not so dogmatic on using other libraries. (BTW: if you don't like libraries why don't you use java logging and remove slf4j, too?). I think if I can use a library that reduces the amount of code (and effort) I'll use it. Also I don't care much about things that might happen in future - if it really happens that we have a library dependency problem then we still have the possibility to change it (and JENKINS-40439 is exactly such a change request, isn't it?). Moreover, there are some ways to hide dependencies - if required, e.g. I'm using the maven shade plug-in to hide some of the dependencies in my findbugs parser wrapper that are in conflict with Jenkins dependencies.

          Several parsers now use digester and I see no real requirement to reimplement all these parsers just to drop the dependency (of course these parsers could use Java 7 classes only, but why should be rewrite working code?). And this would be even more problematic for the FindBugs parser: here the FindBugs team already provides one (which in contrast to your parser also handles some edge cases), why should I implement another one? I would like to spend this effort somewhere else...

          (Just a side note to 3.: the reason for this requirement is from some users having files with thousands of warnings. Parsing these files into a List of warnings in memory is quite slow so using size() on this list will not help. But this is actually not a high priority requirement.)

          Ulli Hafner added a comment - I think it is possible to remove the Jenkins dependencies, it just will require some additional work: currently the model is part of analysis-core, the parsers are in the warnings plug-in. If we move the parsers to a separate library then the model needs to be duplicated (and the extension point needs to be replaced by a registry) and after the parsing the model needs to be transformed to the Jenkins model. On the other hand I'm not so dogmatic on using other libraries. (BTW: if you don't like libraries why don't you use java logging and remove slf4j, too?). I think if I can use a library that reduces the amount of code (and effort) I'll use it. Also I don't care much about things that might happen in future - if it really happens that we have a library dependency problem then we still have the possibility to change it (and JENKINS-40439 is exactly such a change request, isn't it?). Moreover, there are some ways to hide dependencies - if required, e.g. I'm using the maven shade plug-in to hide some of the dependencies in my findbugs parser wrapper that are in conflict with Jenkins dependencies. Several parsers now use digester and I see no real requirement to reimplement all these parsers just to drop the dependency (of course these parsers could use Java 7 classes only, but why should be rewrite working code?). And this would be even more problematic for the FindBugs parser: here the FindBugs team already provides one (which in contrast to your parser also handles some edge cases), why should I implement another one? I would like to spend this effort somewhere else... (Just a side note to 3.: the reason for this requirement is from some users having files with thousands of warnings. Parsing these files into a List of warnings in memory is quite slow so using size() on this list will not help. But this is actually not a high priority requirement.)

          Tomas Bjerre added a comment -

          "if you don't like libraries"

          I do like libraries. I dont like libraries that polutes my classpath. I dont want to force users of my library into using a specific version of some other library.

          "why don't you use java logging and remove slf4j, too?"

          I was using slf4j-api, and only the basic functionality. That is very unlikely to casue problems. But on the other hand I only do very little logging so I actually removed slf4j-api too.

          The FindBugs report is tricky, yes. If there is a findbugs-parser ready and working using digester then that, to me, is a valid argument for having digester on the classpath. And use shading to avoid classpath collisions.

          Tomas Bjerre added a comment - "if you don't like libraries" I do like libraries. I dont like libraries that polutes my classpath. I dont want to force users of my library into using a specific version of some other library. "why don't you use java logging and remove slf4j, too?" I was using slf4j-api, and only the basic functionality. That is very unlikely to casue problems. But on the other hand I only do very little logging so I actually removed slf4j-api too. The FindBugs report is tricky, yes. If there is a findbugs-parser ready and working using digester then that, to me, is a valid argument for having digester on the classpath. And use shading to avoid classpath collisions.

            drulli Ulli Hafner
            tuukkamustonen Tuukka Mustonen
            Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              Resolved: