IRC logs for #buildstream for Monday, 2018-04-09

*** tristan has joined #buildstream04:03
*** dominic has joined #buildstream06:54
juergbitristan: gitlab is acting up again, so commenting here. mcatanzaro mentioned he is using 1.1.2, so I assume it can be closed07:06
tristanjuergbi, aha... but we have bigger problems...07:43
tristanjuergbi, where did mcatanzaro say that ?07:44
tristanhere in channel maybe ?07:44
tristanI am unhappy with the state of things today, anyway :'(07:45
tristanIt all started, when I tried to reproduce his problem... went ahead with the most straight forward use case07:45
tristanopen a workspace for glib-networking.bst... build glib-networking.bst07:46
juergbitristan: it's in the issue description07:46
juergbi>  bst 1.1.2 crashes07:46
tristanGo ahead and modify a file in glib-networking checkout07:46
*** toscalix has joined #buildstream07:46
tristanAnd build the workspace again, let's see if there's going to be a crash right ?07:46
tristanBut worse than that07:46
tristanIt just happily reports "cached"07:46
tristanSo, surely this most basic case must be covered right ? no... checked tests/frontend/workspace.py...07:47
juergbioh :/07:47
tristanit only has one test, which adds/removes files07:47
juergbiI thought we did have coverage for such basic workspace build tests07:47
tristanI added a new test now, which has 3 different paths; one add, one remove, one modify07:48
tristanmodify fails07:48
tristanSo then... I thought lets fix it...07:48
*** toscalix has quit IRC07:49
tristanWorkspaces are element wide now... but... we have tons of spaghetti bloat which delegates calls from Element -> Source, which swings back to the Element-wide workspace07:49
*** toscalix has joined #buildstream07:49
tristanSo I guess, when calculating a cache key for a workspaced element with more than one source, we just blindly checksum the whole directory recursively... *twice*07:49
tristanjuergbi, I'm in the middle of factoring out all the junk from Source07:49
tristanthat should clear the muddy waters a bit, and help me get to the bottom of solving the failing test case07:50
tristanSo far I now have Element._update_source_state() which does the additional workspace stuff, and I've removed the __track state from Source to replace with more consistent __tracking_scheduled / __tracking_done in Element07:51
tristanAt the end of it, there will no longer be any __workspace pointer in Source07:51
* tristan is doing operation by operation in separate commits07:51
tristanone commit is: Removing pointless Element._workspaced(), since we have Element._get_workspace()07:52
tristanCan you believe that was somehow there ? Yay lets ask every source if we gave it a workspace in a loop, to find out if the element is "workspaced" !07:53
tristanMan, what a morning07:53
juergbiwell, leftovers from per-source workspaces are not too surprising07:53
juergbiI'm more surprised that we didn't have a test case for this07:54
tristanI am disappointed in both07:54
tristanThe leftovers left behind when workspaces were made to be per-element, should really never have happened, and it appears that the situation has just worsened since then07:56
tristaninstead of setting things straight, people "follow whats there" and add growth to existing warts07:56
tristanit's not the right way :-S07:56
juergbiit would obviously be better if such things never happened, however, the larger a project the more likely such things happen. we just have to make sure to fix it as soon as we notice it08:00
gitlab-br-botbuildstream: issue #347 ("AssertionError when caching artifact") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/34708:26
*** noisecell has joined #buildstream08:36
tristanUmmm, tlater... is there any reason that you decided that we dont use the mounting codepath for sources if they cannot be "built incrementally" ?08:40
tristanhttps://gitlab.com/BuildStream/buildstream/blob/master/buildstream/element.py#L154708:41
tristanI think that we still want to mount the workspace into the sandbox, even if the artifacts cannot be "diffed" right ?08:41
tristanOr we explicitly want to disallow the build from modifying the workspace here ?08:42
tlatertristan: I don't quite remember the context, I think this was part of that one bug where we had to ensure that stuff like `make` doesn't recompile everything?08:42
tristan989bb3fe000dc8c843b7a8293f9cf0a3293b8d3908:42
tristanelement.py: Disallow incremental builds for caches that can't diff08:42
tlaterAh, right08:43
tristanI just dont understand exactly why08:43
tristanI guess I'll leave this part in place whilst removing the silly ridiculous loops around there08:43
tlaterI thought mounting the workspace into a sandbox was equivalent to making it build incrementally?08:43
tlaterI might have misunderstood that, though08:44
tristanWell, it's *part* of it08:44
tristanBut what was the motivation to disable that when the artifact cache could not assist in the diffs, is rather my question08:44
tlatertristan: It was this bug: https://gitlab.com/BuildStream/buildstream/issues/21508:45
tlaterIt's a bit hard to wrap my head around this again right now, for some reason08:46
juergbitristan: if we can't track changes in dependencies, we can't allow incremental builds08:46
juergbidue to incorrect timestamp08:46
tlaterjuergbi: I'm trying to remember why the incorrect time stamp means that we *can't* allow incremental builds08:47
tlaterWhat would be incorrect about the build?08:47
juergbiupdated header file will still have the same timestamp08:47
juergbiso make won't do anything08:47
tlaterAh, right08:47
tlaterAnd that would cause the updated dependency to not have an effect on the resulting artifact08:47
tlaterWhich is a big no-no because it would be a broken artifact08:47
juergbicorrect08:47
tristanjuergbi, Right so... whether we can do them incrementally or not, I dont think is related to whether we want a workspace to be *mounted* into the sandbox rather than recursively copied08:48
tristanright ?08:48
tristanthat's rather my angle08:48
tristanit wont be incremental anyway08:48
juergbitristan: if we mount it read-write, we'd have to be able to clean it08:48
juergbiwe can't force non-incremental build08:48
tristanHmmm08:48
juergbias I've mentioned before, long term I want to keep clean workspace dir and build dir completely separate08:49
tristanOk anyway, I'll treat this separately, although I believe I'm steering directly towards it08:49
tristanbecause of the bug, I will probably have to discover the mystery of how we determine the cache key of the workspace; which has probably changed08:50
juergbithat hasn't changed that recently, iirc08:50
juergbiwe added the two stage approach when introducing incremental workspace builds but you're already aware of this08:50
*** jonathanmaw has joined #buildstream08:51
tristanYes, but it's broken so something has changed :-/08:52
tristanjuergbi, I can only surmise it has to do with how we have recorded the latest successful build in the workspace metadata08:52
juergbipossibly, yes08:53
juergbimaybe git bisect would help together with the new test case?08:53
tristanbut I have to finish cleaning the lens here to see it more clearly08:53
tristanit could, but I'm almost finished completing the work of "Making workspaces element wide instead of source specific"08:54
tristanSo I'll finish that first and then probably debugging it will be easy08:54
gitlab-br-botbuildstream: merge request (135-expire-artifacts-in-local-cache->master: WIP: Resolve "Expire artifacts in local cache") #347 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/34708:58
gitlab-br-botbuildstream: merge request (135-expire-artifacts-in-local-cache->master: WIP: Resolve "Expire artifacts in local cache") #347 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/34709:11
*** tiago has joined #buildstream09:15
gitlab-br-botbuildstream: merge request (135-expire-artifacts-in-local-cache->master: WIP: Resolve "Expire artifacts in local cache") #347 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/34709:18
tristantlater, f761140f18a7d54caf6e6dba8a722b9ff1f4430e left Element._set_source_workspaces() deadcode behind, btw09:20
tlaterThat wasn't called anymore? Frankly, I just assumed that we needed to call into sources still for some reason.09:21
tlaterThat was probably the time to refactor this mess, I should have gone a bit deeper.09:21
gitlab-br-botbuildstream: merge request (jmac/performance-ci->master: Add benchmark to CI test and document CI.) #341 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/34109:22
tristantlater, it was a mess before you arrived, I mean; you changed it so that Workspace.init() calls source._set_workspace() directly, bypassing Element._set_source_workspaces(), without removing it :)09:23
tlaterAh o\. Yeah, that's a little stupid.09:23
tristanI will make that cache the workspace on Element, and make Element._get_workspace() use a cached value, instead of doing the lookup it currently still does09:24
tristantlater, it's amazing how much of a mess this has become :-S09:24
* tristan almost finished removing __workspace pointer from Source09:24
tristanwould be good if the linter could help us find private methods which are not reachable by public ones, at least09:30
* tristan understands that it's difficult to find deadcode with a linter, but at least private methods should be doable09:31
tlatertristan: We disabled a bunch of the private/public helpers because we follow a different-than-usual convention09:31
tlaterNot sure it could find that kind of deadcode in python, though.09:32
tristantlater, we are now *getting* close to the pep8 standards, I think the only exception really will be Sandbox()09:33
tristanwell, even then09:33
tristanprobably we implicitly follow the pep8 standard for underscores, after we finished the overhaul09:34
tristanUmmm09:36
tlaterOh, that would be neat. From my understanding the public/private API marking we do isn't as usual, but I haven't looked closely at what pep8 says about it.09:37
tristanSo, can anyone tell me why we have this check in source.py: https://gitlab.com/BuildStream/buildstream/blob/master/buildstream/source.py#L615 ?09:37
tristantlater, maybe you know about that "invalidate the current workspace key when setting a new workspace" thing ?09:37
tristanI am moving that to Element, and I am going to turn it into "assert self.__workspace is None"09:38
tlaterHm09:38
tlaterIt might be that this is "setting" it in memory, rather than creating it09:38
tristanit is just caching the workspace close to the source (not the element)09:39
tristanWhat I dont understand is, how can the same element ever have more than one workspace in a given session ?09:39
tlaterI think there's the workspace reset command that kind of requires this sort of thing09:39
tristanUmmm09:39
tristanthat will first close and then open the workspace, though.09:40
tristanI'll see what happens09:40
tlaterYeah, that will require changing what the source thinks is the current workspace09:40
* tlater isn't sure he introduced workspace keys though, so not entirely certain this is required09:41
*** aday has joined #buildstream09:43
tristanif the workspace reset test fails, then I can't put the assert and have to treat it differently09:44
tristanbut I think in *any* case, it's unset, and set; separately; it is never *changed*09:45
tlaterCertainly not the end of the world to poke it ;)09:45
tristanNow the question of cache keys, I guess I'll be ugly and make cache key calculation compatible09:45
tristantlater, not the end of the world, but misleading to the reader09:46
* tlater was referring to seeing if an assert breaks it09:47
tristanit invites speculation that weird things might happen during the life cycle of an element, whereas an assert reads "This cannot happen"09:47
tristanAh09:47
gitlab-br-botbuildstream: merge request (jmac/artifact_cache_error_message->master: Artifact cache: Mention the remote URL when we fail to fetch remote refs) #354 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/35409:49
tristanhmmm, it does raise the assert; maybe I'll keep the hash lookup instead09:50
gitlab-br-botbuildstream: merge request (tristan/element-wide-workspaces->master: Untangle sphaghetti workspace element/source mess) #368 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/36810:11
gitlab-br-botbuildstream: merge request (tristan/element-wide-workspaces->master: Untangle sphaghetti workspace element/source mess) #368 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/36810:12
tristanjuergbi, if you want to proof read the above ^^^^^ feel free10:14
jmacCould I have a review or merge of !341 & !354 please?10:20
gitlab-br-botbuildstream: merge request (jmac/artifact_cache_error_message->master: Artifact cache: Mention the remote URL when we fail to fetch remote refs) #354 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/35410:25
gitlab-br-botbuildstream: merge request (tristan/element-wide-workspaces->master: Untangle sphaghetti workspace element/source mess) #368 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/36810:25
*** dominic has quit IRC10:36
*** dominic has joined #buildstream10:36
gitlab-br-botbuildstream: merge request (tristan/element-wide-workspaces->master: Untangle sphaghetti workspace element/source mess) #368 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/36810:41
tristantlater, all; if you have any core Element/Source especially workspace related stuff... -> Rebase10:44
tristanjuergbi, I did bisect after all11:06
tristantlater, you broke it in: f761140f11:06
tristanlets see how it broke...11:06
juergbioh, that was supposed to be a no-behavior-change refactoring11:07
tristanfwiw, it's this: https://bpaste.net/show/be95b73bb66411:07
juergbiiirc, I even diff'd the moved code to verify but apparently not sufficient11:07
tristanjuergbi, the above is a no-behavior-change11:08
tristanI ran face first into a wall of spaghetti standing in between me and figuring out how things broke11:08
tristanso I ate the spaghetti first11:08
tristanThe above paste is the test case showing breakage11:09
tristanthe non-strict side of the test case breaks because of the bug we fixed last week11:09
tristanthe strict side shows whether it passes or fails11:09
tristanf761140f is the commit which causes the file modifications to no longer be considered in "whether a workspace has changed"11:10
tristanAs I suspected, that happens *directly* after adding ArtifactCache.diff()11:10
tristanNot sure it's related though11:10
juergbiI'm confused11:10
juergbiyou say f761140f broke the issue but at the same time you're saying it's a no-behavior-change11:11
juergbi(broke the feature, not the issue, of course)11:12
tristanjuergbi, MR !368 which I merged above, is a no-behavior-change refactor11:14
tristanjuergbi, https://bpaste.net/show/be95b73bb664 is a test case which shows broken behavior, which I have bisected down to f761140f11:14
tristanjuergbi, clear ?11:14
juergbiyes, I misinterpreted 'the above'11:15
*** aday has quit IRC11:15
tristanit's actually very recently broken11:16
tristanWhich would explain why people havent been shouting about it, and why mcatanzaro should not be using master but should stick with the other bugs in 1.1.211:16
*** aday has joined #buildstream11:17
juergbiI don't see a significant difference in the get_workspace_key => get_key move11:17
juergbiunless it's a workspace path issue11:17
juergbii.e., it's checksumming the wrong directory. but then added files would also not work11:18
tristanI see the bug11:18
tristanjuergbi, tlater; when translating the cache key algo, it became the following:11:19
tristan   self.__key = [(relpath, unique_key(self.path)) for relpath, fullpath in filelist]11:19
tristanThat should be unique_key(fullpath) I think11:19
tristanyup, that fixes it11:21
tristanThe workspace key has turned into a list of paths plus the checksum of the base directory for each path11:21
tristaninstead of ever checksumming the files11:21
tlaterOh11:21
* tlater apologizes for blowing that up11:22
juergbioh, right11:22
tristanso not related to diffing artifacts after all11:24
jmacDo we have any means of specifying multiple mirrors for a given source?11:24
tristanjmac, read https://gitlab.com/BuildStream/buildstream/issues/328 carefully11:25
jmacRight, thanks11:26
tristanThe answer is no  (or yes if you have used separate aliases, when it gets implemented)11:26
gitlab-br-botbuildstream: merge request (tristan/fix-workspace-change-detections->master: Fix regression in workspace cache key calculation) #369 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/36911:32
jonathanmawtristan: in issue https://gitlab.com/BuildStream/buildstream/issues/328 (downloading sources from mirrors), do we want to support mirror aliases that are defined for subprojects to go more than one subproject deep?11:47
jonathanmawe.g. being able to define the aliases for foo:baz:bar:bar-git?11:47
tristanjonathanmaw, I think these are separate activities, but I certainly think we want that kind of addressing yes11:48
tristanjonathanmaw, addressing is one part; and it unlocks some features, like explicit cross junction tracking (which can be used with project.refs)11:49
tristanjonathanmaw, addressing also unlocks the mirroring details; I think it makes more sense to implement solid addressing of elements and fixing the loader to be able to deal with loading explicitly addressed targets in this way *first* before diving into other features which require it11:50
tristanjonathanmaw, however I would understand if you had incentives to implement source mirroring without cross-junction support first11:50
tristanhmmm, recession of gitlab runners ? https://gitlab.com/BuildStream/buildstream/pipelines/2019264211:51
jonathanmawtristan: okay, I'll consider it more seriously once we have a general solution for cross-junction support11:53
jonathanmawIs there an issue for that?11:54
tristannot that I know of no11:54
tristanthere might be11:54
gitlab-br-botbuildstream: merge request (tristan/fix-workspace-change-detections->master: Fix regression in workspace cache key calculation) #369 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/36911:55
*** cs_shadow has joined #buildstream12:11
jonathanmawtristan: yeah, looks like the closest we've got is only tangentially-related, where https://gitlab.com/BuildStream/buildstream/issues/331 refers to your E-mail proposal https://mail.gnome.org/archives/buildstream-list/2018-March/msg00030.html, which mentions the junction path prefix12:11
jonathanmawand that they're currently only used for display purposes, and need some minor work to support using them to address elements.12:14
tristanright12:18
tristanso it was a big day12:18
tristanjmac, I'm skeptical about merging !341 in buildstream, I dont know what it adds; I dont know why we would run the benchmarks against a single version of BuildStream and require a "same hardware" every time, seems less useful than running the benchmarks on a single machine with nothing else running at the time against multiple versions of buildstream, and not caring much about the actual numbers (only caring about the observable *differences*)12:21
tristanIf we add that to buildstream repo, as is, I dont see what we get out of it really12:21
* tristan is going to eat supper and soothe todays headache with some soju12:22
*** tristan has quit IRC12:28
jmacWe do run the benchmarks on a single machine with nothing else running12:38
jmacI don't see how you can figure out the differences without the absolute numbers12:39
jmacAlso, it would have been helpful if you mentioned this two weeks ago when I raised it12:39
jmacIt also means the benchmarks script itself is regularly tested against buildstream. We've seen one error last week which would have been caught if benchmarks were running against buildstream regularly.12:42
*** bethw has joined #buildstream13:38
paulsherwood+1 for benchmarks of absolute times13:42
paulsherwoodi think tristan has a blindspot about this, tbh... it's the same underlying problem as logs without absolute times imo13:42
*** bethw has quit IRC14:21
gitlab-br-botbuildstream: merge request (311-opening-a-workspace-with-a-cached-build->master: WIP: Resolve "Opening a workspace with a cached build") #370 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37014:57
gitlab-br-botbuildstream: merge request (311-opening-a-workspace-with-a-cached-build->master: WIP: Resolve "Opening a workspace with a cached build") #370 changed state ("closed"): https://gitlab.com/BuildStream/buildstream/merge_requests/37014:59
gitlab-br-botbuildstream: merge request (311-opening-a-workspace-with-a-cached-build->master: WIP: Resolve "Opening a workspace with a cached build") #371 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37114:59
gitlab-br-botbuildstream: merge request (issue-21_Caching_build_trees->master: WIP: Issue 21 caching build trees) #372 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37215:01
gitlab-br-botbuildstream: merge request (311-opening-a-workspace-with-a-cached-build->master: WIP: Resolve "Opening a workspace with a cached build") #371 changed state ("closed"): https://gitlab.com/BuildStream/buildstream/merge_requests/37115:02
gitlab-br-botbuildstream: merge request (issue-21_Caching_build_trees->master: WIP: Issue 21 caching build trees) #372 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37215:17
gitlab-br-botbuildstream: merge request (issue-21_Caching_build_trees->master: WIP: Issue 21 caching build trees) #372 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37215:24
*** noisecell has quit IRC15:28
gitlab-br-botbuildstream: merge request (issue-21_Caching_build_trees->master: WIP: Issue 21 caching build trees) #372 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37215:29
gitlab-br-botbuildstream: merge request (issue-21_Caching_build_trees->master: WIP: Issue 21 caching build trees) #372 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37215:40
*** bethw has joined #buildstream15:41
gitlab-br-botbuildstream: issue #348 ("[RFE] Add plugin to deploy tarball") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/34815:45
gitlab-br-botbuildstream: issue #349 ("[RFE] Add plugin to generate docker (OCI) images") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/34915:45
gitlab-br-botbuildstream: issue #350 ("[RFE] Add plugin to generate qemu images") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/35015:47
gitlab-br-botbuildstream: issue #351 ("[RFE] Add plugin to deploy to a ostree repo (local and/or remote)") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/35115:58
*** dominic has quit IRC16:16
*** toscalix has quit IRC16:36
*** bethw has quit IRC16:50
*** bethw has joined #buildstream16:51
*** bethw has quit IRC16:56
*** bethw has joined #buildstream16:56
*** jonathanmaw has quit IRC17:24
juergbitlater: fyi, I wouldn't mind the humanfriendly dependency. My point is more that we need a clearly defined file format, and that aspect may actually be more difficult with humanfriendly17:55
*** bethw has quit IRC18:20
*** valentind has joined #buildstream18:57
*** valentind_ has joined #buildstream19:19
*** valentind has quit IRC19:20
*** bethw has joined #buildstream19:21
gitlab-br-botbuildstream: merge request (jjardon/update_depencies->master: docs: Update list of dependecies) #373 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37319:59
gitlab-br-botbuildstream: merge request (jjardon/update_depencies->master: docs: Update list of dependecies) #373 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37320:00
*** tristan has joined #buildstream20:18
*** xjuan has joined #buildstream20:24
*** bethw has quit IRC20:53
*** aday has quit IRC20:55
*** xjuan has quit IRC21:21
*** bethw has joined #buildstream21:38
gitlab-br-botbuildstream: merge request (reset-all-workspaces->master: Add option to reset multiple workspaces) #374 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37422:33
gitlab-br-botbuildstream: merge request (reset-all-workspaces->master: WIP: Add option to reset multiple workspaces) #374 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37422:49
*** bethw has quit IRC22:58
gitlab-br-botbuildstream: merge request (reset-all-workspaces->master: WIP: Add option to reset multiple workspaces) #374 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37422:58
gitlab-br-botbuildstream: merge request (reset-all-workspaces->master: Add option to reset multiple workspaces) #374 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37423:28
gitlab-br-botbuildstream: merge request (reset-all-workspaces->master: Add option to reset multiple workspaces) #374 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/37423:37

Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!