Andrew, it sounds like you're suggesting testShadowJar and enableSpotless should also be the default for all modules? Do you have an idea of how much effort is required for migrating? For failOnWarning / ErrorProne, the migration is pretty straightforward, so I created starter bugs for each module [1] with step-by-step instructions that anybody could pick up, and so far it's been fairly successful-- only 13 out of 47 left to go. Do we have anything tracking the work for testShadowJar / spotless?
[1] https://issues.apache.org/jira/issues/?jql=labels+%3D+errorprone On Thu, May 31, 2018 at 9:20 AM Andrew Pilloud <apill...@google.com> wrote: > There is now a testShadowJar option on applyJavaNature, which allows you > to run your tests against the shaded jar. Everyone should turn it on for > their module along with failOnWarning and enableSpotless for maximum > testing. > > On Thu, May 24, 2018 at 1:24 PM Andrew Pilloud <apill...@google.com> > wrote: > >> 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 >>>>>>>>>>>>>> >>>>>>>>>>>>>
