Thanks, Kasia for noticing this and suggesting a solution. I couldn't find the suggestion on Mark's PR or the rollback PR, but sounds like we have a plan. I agree with Michael that this seems like a problem that would be hard to catch in precommit since we don't build all the projects in a precommit, and one of the projects had a dependency on sdist task, which was changed.
On Thu, Mar 14, 2019 at 4:05 PM Michael Luckey <[email protected]> wrote: > Thanks Katarzyna for that explanation. > > On Thu, Mar 14, 2019 at 8:37 PM Katarzyna Kucharczyk < > [email protected]> wrote: > >> Hi, >> Michael - many thanks for letting know and Ahmet for reverting. I think I >> know which task caused the problem. >> >> A quick explanation about what's happened. Load test gradle task was >> created in load_tests module. A build/ directory which was excluded in >> 'sdist' task wasn't the same as the one created inside the module. While >> copying it this created some kind of loop. I already suggested my solution >> to Mark. >> >> @Valentyn, the load_tests (in smoke option) haven't been added to >> preCommit as not important to check in a different way than on demand (by >> phrase triggering). Anyway, this situation shows that such decision should >> be re-thought. Also, a solution here would be moving load_test task to the >> main gradle python file. I decided to keep them separately because they >> weren't designed to be part of big task such as preCommit. >> > > We should keep in mind, that creating a submodule is in fact really a > heavyweight process. Whereas adding a task should be considered pretty > lightweight.IIUC, that load test module only adds a single task, which > might be called with different args. If so, that task is already 'kept > separate' in itself. > > So if this kind of separation is the only reason - in contrast to a more > semantic 'this *is* a different subproject' we probably should restrain > from creating as such. But my understanding could be wrong here. > > Regarding the miss out with pre commit test, it is unfortunately not > enough to rely on some subset of tasks if doing such heavy lifting on the > build system. Even a full build will not reveal all problems, as gradle > only configures and of course runs what is asked for. There is probably not > much we can do about that apart from executing everything, which seems > unfeasible. > > >> >> Let me know what you think. >> >> Kasia >> >> On Thu, Mar 14, 2019 at 7:32 PM Michael Luckey <[email protected]> >> wrote: >> >>> Thanks for that revert! >>> >>> I d really love to get those proposed changes of #7675 in, as this >>> '--keep-temp' also causes some troubles during build. >>> >>> Please let me know if/how I could support here. >>> >>> michel >>> >>> On Thu, Mar 14, 2019 at 5:21 PM Ahmet Altay <[email protected]> wrote: >>> >>>> Sent https://github.com/apache/beam/pull/8059 to revert #7675. >>>> >>>> On Thu, Mar 14, 2019 at 8:10 AM Valentyn Tymofieiev < >>>> [email protected]> wrote: >>>> >>>>> This most likely caused by https://github.com/apache/beam/pull/7675. >>>>> I suggest we revert and roll-forward with a fix. We should also understand >>>>> why this didn't surface in the precommit tests. >>>>> >>>>> On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey <[email protected]> >>>>> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> while trying to get build working on my vanilla vm, I encounter >>>>>> strangely failing python sdists tasks. It keeps complaining about being >>>>>> unable to copy files. Those files are in a deeply recursive structure >>>>>> which >>>>>> led me to the assumption this might not be intended. This seems to be >>>>>> caused by merge of [1], not verified though. >>>>>> >>>>>> This also happens on our nightly snapshot build [2]. Anyone else >>>>>> having this problem? >>>>>> >>>>>> thx, >>>>>> >>>>>> michel >>>>>> >>>>>> [1] https://github.com/apache/beam/pull/7675 >>>>>> [2] https://scans.gradle.com/s/rllnbwj4lj4qc >>>>>> >>>>>> >>>>>> >>>>>
