I think it would be easier to review and discuss these changes with smaller, more focused PR’s. Is there a way to split up these changes? That would help me a lot and I would be less concerned about introducing a problem with our POM’s / dependencies.
Thanks, Anthony > On Oct 1, 2018, at 2:04 PM, Dan Smith <dsm...@pivotal.io> wrote: > > If what we are doing is just making sure that each geode module is > explicitly declaring all of its first order dependencies (things that > directly show up in import statements) in its build.gradle, then this seems > good to me. > > I don't think we want to stop using gradle's ability to pull in all of the > transitive implementation dependencies that we don't directly use. We don't > want to get into the business of maintaining those manually. > > Removing unused dependencies and pulling in this lint plugin also seem like > good improvements. > > -Dan > > On Mon, Oct 1, 2018 at 1:44 PM Patrick Rhomberg <prhomb...@apache.org> > wrote: > >>> Patrick, you just mentioned the java-library plugin's ability to >>> distinguish between api and implementation dependencies. Geode doesn't >> use >>> that plugin yet AFAIK, nor does the PR introduce that plugin right? >> >> That is correct, we do not yet make use of the `java-library` plugin (which >> has the `implementation` and `api` configurations) and are still using the >> `java` plugin (and rely on the `compile` configurations). I brought it up >> only to mention that there will be a great deal of inter-module build >> breakages if we were to try to transition to using `implementation` while >> our actual dependencies of each module were not yet explicit. >> >> And yeah, all of this is on the road to Java9 modules. >> >> As for accepting this PR, there are still multiple issues with it as it >> stands -- I mostly opened it in its current state to spur this discussion. >> I'm was going to go open a handful of tickets so that each individual PR >> and commit are more bite-sized. >> >> On Fri, Sep 28, 2018 at 4:26 PM, Bill Burcham <bburc...@pivotal.io> wrote: >> >>> So it sounds like (per Robert) we use the gradle-lint-plugin's >>> unused-dependency rule to warn us of unused dependencies. That handles >> one >>> side of the equation (only list *necessary* dependencies). >>> >>> I haven't heard anyone mention tool support for the other side of the >>> equation: ensuring that we list the *sufficient* list of dependencies per >>> build.gradle. >>> >>> Patrick, you just mentioned the java-library plugin's ability to >>> distinguish between api and implementation dependencies. Geode doesn't >> use >>> that plugin yet AFAIK, nor does the PR introduce that plugin right? >>> >>> All this seems of a piece with the march toward modularity, exemplified >> by >>> Java 9 modules. Patrick, is your vision something like: >>> >>> 1. accept this PR >>> … >>> 2. introduce the java-library plugin and segregate dependencies into api >>> vs. implementation ones >>> … >>> N. define Java modules >>> >>> >>> On Fri, Sep 28, 2018 at 10:39 AM Patrick Rhomberg <prhomb...@apache.org> >>> wrote: >>> >>>> Bill has the heart of it, yes. >>>> >>>> I should have also mentioned that this ties into java-library plugin >>>> configuration, notably that the `compile` configuration is deprecated. >>>> For dependences that we do not wish to leak, we will need to use >>>> `implementation`. For dependencies which we intentionally elect to >> share >>>> with our consumers, we should use `api`. >>>> >>>> For modularity, it should not break one module's ability to build for >>>> another module to declare a `compile` configuration to be >>> `implementation`, >>>> unless of course that dependency is an active and necessary component >>> part >>>> of that module's consumption. In that case, it should be declared part >>> of >>>> the modules `api`. >>>> In this vein and as Bill pointed out, a module should have an accurate >>>> representation of its own build-time dependencies. >>>> >>>> In time, this will allow a much improved build experience, since >>>> `implementation` and `api` are positioned for better granularity of >>>> incremental building. >>>> >>>> On Fri, Sep 28, 2018 at 10:23 AM, Robert Houghton < >> rhough...@pivotal.io> >>>> wrote: >>>> >>>>> Hi Bill, >>>>> >>>>> We are using a Gradle plug-in to identify dependencies that are >> unused, >>>> or >>>>> are declared in the wrong module or scope. >>>>> This is called out by the Gradle documents on improving build [ >>>>> https://guides.gradle.org/performance/#avoid_unnecessary_and_unused_ >>>>> dependencies]. >>>>> The plug-in documentation is here [ >>>>> https://github.com/nebula-plugins/gradle-lint-plugin/ >>>>> wiki/Unused-Dependency-Rule >>>>> ] >>>>> >>>>> Thank you, >>>>> -Robert Houghton >>>>> >>>>> On Fri, Sep 28, 2018 at 10:18 AM Bill Burcham <bburc...@pivotal.io> >>>> wrote: >>>>> >>>>>> From the PR, Anthony, it seems to me that Patrick is proposing that >>>> each >>>>>> build.gradle be explicit about mentioning the "things" it depends >> on. >>>> For >>>>>> example: >>>>>> >>>>>> [image: image.png] >>>>>> >>>>>> Look at how geode-connectors goes from mentioning a few >> dependencies >>> to >>>>>> mentioning many more. The value of this is that it ostensibly shows >>> us >>>>> all >>>>>> the things that geode-connectors actually depends on. >>>>>> >>>>>> The challenge I see is two-fold: >>>>>> >>>>>> • as with Java imports and "unused imports", there is the risk of >>>> listing >>>>>> more dependencies than we actually need >>>>>> • there's also the risk that we still don't have a complete list of >>>>>> dependencies >>>>>> >>>>>> Patrick: do you have tool support for this? Is there a tool that >> can >>>>>> identify or even remove unused dependencies? What process do you >> use >>>> for >>>>>> finding these heretofore hidden dependencies? Did you run gradle >>>>>> app:dependencies? >>>>>> >>>>> >>>> >>> >>
