Spotless is slightly off topic and less serious than test coverage problems, but see https://issues.apache.org/jira/browse/BEAM-4394.
On Fri, Jun 1, 2018 at 9:30 AM Scott Wegner <[email protected]> wrote: > 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 <[email protected]> > 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 <[email protected]> >> 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 <[email protected]> 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 <[email protected]> >>>> 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 <[email protected]> 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 <[email protected]> >>>>>> 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 <[email protected]> >>>>>>> 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>>> 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 < >>>>>>>>>> [email protected]> 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 < >>>>>>>>>>> [email protected]> 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 <[email protected]> >>>>>>>>>>>> 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 <[email protected]> >>>>>>>>>>>>> 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 < >>>>>>>>>>>>>> [email protected]> 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 >>>>>>>>>>>>>>> >>>>>>>>>>>>>>
