#development
- r
rough-minister-58256
03/22/2017, 3:52 AMtheoretically, that test is supposed to test cycle detection in
tho. we should probably get someBuildGraph
specific cycle detection tests in place, I suppose - but maybe that would obviate the need for the same tests inGraphBuildGraph - f
full-kilobyte-93429
03/22/2017, 3:53 AMit's an integration test - r
rough-minister-58256
03/22/2017, 3:53 AMI guess cycle detection is necessary in both graphs - r
rough-minister-58256
03/22/2017, 3:54 AMsince BG is theoretically still mutable etc - r
rough-minister-58256
03/22/2017, 3:54 AMpost-scheduler - f
fancy-queen-20734
03/22/2017, 5:13 PM@witty-crayon-22786 is the effect of the shared future bug that the engine hangs? I got my patch working, but it’s pretty panicky. I’m seeing an issue where sometimes pants hangs. I’m not sure if it’s my changes or not yet though. - r
rough-minister-58256
03/22/2017, 5:18 PM@fancy-queen-20734 did you rebase to latest master? there was a revert yesterday related to hangs. - f
fancy-queen-20734
03/22/2017, 5:19 PMah, I probably haven’t picked that up yet - r
rough-minister-58256
03/22/2017, 5:21 PMif you have a reliable repro / deadlocked process, it might be good to grab a stacktrace if you can get it (iirc,
)lldb -b -o “thread backtrace all” -o “detach” -p <PID> - r
rough-minister-58256
03/22/2017, 5:23 PMI think Stu was having a hard time reproducing locally - f
full-kilobyte-93429
03/22/2017, 5:25 PMdidn't look closely at the error message last night, turns out we still detect cycles, it's a different error message now - f
full-kilobyte-93429
03/22/2017, 5:26 PMCopy code- self.assertIn('contained a cycle', pants_run.stderr_data) + self.assertIn('Cycle detected', pants_run.stderr_data) - r
rough-minister-58256
03/22/2017, 5:26 PMright - now it comes from BuildGraph - f
full-kilobyte-93429
03/22/2017, 5:26 PMwas that not early enough? - r
rough-minister-58256
03/22/2017, 5:27 PMbut when we had cycle detection earlier in the engine, it was cycle detecting in a different mode w/ different output earlier in the integration test run - causing us to have to modify that particular integration test accordingly - r
rough-minister-58256
03/22/2017, 5:28 PMthis was what I was alluding to yesterday - if we have earlier cycle detection, that integration test is basically obsolete - r
rough-minister-58256
03/22/2017, 5:28 PMor at least should be generalized to not be BuildGraph specific - r
rough-minister-58256
03/22/2017, 5:28 PM(mainly just a naming/namespace thing at this point) - f
full-kilobyte-93429
03/22/2017, 5:31 PMright, will rename as part of https://github.com/pantsbuild/pants/issues/4358 - f
full-kilobyte-93429
03/22/2017, 5:32 PMjust to confirm so we didn't lose cycle detection completely we just lost the early cycle detection from engine - r
rough-minister-58256
03/22/2017, 5:33 PMyeah, tho I think we ultimately want both - f
full-kilobyte-93429
03/22/2017, 5:33 PMwhy both? can kill the latter if can detect by engine - r
rough-minister-58256
03/22/2017, 5:33 PMwell, currently BuildGraph as used in tasks is still mutable - r
rough-minister-58256
03/22/2017, 5:34 PMso tasks can do things later that could introduce cycles, past the engine - f
full-kilobyte-93429
03/22/2017, 5:34 PMi see - f
full-kilobyte-93429
03/22/2017, 5:34 PMas a safety net till it's immutable - r
rough-minister-58256
03/22/2017, 5:35 PMyeah or until we kill it entirely - f
full-kilobyte-93429
03/22/2017, 5:44 PMi am going to first exploring returning the mapping of (key, value) back fromST - w
witty-crayon-22786
03/22/2017, 5:45 PMI guess it would make sense to defer the ST cycle detection ticket until we figure out what is happening with the BuildGraph API. - r
rough-minister-58256
03/22/2017, 5:46 PMaren’t there potential cycles in the
that we might never see inGraph
?BuildGraph