Split artifact into base, ant, and maven artifacts

[pickypg]

I just wrote a Gradle variant of this plugin that does not wrap the ant task to do its work.

Within it, I provided a ForbiddenApiRunner, and ForbiddenApiRunnerBuilder to make it usable, that could be used to generically run from Ant, Maven, or Gradle (or whatever the next build tool is). From there, you could separate the Ant and Maven code into their own packages/modules. Each module could still shade in their dependencies if that's desirable, but it avoids the Maven build unnecessarily containing Ant build code.

Doing this could also greatly simplify #13. Granted, Gradle makes it a bit easier to consume dependencies via configurations, I have already gotten that working with the Gradle version.

[uschindler]

I have seen your plugin. But I don't want to split the JAR file into several modules. It is no problem to have all classes in one plugin - Maven does not hurt Ant or Command line, and adding Gradle is only adding a few .groovy scripts - also this make the already complicated build of forbidden-apis too complicated. Adding Gradle is already on the TODO list...

I see no sense in ForbiddenApiRunner, and ForbiddenApiRunnerBuilder - sorry. Why not directly use Checker from the groovy/gradle code? I can maybe add the Builder for easier usage, but the Checker constructor is already cleaned up using EnumSets instead a chain of booleans. The Checker class was already made universal useable - so I am not sure what the Runner helps you?

FYI, I have already separated the logger interface from Checker locally, which is no longer abstract, so you can use it very simple. Just needs to be committed.

[uschindler]

See #67 for the logger changes.

[uschindler]

Doing this could also greatly simplify #13. Granted, Gradle makes it a bit easier to consume dependencies via configurations, I have already gotten that working with the Gradle version.

I am not sure if this issue changes anything about this. As said before the implementation classes for several build systems are not interfering with each other.

I think #13 is more related to #65.

[pickypg]

From my perspective, the Checker class does not do everything, which is why I wrapped it with the ForbiddenApiRunner to combine all of the niceness that each build tool provides. Personally, I do not think that the Checker class should do everything in terms of putting things together anyway; it should do the checking very well, but there's no need for it to know how to do things like putting the classpath together. I also don't want every build tool to have to repeat the exact same process for doing it either, which is why I made the runner.

I'm happy to see the Logger wrapper emerge. It removes the need for my PR (#65). Once that is merged and you release 1.9, then I can switch over to that in my Gradle wrapper and remove my forked copy of the Checker class.

I think #13 is more related to #65.

I disagree, but I think that may be because I am not as familiar with Maven's plugin system. As a result, I see a normalized model, which this would force, as much more attractive to interact with than compared to making each build tool work. You could even use it from the CLI.

Specifically with respect to Gradle, their Configurations can be added dynamically and they automatically make loading things from Maven (or other accessible locations) trivial to support just like normal signature files.

dependencies {
  forbiddenApis "org.whatever:whatever:2.0.0@txt"
}

It also makes it so that you can add other files to it (e.g., .zip files) that the build tool can extract before inserting. It may be my lack of familiarity with Maven plugins, but I expect that is a lot more work there that would still need to be build tool specific.

[uschindler]

From my perspective, the Checker class does not do everything, which is why I wrapped it with the ForbiddenApiRunner to combine all of the niceness that each build tool provides.

The "each" is exactly why this is not provided with forbidden-apis. The Wrapper implements already some stuff around file system (like selcting class files from a directory and implementing includes/excludes). But this only applies to Maven builds and CLI builds. Your wrapper class is of no use for ANT based builds, because this one is much more flexible: Ant does not use the concept of a file, it has something called Resource which supplies InputStreams. The Ant plugin allows to place any set of resources in the body! So you can check a JAR file or a remote resource for violations. I already use this to check the final JAR file. You cannot do this with Maven; with Gradle you would need to unpack first. In Ant this can be done completely resource based. See the corresponding test:

forbidden-apis/src/test/antunit/TestFinalJAR.xml

Line 47 in 1020ac4

<target name="testFinalJARWithFinalJAR">

; also Classloader handling in ANT is completely different.

So in my opinion, a user of forbidden apis uses the Checker class but has to implement the file scanning (aka resource handling) on his own. This is a bit code duplication, but this may be improved in a later version by factoring out some more common parts.

I have no problem with your plugin adding the wrapper class, but to me this class looks like a big delegator that only adds like 20 lines of code for filesystem handling. In addition it also hides the ForbiddenApiException behind a IllegalStateException (which seems totally wrong, what state is illegal?). I would prefer to just implement the basic setup of classpath and filesystem in your groovy skript (the gradle Task). Samle like Maven, Ant, Cli is doing (and duplication is minimal, at least less code than the wrapper which is as large as Checker on its own).

You might know from @rmuir: I hate enterprise-like code implementing millions of interfaces and wrapping every piece of code :-) So sorry for my complaints! This is also my major complaint about "the big builder" aka Elasticsearch (which got better in 2.0: it removed a lot of useless interfaces and wrappers everywhere).

I think #13 is more related to #65.

I think I misunderstood #65. #13 is just not really related to splitting the module as proposed here. I agree #13 is harder to do with Maven (but this is more because I am still thinking about the most clean solution). Your proposal like adding a simple "org.whatever:whatever:2.0.0@txt" also works with Maven. You would just express this with XML instead of colons.

The current Checker framework would allow to use this easily. So in Maven you can resolve the artifact and pass it as file signature or InputStream/Reader. So easy to implement. I assume the same for gradle. But also this makes clear that Checker is the right class, the wrapper is just too specific to Gradle or CLI. But Ant and Maven have much more power with handling non-filesystem resources, so its abstracted in the Checker class only taking InputStreams.

[uschindler]

Hi, unrelated to the discussion about the usefulness of the wrapper class, I refactored the package structure in #69, so the 3 interfaces (CLI, Ant, Maven) are now in separate packages.

[uschindler]

Hi, for your information I started to implement the Gradle plugin in 2 simple Java classes, once it is ready for testing, maybe you can have a look? See #68.

[uschindler]

The new forbiddenapis plugin shipped inside the forbiddenapis.jar file works now. Currently it is not yet merged, but you can checkout the branch and compile it. See #68.

[uschindler]

I close this now as the gradle plugin was merged in. It just needs documentation and tests. After that I will look into releasing this as version 2.0.