jeez - hornets nest interesting that going to a single module makes our distribution smaller. I assume that we must have though the opposite when we started using pieces of groovy rather than "-all". at this point there shouldn't be any more weirdly licensed files in groovy given that they have been releasing under apache for as long as we have. i can't really come up with a reason not to go to "-all" on 3.4.0, so i guess i'm +1 for that.
the :install command could support classifiers because grape/ivy support it as an option. we could treat that as a separate issue i guess - new JIRA? On Sun, Sep 16, 2018 at 6:00 PM Robert Dale <[email protected]> wrote: > So, I was taking a closer look at this there's potentially a larger mess we > want to address. > > TLDR: instead of removing groovy-sql, replace all groovy deps with > groovy-all-indy. > > First, a bit of history. tp32 did actually use groovy.sql.Sql > in ConsoleImportCustomizerProvider. This class was deprecated in 3.2.4. > Later, it was removed in 3.3.0. > > With the plan being to remove groovy-sql completely in 3.4.0, I was testing > my instructions on using the :install feature to reinstall it just in case > had someone depended on it. It of course downloaded groovy-sql but it was > different than what was in lib/. It was missing '-indy'. It also > downloaded the non-indy version of groovy. I had to go learn what '-indy' > was about. For those who don't know '-indy' is a maven classifier to denote > that the jar was built with Java 1.7 'invokedymanic' support as opposed the > legacy 'call site'. The short of it is that invokedymanic is more > performant than call site. > > As I was comparing groovy lib/, I noticed that console and server already > include both groovy-2.4.15-indy.jar and groovy-2.4.15.jar. However, the > docs[1] state that only one or the other should be included. Same goes for > any 'indy' and 'non-indy' jar of the same module. Luckily, we don't have > any other conflict like that although there is a mix of other indy non-indy > modules. > > I tried again and was unable to download the indy version of groovy-sql > because the :install command does not allow a 'classifier' parameter. It > will only download the non-indy verison of modules. Even if it could > download the 'indy' version, the groovy dependencies are built such that > all 'indy' modules have a dependency on non-indy modules. > > So, there are several issues: > 1) there is included a conflicting mix of indy and non-indy builds of the > same module - groovy. This is bad and should be fixed. > 2) there is included a mix of indy and non-indy builds of different > modules. This is not necessarily bad but does mean we're using potentially > less performant pieces of groovy. > indy) groovysh, json, jsr223 > non-indy) console, swing, templates, xml > 3) Install command doesn't support classifier parameter. I'm not aware of > anyone complaining about this before so it's not necessarily an issue in > itself. However, if we were to desire that all groovy modules be indy-only, > then classifier support would be required. But then I don't know how to > prevent the non-indy dependencies of that module from being downloaded. It > seems like a mix would always occur. > > There is one simple solution interestingly enough: depend only on > groovy-all-indy. Not only does this keep all indy modules consistent and > prevent non-indy conflicts/duplicates, it actually shrinks the distribution > size by 4M. This is from not distributing the duplicate non-indy version > of groovy and because groovy-all isn't much bigger than the separate > modules we have now. The other benefit is that we can reduce groovy > dependencies down to a single module across the project. The alternative > is ugly - having a direct dependency on each module in the dependency tree > and having an excludes on each of those dependencies to prevent them from > loading the non-indy deps. > > In conclusion, instead of removing groovy-sql, I am proposing that instead > we replace all groovy deps with groovy-all-indy. > > Thoughts? > > Stephen, are the groovy scripts submitted to Gremlin Server compiled with > invokedynamic? What about Gremlin Console? I know the groovy console does > not have this enabled by default. > > 1. > > http://docs.groovy-lang.org/docs/groovy-2.4.15/html/documentation/invokedynamic-support.html#_two_jars > > Robert Dale > > > On Fri, Sep 14, 2018 at 9:32 AM Robert Dale <[email protected]> wrote: > > > No, nothing in the docs. There are some references to java.sql.Timestamp > > but only within the context of gryo. I'll just make an upgrade note that > > it was removed and if required, it can be installed through the :install > > command. > > > > Robert Dale > > > > > > On Fri, Sep 14, 2018 at 6:34 AM Stephen Mallette <[email protected]> > > wrote: > > > >> I think groovy-sql was there because there was a time when I was into > >> polyglot data transforms in Gremlin and groovy-sql enabled you to > connect > >> to JDBC data sources in a nice way. So, I included groovy-sql as a > >> convenience. I'm not against removing it, though I think we should > remove > >> it in 3.4.0 only in case someone is currently depending on it > indirectly. > >> Please try to double check that we aren't using groovy-sql in any of the > >> documentation for some odd example or something. > >> > >> On Thu, Sep 13, 2018 at 5:20 PM Robert Dale <[email protected]> wrote: > >> > >> > gremlin-driver has a direct dependency on groovy-sql. I removed it in > >> > master and all tests pass (docker/build.sh -i -t -n) so there does not > >> > appear to be any direct or indirect usage. > >> > > >> > Why is it there? Looks like it was pulled in because of > >> > https://issues.apache.org/jira/browse/TINKERPOP-713 > >> > However, groovy-sql does not appear to be a dependency of > >> groovy-console or > >> > anything groovy (except groovy-all). Wasn't then, isn't now. > >> > > >> > Any objections to removing it? > >> > > >> > -- > >> > Robert Dale > >> > > >> > > >
