IRC logs for #buildstream for Friday, 2017-11-03

gitlab-br-botpush on buildstream@zip (by Mathieu Bridon): 5 commits (last: plugins/sources/tar.py: Ignore possible leading '.' directory.) https://gitlab.com/BuildStream/buildstream/commit/9870257fd81299c282f909d04b88d821f90b374306:49
gitlab-br-botbuildstream: merge request (zip->master: WIP: Add a new zip source) #131 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/13106:49
gitlab-br-botpush on buildstream@zip (by Mathieu Bridon): 1 commit (last: Add a new zip source) https://gitlab.com/BuildStream/buildstream/commit/958d118a380a4785e26f4542532216c33bad408d06:57
gitlab-br-botbuildstream: merge request (zip->master: WIP: Add a new zip source) #131 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/13106:57
*** tristan has joined #buildstream07:26
gitlab-br-botbuildstream: merge request (zip->master: Add a new zip source) #131 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/13107:53
*** valentind has joined #buildstream09:24
*** jude has joined #buildstream09:39
*** jude has quit IRC09:49
*** ssam2 has joined #buildstream09:50
*** jude has joined #buildstream09:50
tristanssam2, sorry sometimes I dont notice the MRs which appear on not-buildstream adjacent projects... just merged your defs2bst patch which it looks like baserock is blocking on09:57
ssam2nice, thanks09:59
ssam2i feel like the baserock loose ends are finally getting wrapped up10:00
ssam2how would you feel about moving defs2bst into the baserock/ project ?10:00
ssam2my thinking was, then we could set up a full CI pipeline for it that ran on the Baserock CI runners10:00
tristanI'm not entirely keen on that honestly10:06
ssam2fair enough10:07
tristanssam2, My thinking is that baserock is only one of the projects which are using definitions/YBD - and that we want to maintain a migration path for at least one other project which will need one10:07
ssam2sure, well my thinking is that baserock has no real reason to keep the .morph files around any more10:08
ssam2other than testing defs2bst, but testing defs2bst should really be done as part of defs2bst10:08
tristanwell, I dont disagree with that10:08
tristanAlso there are some special cases in there that are probably baserock specific10:08
ssam2moving it to baserock was just an easy way to get access to the faster CI runners... but, there is probably another way to do that10:09
tristanBut if we're going to drop the .morph files, why is it important for baserock to have ownership of the conversion project after ?10:09
tristanright10:09
tristanI see10:09
tristanstill, it is important that we care about migration of other YBD using projects10:10
ssam2agreed10:10
ssam2looks like it's pretty easy to make the baserock-manager-runner2 runner available for defs2bst without moving it10:11
tristanssam2, I personally dont care a great deal about the perfect-ness of the CI of the conversion script, but if we wanted a great CI for it; we would want a reduced data set which exercises and tests only the conversion features10:11
ssam2hmm, ok10:11
tristani.e. there is nothing great about redundantly building a whole heavy system, one which might not exercise all of YBD's features, even10:12
ssam2true; but computer time is much cheaper than engineer time10:12
tlatertristan: So, apart from your comment about the tests, the only thing left for the multiple targets branch is dealing with --except10:12
tristanbetter would be a small as possible project which exercises each YBD feature10:12
tristanssam2, yes - which is why I lead with "I dont really care that much" :D10:13
ssam2right :-)10:13
tristanI mean, it would be great to have, but I'm sure there are more important things to work on10:13
tristantlater, no10:13
ssam2yeah. but if we do merge .bst files to baserock definitions, defs2bst will have *no* CI -- i'm trying to figure out the quickest way to ensure it still has some CI10:14
tristantlater, I want to merge that branch as an isolated thing, and the --except stuff is separate10:14
ssam2and if someone wants to polish it later, all the better10:14
tlatertristan: That's fair - then, do the tests have to be part of the frontend stuff? It's going to be quite hard to assert that elements are built in the correct order using just buildstream output.10:15
tristanssam2, If we can run it for a "minimal system" sample first... A.) There is no need to chase a moving target of that sample... B.) The tests only need to run when the defs2bst repo is modified10:15
tristantlater, `bst show` by default shows build plan order, surely there is something to do with that ?10:15
ssam2tristan, agreed10:16
tlatertristan: I'm pretty sure it shows deps(Scope.ALL)10:16
tristantlater, that can be controlled with the --deps option10:16
tristantlater, it's really quite flexible10:16
tlaterAh, alright, I overlooked that10:16
tristantlater, I think you can already understand from this experience, why it's important to stop contributing to the mess of tests which poke the innards of BuildStream10:17
tristantlater, i.e. when you made this change, thankfully you only had to modify a hand full of tests, because a ton of them are already migrated10:18
tristanbesides having to maintain a bunch of hard-to-follow code, of course10:18
tristanI'm thinking of doing a migration of the remaining old-school crappy tests in tests/sources, looks like it wont take me too long and I need to address https://gitlab.com/BuildStream/buildstream/issues/145 anyway, which means I need proper source tests for tar at least10:19
tlaterYeah, it makes sense. I'm just never sure whether to stick with the current test suite or not, but I guess from now on the policy is to always test using the frontend.10:22
tristantlater, yes, always - in fact I'm thinking its going to be worthwhile soon to move LoadErrorReason to be generalized ErrorReason which we can use for all BstErrors, so we can easily pass that back from child processes and assert that in tests10:23
tristantlater, I want to migrate almost everything else, so far I think the only thing which is certainly worth keeping around is the _yaml tests10:23
tlatertristan: Could the 'integration' tests also be migrated over at some point?10:24
tristanto pytest ?10:24
tlaterThere seems to be little reason to keep a shell script around for those anymore.10:24
tristanI have been thinking that yeah10:24
tristanBut, it's important to mark and distinguish them10:25
tristantlater, i.e. they should not run by default with `./setup.py test`10:25
tlatertristan: pytest has options to specify the test dir, which we really should use anyway.10:25
tlaterFrom there it's simple to just have two separate test commands.10:26
tristanthat's an option - and possibly we want to move some of `testutils` into buildstream core10:26
tristanso that third party plugin repositories can leverage those fixtures10:26
tristanI think this is quite not the today priority, though :D10:27
tristancertainly we agree on general direction, though10:27
*** jude has quit IRC10:27
tristanSo in other news, bochecha created a new `zip` source, which shares most of it's code with `tar`, and makes it easy to implement other "download a file" kinds of source10:28
tristanIts has good test coverage, I want to merge it10:28
tristanpushing it back to after the 1.0 release (cause technically, it's a feature addition), would seem to cause more friction than add risk in this case10:29
tristanThoughts ?10:29
paulsherwood+110:29
paulsherwood(not that my vote counts for much :-))10:30
ssam2it doesn't seem like a particularly risky feature addition, i say merge it10:30
tristanpaulsherwood, we appreciate you :D10:30
paulsherwoodheh :)10:30
tristanyeah, he also made an effort to do good tests and make it well shareable, I'm very happy with the patch10:30
tristanalright10:31
tristanwill do10:31
valentindGreat! Was waiting for it.10:39
*** valentind has quit IRC10:41
gitlab-br-botpush on buildstream@master (by Tristan Van Berkom): 3 commits (last: tests: Reuse utils.sha256sum) https://gitlab.com/BuildStream/buildstream/commit/f330024d640f340e92ea221ff26a343bf26a011510:45
gitlab-br-botbuildstream: merge request (zip->master: Add a new zip source) #131 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/13110:45
gitlab-br-botbuildstream: Mathieu Bridon deleted branch zip10:45
gitlab-br-botpush on buildstream@master (by Tristan Van Berkom): 1 commit (last: doc/source/pluginindex.rst: Added zip source to the plugins index) https://gitlab.com/BuildStream/buildstream/commit/45501e618079245e22f5f337cd0d77a939bbfeef10:51
*** givascu has joined #buildstream11:00
tlatertristan: I assume no targets should result in an error? It looks like running with an empty list makes sense in some applications, but not buildstream.11:11
tristantlater, Yeah that makes sense11:24
tristantlater, I think for `bst show` it's a bit ambiguous, but lets roll with an error for now11:25
tlatertristan: `bst show` should default to showing the whole project IMO, but yes, easier for now :)11:26
tristanMaybe one day someone will say "with my script which introspects bla bla and generates a list of elements to ask `bst show` about, I dont want an error when there is no arguments"11:26
tristanWe can deal with that, on that day I think11:26
tristanIn any case I think it's a sane default for now, error out with nothing to do11:26
*** adds68__ has quit IRC12:20
*** adds68__ has joined #buildstream12:22
cs_shadowHi @tristan. Thanks for your feedback on the mount workspaces MR. I wanted to discuss a couple of things with you about it. For elements like `import` that don't actually "run" anything in the sandbox, I wasn't sure how the mounting should work as it happens when the sandbox is run. Did you had any thoughts on that?12:32
cs_shadowI was thinking perhaps I could run some dummy commands like `true` but there's no guarantee that it would be present in the sandbox12:32
tristancs_shadow, its a source like any source12:34
tristancs_shadow, basically, when you workspace it initially, it is "staged" via the normal staging codepaths into a user controlled workspace directory12:35
tristancs_shadow, when you build anything with an active workspace, the core ensures that, instead of copying over stuff from a workspace directory (current stuff), it is mounted instead12:35
tristanahhhh12:36
tristanI see what you mean, this is a little annoying isnt it12:36
cs_shadowyeah :)12:36
tristangrrr12:36
tristanSo what do we do about this :-/12:36
gitlab-br-botpush on buildstream@master (by Tristan Van Berkom): 2 commits (last: tests/sources/tar.py: Converted tar test to use the CLI and enhanced) https://gitlab.com/BuildStream/buildstream/commit/069dcb4f43e64197224fd9d9bc56c6d6d54ac84712:37
gitlab-br-botbuildstream: issue #145 ("Tar Source should normalize leading `.`") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/14512:37
cs_shadowso far, `import` is the only one that I've seen causing problems. I'm not sure how common this would be12:37
tristanso elements are going to call "stage_sources"12:37
tristanAnd while some elements only expect it to be present in the sandbox (mount will work)12:38
tristanOther elements expect the data to appear at that location, and dont run a sandbox12:38
cs_shadowyes, i added that option for the plugins that are not running a sandbox12:38
cs_shadowbut admittedly it's a cop-out solution12:38
tristanDoing some processing on data with some python code and buildstream utilities, without running a sandbox, certainly happens12:39
tristanWhen it happens in compose elements, it happens on artifacts instead of sources12:39
tristanHmmm12:39
tristanWell, maybe we can churn some things here12:40
tristanmaybe it makes sense to say that "If the element does not need a sandbox, then we can assume it's unsafe to mount"12:41
tristanNot sure if that entirely makes sense, but certainly, not supporting the incremental build thing transparently for elements which dont use sandboxes sounds acceptable at face value12:42
tristancs_shadow, there is another interesting side effect, which maybe does not matter a great deal initially12:43
tristancs_shadow, from what I can tell, if we support incremental builds from workspaces, it means that the first time you build... it will need a new cache key (because calculated from workspace)...12:44
tristanand *then*... the next time you build, *even* if you didn't modify anything in the workspace12:44
tristanit will require a rebuild again, because local directory is effected and so is cache key12:44
tristanthere is another interesting stumper12:45
cs_shadowhmm, during my testing, it didn't appear to be doing that. But I'll double check12:46
tristanOne way around that might be to create a manifest at workspace creation time, and only consider files which were originally in the workspace in the cache key; but that has other drawbacks too12:46
tristancs_shadow, if it's not, there is probably a bug somewhere; the rule for calculating a cache key from a workspaced source, is to do a recursive checksum of the directory content12:46
tristanin fact, this will not happen only on initial workspace, but potentially on every build12:47
cs_shadowokay, I'll check what's going on there. Reg. the MR, do you think we can move forward with it? (perhaps after better explaining how `mount_workspace` option should be used)12:48
tristanmodify foo.c, causes it to require rebuild, building changes foo.o, meaning another build is required12:48
tristanNo,12:48
tristancs_shadow, we need to think about this, I really dont think mount_workspace belongs in the API, rather a better path is to understand why it needs to be treated differently (initial thoughts "need of a sandbox", maybe ?) and treat it differently for that reason12:49
cs_shadowInstead of "need of a sandbox", it's probably more like "need to run commands in a sandbox" - as it probably still wants the directory structure12:51
cs_shadowalthough the elements would need some way of expressing that12:52
tristanfor the double rebuilds, it seems like a course of action could be to refuse to know about cache keys at startup time, and resolve them as a result of a build, in the case of workspaced elements (and elements which depend on such)12:52
tristanBut that problem, I'm happy to deal with after12:52
tristancs_shadow, yes it's certainly possible that *some* api is needed, I feel strongly that that API should not be an element knowing about what a workspace is12:53
tristanit may be something which defines the nature of the element which might be useful to deduce other things as well, I just dont want to take that lightly at all12:54
tristanso regarding the double-builds, I would go so far as to merge it with an open issue about improving that, possibly with the dynamic cache key resolution approach, which is tricky to implement (similar to bst build --track, but a bit more hare brained)12:55
tristanBecause really, workspaces are *often* used with --no-strict, and are for developers to test stuff, the artifacts are never pushed12:56
tristanAnd also, the second build (assuming --no-strict) is going to be mostly a noop, if incremental builds work at all12:56
tristanso it's an annoyance, but not horrible12:56
cs_shadowokay, thanks. I'm going to think about the sandbox run issue a bit more and get back to you once I have some nicer alternatives12:57
tristancs_shadow, yeah I think it's very tricky, I'll also sleep on it and will be thinking on it since you raised that unforeseen obstacle :-S12:58
cs_shadowthanks! :)12:59
tristanthese apis are at the very heart of what BuildStream does so, it's critical to get it right12:59
cs_shadowdefinitely, makes sense.13:00
*** adds68__ has quit IRC13:05
gitlab-br-botpush on buildstream@remove-arches (by Tristan Van Berkom): 20 commits (last: _platform/linux.py: Add preflight check to detect user namespaces) https://gitlab.com/BuildStream/buildstream/commit/a9a2bcece3b90092d9260f5c46675a0e17df381d13:28
gitlab-br-botbuildstream: merge request (remove-arches->master: WIP: Remove arches) #117 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/11713:28
tristancs_shadow, there is a whole other approach I just thought of13:30
tristancs_shadow, instead of deciding on whether we act differently at stage time for elements which want to use sources outside of a sandboxed environment...13:31
tristancs_shadow, we could instead just decide on whether a given element supports workspacing _at all_13:31
tristanNot sure how valuable that idea is, just throwing it out there as a thought exercise13:32
cs_shadowthat's not outrageous. Although in case of `import`, having a sandbox might make sense... not sure13:33
gitlab-br-botpush on buildstream@remove-pre-post-commands (by Tristan Van Berkom): 20 commits (last: Catch attempts to compose a list) https://gitlab.com/BuildStream/buildstream/commit/8c4d8ca2b7c886690fb933428c6768c745667db713:34
gitlab-br-botbuildstream: merge request (remove-pre-post-commands->master: WIP: Remove pre post commands) #118 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/11813:34
tristancs_shadow, I think you mean, having a workspace might make sense13:35
tristanin which case, yeah13:35
tristanit's true it might13:35
cs_shadowoh right, that was a trpo13:35
cs_shadowtypo*13:35
tristan:)13:35
cs_shadowgrr13:35
* cs_shadow is stepping out to get some lunch13:36
* tristan is leaving... right about now :)13:37
gitlab-br-botpush on buildstream@multiple_targets (by Tristan Maat): 2 commits (last: Move tests) https://gitlab.com/BuildStream/buildstream/commit/d3eb64a0baf326a3b6242a49d37659c4423f9c6513:40
gitlab-br-botbuildstream: merge request (multiple_targets->master: Multiple targets) #135 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/13513:40
gitlab-br-botpush on buildstream@multiple_targets (by Tristan Maat): 2 commits (last: Add tests for multiple targets) https://gitlab.com/BuildStream/buildstream/commit/c98a2743948c5359d39d744dad4f03871d25a63513:41
gitlab-br-botbuildstream: merge request (multiple_targets->master: Multiple targets) #135 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/13513:41
gitlab-br-botpush on buildstream@multiple_targets (by Tristan Maat): 2 commits (last: Add tests for multiple targets) https://gitlab.com/BuildStream/buildstream/commit/3f2dc772fd3381db4594ed1cea9604c365546de113:44
gitlab-br-botbuildstream: merge request (multiple_targets->master: Multiple targets) #135 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/13513:44
tristantlater, just made a little comment right now about an assertion I think we should remove... aside from that your branch rebased against master looks ready to go to me13:47
tlatertristan: Right, yeah, want to merge it right away?13:48
tlater(It *is* a Friday)13:48
tristantlater, I'm about to step out honestly... I'll have my phone though :)13:48
tlatertristan: Nah, I get it :)13:49
tlaterThough I'd like to just get your thoughts on the --except work so I can start with that, if you don't mind13:49
*** adds68__ has joined #buildstream13:49
tristangrrr13:49
tlaterHeh, it's alright if you don't, I can probably find something else to do for the next few hours13:50
tristantlater, ok well, remove that assertion and rebase, and I will merge that from my phone assuming all passes13:50
* tlater should really start discussing things with you early.13:50
tristantlater, let me know what it is with except ?13:50
tristanLike right now, and I'll tell you if I have a straight answer13:50
tlatertristan: You said you wanted an 'intersection' element for this work, I'm not sure how that would relate to a solution for it at all13:51
tristanNah, I dont want a new element13:51
tlaterGah, sorry, element API13:51
tristanAh, nah it's not important13:51
tristantlater, I thought the algo of calculating an intersection might be api-worthy, that's all13:52
tristanbut that's not important13:52
tlatertristan: My sketch for this would be essentially to do a breadth first search through the graph, and simply removing excepted elements - O(V+E), so pretty efficient.13:52
tristantlater, I guess what an intersection is... is where graph A overlaps with graph B, giving you the elements you would logically want to exclude from targets A using exceptions B13:52
tlaterHmm... I can just draft these things and show you Monday, that's probably better than keeping you from leaving now13:53
tristantlater, right, I think its a *little* more complicated, as you want to avoid removing from graph A, elements which graph B intersect with, but are orthogonally still depended on from other elements in A13:54
tristanI think that's what we do now13:54
tlatertristan: Yeah, but the algorithm I worked on throughout this week actually knows that an element still has parents13:54
tristanI.e. excepting is recursive, except it wont except things which are commonly depended on from outside the exception13:54
tristantlater, sounds reasonable anyway13:55
tlaterAlright, cool13:55
tristan"still has parents" sounds pretty algo specific :)13:55
tristanbut I think I get it13:55
tristancounting the references of dependencies13:55
tristansounds sane13:55
gitlab-br-botpush on buildstream@multiple_targets (by Tristan Maat): 10 commits (last: _loader.py: Adjust the loader to support multiple targets) https://gitlab.com/BuildStream/buildstream/commit/cd591d7ac9061c17283bf536cea0b58b14d783b313:56
gitlab-br-botbuildstream: merge request (multiple_targets->master: Multiple targets) #135 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/13513:57
*** tristan has quit IRC13:59
*** adds68__ has quit IRC14:12
*** adds68 has joined #buildstream14:12
*** Ebardie has joined #buildstream14:34
ssam2the buildstream docs appear to have vanished14:51
ssam2the last 'pages' CI job seems to have succeeded ... https://gitlab.com/BuildStream/buildstream/-/jobs/3888831714:53
ssam2slightly weird that the 'pages' job has an 'artifacts' entry, but the GitLab CI UI doesn't list any artifacts from that job14:55
* ssam2 tries triggering the pipeline for 'master' again15:02
ssam2that seems to have fixed things16:02
*** bochecha has joined #buildstream16:06
*** bochecha has quit IRC17:01
*** bochecha has joined #buildstream17:02
*** tristan has joined #buildstream17:25
tristanssam2, I've been watching this fwiw: https://gitlab.com/gitlab-org/gitlab-ce/issues/3489917:38
tristanknown bug with gitlab pages17:38
tristanusually doesnt effect us, but has in the past17:38
*** bochecha has quit IRC17:46
ssam2right! this case is weirder as the pages job didn't fail at all18:11
ssam2but it deleted all the pages, so something must have gone wrong.18:11
ssam2there was one pages job that got cancelled, so could have been that18:11
tristanthere is apparently some race condition18:29
tristanssam2, I think something about another job being started while last one was deploying, cancelling the last or something really stupid18:30
tristananyway18:30
ssam2ouch18:30
tristanyeah seems like it's really duct taped together18:31
ssam2i guess we'll notice quick enough when it happens, anyway18:31
tristanthe whole thing also seems to be designed around the use case of a git repository who's goal is to deploy static web pages18:31
tristananyway, they'll surely figure it out18:32
*** ssam2 has quit IRC19:01
*** valentind has joined #buildstream19:45
valentindI updated buildstream, now I get the message: "WARNING Unable to create user namespaces with bubblewrap, resorting to fallback"19:51
valentindWhat is the requirement for bubblewrap?19:51
valentindAh I got it. It was because put a wrapper script that would print the command line. And that did not work with buildstream.19:54
*** tristan has quit IRC22:07

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