Thanks for reporting the problem and share your thoughts Michael! Kasia suggested a fix <https://github.com/markflyhigh/beam/pull/5> to my dev branch. My roll-forward is out in https://github.com/apache/beam/pull/8067, which contains a fix that excludes any possible build/target/dist in sub-directory when copy SDK source code. (see line 1619 <https://github.com/apache/beam/pull/8067/files#diff-23833058cbf2c1172b90e7764032aa59R1619> )
As for testing, I should have been more careful since it's a fundamental change that essentially affects all Python builds/tests. And I agree with Michael about Gradle task vs subproject. In my own experience in Python SDK, adding a test suite generally just need to create a new Gradle task since most of the testing steps are either provided or done by executing command line. Mark On Thu, Mar 14, 2019 at 5:59 PM Valentyn Tymofieiev <[email protected]> wrote: > 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 >>>>>>> >>>>>>> >>>>>>> >>>>>>
