I've make the SQL PR able to be merged standalone so I can get back to work. I've also opened issues to track the things I found and dumped my work in progress into a PR.
https://issues.apache.org/jira/browse/BEAM-4402 https://issues.apache.org/jira/browse/BEAM-4403 https://github.com/apache/beam/pull/5471 Andrew On Thu, May 24, 2018 at 11:54 AM Kenneth Knowles <k...@google.com> wrote: > If it is taking days of work we should definitely put it behind a flag and > do it incrementally, find a way to share the work. > > If our tests aren't running on the actual artifacts, then we don't really > have assurance that they work. That should interest just about everyone. > (though it is also status quo relative to mvn) > > Kenn > > On Thu, May 24, 2018 at 10:17 AM Andrew Pilloud <apill...@google.com> > wrote: > >> I think I'm down to 11 packages with failures, some of which might be >> precommits that don't run outside of Jenkins. Its not an issue of figuring >> them out, it is an issue of time spent doing so. >> >> On Thu, May 24, 2018 at 9:31 AM Lukasz Cwik <lc...@google.com> wrote: >> >>> Were you able to figure out how to fix them or still having problems? >>> >>> On Thu, May 24, 2018 at 9:27 AM Andrew Pilloud <apill...@google.com> >>> wrote: >>> >>>> I spent all of yesterday investigating and fixing dependency issues >>>> outside of SQL. I really regret the decision to write a test for this. >>>> Would it be acceptable for us to put testing with the output jar behind a >>>> flag like we do for failOnWarning? >>>> >>>> On Wed, May 23, 2018 at 5:21 PM Kenneth Knowles <k...@google.com> wrote: >>>> >>>>> What's the status of moving it forward? Is it a ton of work / too much >>>>> to do quickly? >>>>> >>>>> On Wed, May 23, 2018 at 9:11 AM Andrew Pilloud <apill...@google.com> >>>>> wrote: >>>>> >>>>>> To loop the list in on discussions going on in >>>>>> https://github.com/apache/beam/pull/5443: our normal tests don't run >>>>>> against the shaded jars. Gradle can run the tests against the shaded >>>>>> jars, >>>>>> but a bunch fail due to dependency issues. It's not just SQL. >>>>>> >>>>>> Andrew >>>>>> >>>>>> On Mon, May 21, 2018 at 11:35 AM Lukasz Cwik <lc...@google.com> >>>>>> wrote: >>>>>> >>>>>>> Shading requires two pieces of information: >>>>>>> 1) Which dependencies should be part of the shaded jar (controlled >>>>>>> by includes/excludes) >>>>>>> 2) How to relocate code within those dependencies (controlled by >>>>>>> relocations) >>>>>>> >>>>>>> The reason why the exclude(".*") exists is because typically it is >>>>>>> an error to produce a shaded package with dependencies which are not >>>>>>> relocated. When libraries do this, it causes a lot of >>>>>>> NoClassFound/NoMethodFound errors for users since a user can't know >>>>>>> which >>>>>>> version of a dependency they are actually getting (the one that was >>>>>>> bundled >>>>>>> part of your jar or the one they depend on as a library). Only >>>>>>> applications >>>>>>> should ever really do this, libraries should always repackage all their >>>>>>> code to prevent such errors. >>>>>>> >>>>>>> Note that in the SQL package, you can provide your own shadowClosure >>>>>>> to the applyJavaNature() which means that the default won't apply. For >>>>>>> example: >>>>>>> https://github.com/apache/beam/blob/a3ba6a0e8de3ae72b8fc6fc6038eb9dc725f092e/sdks/java/harness/build.gradle#L20 >>>>>>> and remove the 'DEFAULT_SHADOW_CLOSURE <<' >>>>>>> >>>>>>> >>>>>>> On Mon, May 21, 2018 at 10:26 AM Andrew Pilloud <apill...@google.com> >>>>>>> wrote: >>>>>>> >>>>>>>> The issue SQL is seeing is caused by a default dependency of >>>>>>>> exclude(".*") added in build_rules.gradle. This breaks the normal >>>>>>>> method of >>>>>>>> building shadow jars as everything must be explicitly included. SQL >>>>>>>> explicitly added calcite to the jar, but not calcite's dependencies. >>>>>>>> I've >>>>>>>> been told this is the desired behavior as we want to ensure everything >>>>>>>> included is relocated. >>>>>>>> >>>>>>>> I don't know much about gradle, but this seems fragile. Is it >>>>>>>> possible to have all dependencies automatically relocated so we don't >>>>>>>> need >>>>>>>> the exclude(".*") rule? >>>>>>>> >>>>>>>> Andrew >>>>>>>> >>>>>>>> On Thu, May 17, 2018 at 7:41 PM Andrew Pilloud <apill...@google.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Yep, I added the issue as a blocker. >>>>>>>>> https://issues.apache.org/jira/projects/BEAM/issues/BEAM-4357 >>>>>>>>> >>>>>>>>> On Thu, May 17, 2018, 6:05 PM Kenneth Knowles <k...@google.com> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> This sounds like a release blocker. Can you add it to the list? >>>>>>>>>> (Assign fix version on jira) >>>>>>>>>> >>>>>>>>>> Kenn >>>>>>>>>> >>>>>>>>>> On Thu, May 17, 2018, 17:30 Lukasz Cwik <lc...@google.com> wrote: >>>>>>>>>> >>>>>>>>>>> Typically we have a test block which uses a configuration that >>>>>>>>>>> has the shadow/shadowTest configurations on the classpath instead >>>>>>>>>>> of the >>>>>>>>>>> compile/testCompile configurations. The most common examples are >>>>>>>>>>> validates >>>>>>>>>>> runner/integration tests for example: >>>>>>>>>>> >>>>>>>>>>> https://github.com/apache/beam/blob/0c5ebc449554a02cae5e4fd01afb07ecdb0bbaea/runners/direct-java/build.gradle#L84 >>>>>>>>>>> >>>>>>>>>>> On Thu, May 17, 2018 at 3:59 PM Andrew Pilloud < >>>>>>>>>>> apill...@google.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> I decided to try our new JDBC support with sqlline and >>>>>>>>>>>> discovered that our SQL shaded jar is completely broken. As >>>>>>>>>>>> in java.lang.NoClassDefFoundError all over the place. How are we >>>>>>>>>>>> testing >>>>>>>>>>>> the output jars from other beam packages? Is there an example I >>>>>>>>>>>> can follow >>>>>>>>>>>> to make our integration tests run against the release artifacts? >>>>>>>>>>>> >>>>>>>>>>>> Andrew >>>>>>>>>>>> >>>>>>>>>>>
