| *** reuben640[m] has quit IRC | 00:27 | |
| *** narispo has quit IRC | 00:29 | |
| *** pro[m] has quit IRC | 00:29 | |
| *** krichter[m] has quit IRC | 00:29 | |
| *** DineshBhattarai[m] has quit IRC | 00:29 | |
| *** Demos[m] has quit IRC | 00:29 | |
| *** segfault1[m] has quit IRC | 00:29 | |
| *** walterve[m][m] has quit IRC | 00:29 | |
| *** tchaik[m] has quit IRC | 00:29 | |
| *** dylan-m has quit IRC | 00:29 | |
| *** m_22[m] has quit IRC | 00:29 | |
| *** albfan[m] has quit IRC | 00:29 | |
| *** jswagner has quit IRC | 00:29 | |
| *** jward has quit IRC | 00:29 | |
| *** milloni has quit IRC | 00:29 | |
| *** ironfoot has quit IRC | 00:29 | |
| *** jjardon has quit IRC | 00:29 | |
| *** chipb has quit IRC | 00:29 | |
| *** douglaswinship has quit IRC | 00:29 | |
| *** slaf has quit IRC | 00:29 | |
| *** finnb has quit IRC | 00:29 | |
| *** radiofree has quit IRC | 00:29 | |
| *** persia has quit IRC | 00:29 | |
| *** benbrown has quit IRC | 00:29 | |
| *** cgm[m] has quit IRC | 00:29 | |
| *** skullone[m] has quit IRC | 00:29 | |
| *** awacheux[m] has quit IRC | 00:29 | |
| *** asingh_[m] has quit IRC | 00:29 | |
| *** dbuch[m] has quit IRC | 00:29 | |
| *** mattiasb has quit IRC | 00:29 | |
| *** SamThursfield[m] has quit IRC | 00:29 | |
| *** kailueke[m] has quit IRC | 00:29 | |
| *** doras has quit IRC | 00:29 | |
| *** theawless[m] has quit IRC | 00:29 | |
| *** Trevio[m] has quit IRC | 00:29 | |
| *** connorshea[m] has quit IRC | 00:29 | |
| *** abderrahim[m] has quit IRC | 00:29 | |
| *** jjardon[m] has quit IRC | 00:29 | |
| *** tlater[m] has quit IRC | 00:29 | |
| *** milloni has joined #buildstream | 00:29 | |
| *** persia has joined #buildstream | 00:29 | |
| *** radiofree has joined #buildstream | 00:29 | |
| *** jjardon has joined #buildstream | 00:29 | |
| *** benbrown has joined #buildstream | 00:29 | |
| *** cgm[m] has joined #buildstream | 00:29 | |
| *** skullone[m] has joined #buildstream | 00:29 | |
| *** awacheux[m] has joined #buildstream | 00:29 | |
| *** doras has joined #buildstream | 00:29 | |
| *** theawless[m] has joined #buildstream | 00:29 | |
| *** asingh_[m] has joined #buildstream | 00:29 | |
| *** connorshea[m] has joined #buildstream | 00:29 | |
| *** SamThursfield[m] has joined #buildstream | 00:29 | |
| *** tlater[m] has joined #buildstream | 00:29 | |
| *** jjardon[m] has joined #buildstream | 00:29 | |
| *** dbuch[m] has joined #buildstream | 00:29 | |
| *** Trevio[m] has joined #buildstream | 00:29 | |
| *** mattiasb has joined #buildstream | 00:29 | |
| *** kailueke[m] has joined #buildstream | 00:29 | |
| *** abderrahim[m] has joined #buildstream | 00:29 | |
| *** irc.acc.umu.se sets mode: +o jjardon | 00:29 | |
| *** douglaswinship has joined #buildstream | 00:29 | |
| *** ironfoot has joined #buildstream | 00:29 | |
| *** ChanServ sets mode: +o ironfoot | 00:29 | |
| *** chipb has joined #buildstream | 00:29 | |
| *** jward has joined #buildstream | 00:29 | |
| *** finnb has joined #buildstream | 00:29 | |
| *** narispo has joined #buildstream | 00:29 | |
| *** slaf has joined #buildstream | 00:30 | |
| *** jswagner has joined #buildstream | 00:31 | |
| *** reuben640[m] has joined #buildstream | 00:54 | |
| *** albfan[m] has joined #buildstream | 01:23 | |
| *** DineshBhattarai[m] has joined #buildstream | 01:34 | |
| *** pro[m] has joined #buildstream | 01:43 | |
| *** Demos[m] has joined #buildstream | 01:44 | |
| *** walterve[m][m] has joined #buildstream | 01:53 | |
| *** tchaik[m] has joined #buildstream | 01:56 | |
| *** krichter[m] has joined #buildstream | 02:26 | |
| *** segfault1[m] has joined #buildstream | 02:56 | |
| *** dylan-m has joined #buildstream | 03:08 | |
| *** m_22[m] has joined #buildstream | 03:13 | |
| *** tristan has quit IRC | 05:17 | |
| *** tristan has joined #buildstream | 05:35 | |
| *** traveltissues has quit IRC | 05:40 | |
| *** traveltissues has joined #buildstream | 05:40 | |
| *** ChanServ sets mode: +o tristan | 06:44 | |
| tristan | Interesting that the code quality scores "degrade" when FIXME comments are added; often times a FIXME comment is added after a refactor leading us to better understand the code and better document what needs fixing | 06:44 |
|---|---|---|
| tristan | hmmm, "tests-no-usedevelop" is heavy duty, almost an hour now: https://gitlab.com/BuildStream/buildstream/-/jobs/570687628 | 06:56 |
| tristan | Ah ! I just remembered, we didn't yet do the prioritization of buildstream.conf as discussed (try buildstream1.conf or buildstream2.conf respectively, falling back on buildstream.conf), was that agreed ? | 06:59 |
| * tristan should make sure that gets into 1.6 | 06:59 | |
| *** hasebastian has joined #buildstream | 07:06 | |
| *** jude has joined #buildstream | 07:27 | |
| tristan | version specific configuration files for master: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1941 | 07:53 |
| *** tristan has quit IRC | 07:57 | |
| *** tpollard has joined #buildstream | 08:06 | |
| juergbi | tiny CI config change to get pipelines green again: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1939 | 08:28 |
| *** phildawson has joined #buildstream | 08:29 | |
| WSalmon | did we make some progress on https://gitlab.com/BuildStream/buildstream/-/issues/1310 recently? i remember chatter, is there a MR that i can test? | 08:40 |
| *** benschubert has joined #buildstream | 08:41 | |
| benschubert | What would be the easiest way to test integration commands? I need a fully fledged sandbox right? Would a `bst show --format %{public}' be enough? | 08:42 |
| juergbi | benschubert: I'm surprised we don't test integration commands yet | 08:43 |
| juergbi | or we simply don't use variables in the existing tests? | 08:44 |
| benschubert | we might, but maybe without variables? | 08:44 |
| benschubert | I'll check | 08:44 |
| benschubert | if we do without variables, would adding a variable be enough or would we rather have a fully fledged test? | 08:44 |
| benschubert | we definitely have integration commands in some tests with variables | 08:45 |
| juergbi | imo, adding a variable would be enough, however, we should clearly note in the test that it's intentional/important to avoid it accidentally getting removed | 08:45 |
| juergbi | hm, so why aren't they failing? | 08:45 |
| juergbi | WSalmon: benschubert is working on it ^^ | 08:45 |
| benschubert | I am not sure | 08:46 |
| *** narispo has quit IRC | 08:46 | |
| benschubert | we have in all our alpine base images `ldconfig %{libdir}` | 08:46 |
| benschubert | so I would have expected failures? | 08:46 |
| *** narispo has joined #buildstream | 08:46 | |
| juergbi | maybe ldconfig doesn't fail with that argument? | 08:47 |
| juergbi | unexpanded | 08:47 |
| *** tristan has joined #buildstream | 08:47 | |
| juergbi | considering that, maybe bst show style test actually makes more sense | 08:47 |
| benschubert | I just tried running a `ldconfig blah`, that fails | 08:47 |
| juergbi | hm | 08:48 |
| WSalmon | thanks guys i apprecate the effort. :D | 08:48 |
| *** cphang has joined #buildstream | 08:49 | |
| *** santi has joined #buildstream | 08:49 | |
| *** ChanServ sets mode: +o tristan | 08:50 | |
| tristan | benschubert, juergbi, any thoughts on !1941 ? | 08:50 |
| tristan | it's a pretty simple thing about config file loading, as I recall it was pretty unanimously agreed on in IRC, abderrahim[m] was also around | 08:51 |
| tristan | think we need to take this detail to the list, I'd like to just push it through | 08:51 |
| tristan | ? | 08:51 |
| juergbi | lgtm | 08:52 |
| benschubert | tristan: seems good to me, maybe ping cs-shadow too? I think recalling he was there in the discussion too | 08:52 |
| tristan | sure, I don't wanna rush things but also want to get some more momentum hehe | 08:53 |
| benschubert | yep definitely :) | 08:54 |
| benschubert | tristan: also, if you have a few brain cycles, about the testing of plugins in buildstream master, I'm wondering if we should just, instead, run tests for the "stable" "buildstream maintained" plugin repositories on each pipeline before merge? (maybe not on all platforms), this would ensure we test everything we care about, and once bst-plugins-experimental is split, we will have more stable, single focused repositories | 08:56 |
| benschubert | On the other hand, it might also be suboptimal, like, if git_tag and git end up in the same repo, we might want only `git` to be tested | 08:57 |
| benschubert | We could otherwise add a marker for specific tests we want to rnu and run only the ones with these markers | 08:57 |
| tristan | benschubert, So under that design basically; we would (A) Make this "source tests" things more of an explicit thing for third part repos to implement in their tests (if it isnt already)... (B) Have the ability to override the BuildStream installation in third party test suites (make sure their tox.ini files allow us to override BuildStream with our local checkout)... (C) Run tox on the well known third party plugin modules maintained under the | 09:00 |
| tristan | BuildStream umbrella as a part of our CI... ? | 09:00 |
| benschubert | yep | 09:01 |
| tristan | benschubert, At a glance, this *sounds* interesting, I think it also involves reviving a discussion about testing docker images which cs-shadow probably doesnt agree with | 09:01 |
| benschubert | and we could have a few repositories that we use that would give us good coverage | 09:01 |
| benschubert | testing docker images? | 09:01 |
| benschubert | ah about adding the external dependencies? | 09:01 |
| tristan | but that's a bit of a detail, I think if we can say "BuildStream supports these blessed plugins", then buildstream-docker-images can create a single test image for each platform which supports all plugins | 09:01 |
| benschubert | yep | 09:02 |
| benschubert | as we would be needing them | 09:02 |
| tristan | I think he preferred more granular images | 09:02 |
| benschubert | I think what he doesn't want (and I agree with him) is a single image to test _all and every possible_ plugin | 09:02 |
| tristan | yeah there I certainly agree | 09:02 |
| benschubert | we should keep the image for testing core with only what core needs for test :) | 09:02 |
| benschubert | good | 09:02 |
| benschubert | so yeah I wonder if that would not be easier | 09:02 |
| benschubert | we could also remove some of those 'sourcetests' hacks and have them explicitely run | 09:03 |
| tristan | I think if we do this, we should certainly untangle the mess which is running BuildStream's tox with --plugins | 09:03 |
| benschubert | yeah, we oculd basically remove it entirely | 09:03 |
| benschubert | ah maybe not | 09:03 |
| tristan | Currently it's so broken, you can't even select specific test on the commandline (not to mention the double run while skipping everything looks like a hack) | 09:03 |
| benschubert | because it's still good to have all the standard source tests | 09:04 |
| benschubert | tristan: you can, but you need to use '-k test_name' instead of giving the file nam | 09:04 |
| tristan | A | 09:04 |
| tristan | Ah | 09:04 |
| benschubert | (probably deserves documentation if people trip on it) | 09:04 |
| tristan | Hmmm, I think I don't agree we want to keep the source tests and treat them specially at all | 09:04 |
| benschubert | So push to plugin implementers the responsibility of adding _all_ the tests? | 09:05 |
| benschubert | Actually... | 09:05 |
| tristan | I mean, what are the requirements ? ... I think we want to ensure we gate all commits on all tests passing | 09:05 |
| tristan | But that doesnt need to be all covered by BuildStream's tox.ini | 09:05 |
| tristan | benschubert, Yes: Give the plugin implementor a reasonable fixture which allows them to declare the standard source tests | 09:06 |
| tristan | But have them at least do that part | 09:06 |
| tristan | If you implement a source, you can implement a Repo() and use this fixture to pass standard tests, with an explicit test in your pytest suite | 09:06 |
| benschubert | good point | 09:06 |
| benschubert | it might make many things simpler | 09:07 |
| tristan | Its kind of how I intuitively expect it to work | 09:07 |
| benschubert | I'd though be wary of doing this before we have split bst-plugins-experimental, I for sure don't want to have all of them testing in core | 09:07 |
| tristan | Then, we lose one thing which is core developers dont get all the tests which run in CI just by running tox locally | 09:07 |
| tristan | However *even that* could be changed | 09:07 |
| tristan | We could have tox.ini do the calling into third party plugin test suites... calling pytest on them directly right ? | 09:08 |
| tristan | Well I say "third party" but I mean "blessed / external" | 09:08 |
| benschubert | yeah, we could download the git repo if it doesn't exist and run tests there | 09:09 |
| benschubert | or ship a 'meta' project (a git project with submodules to everything with a helper to integrate running all tests) | 09:09 |
| tristan | We would still keep a git sha or tag in BuildStream tox.ini which we'd have to update at healthy intervals | 09:09 |
| tristan | We don't want uncertainty of what changed in a CI run, ever | 09:09 |
| benschubert | yep | 09:10 |
| benschubert | and we would not update it locally if the thing is already checked out | 09:10 |
| benschubert | so people could do sweeping changes | 09:10 |
| tristan | That sounds fun, not sure how you do that | 09:10 |
| tristan | I think when you run tox, it will install all the specifics, including the external plugins | 09:11 |
| tristan | But I probably don't know all the features :) | 09:11 |
| benschubert | well, we'd need the source repos, in order ot have the tests, not the pip package (unless we ship the tests in the source tarball) | 09:11 |
| tristan | Ok... just a reminder... it's been some weeks and I really need us to get to the bottom of the question of "How to allow multiple junctions to the same project to exist"... | 09:12 |
| benschubert | and in tox you'd do `if [ ! -d bst-plugins-scm ]; then git clone ....; fi; cd bst-plugins-scm && tox -e pyXX` | 09:12 |
| benschubert | (example, no contractual obligations :D) | 09:13 |
| tristan | And with that, I think I'll finally do the management/secretary work which I think people expected of me: Translating the 2.0 workgroup minutes into the milestone in gitlab | 09:13 |
| tristan | benschubert, sounds interesting... yes I see; they would not *install* their tests even if we pip installed them via git | 09:14 |
| tristan | makes sense | 09:14 |
| benschubert | Ok, I'll sleep on it a bit and make a POC, then I'll push that to the list | 09:15 |
| benschubert | thanks ! | 09:15 |
| tristan | And if there are any specific implications about this, we don't need to document this for plugin writing (i.e. our plugins follow our rules) | 09:15 |
| tristan | implications such as "where do you store your pytest suite" or what test targets or whatever | 09:16 |
| benschubert | yep | 09:16 |
| benschubert | which simplifies many things | 09:16 |
| tristan | benschubert, while I agree that this is a simple/good approach; the only real question is; whether this is irrationally expensive in CI | 09:17 |
| tristan | benschubert, I think it would be good to go ahead with a plan and implement it, and as you suggest, only use decorators to mark tests we want to run in BuildStream in the case that we decide it's too heavy | 09:18 |
| benschubert | if it is, we can put another twist: tests we want to run in buildstream core as part of CI get a `buildstream_core` marker, and we skip all the others | 09:18 |
| tristan | we can make that decision another day | 09:18 |
| tristan | right | 09:18 |
| benschubert | exactly :D | 09:18 |
| tristan | thinking in symphony :) | 09:18 |
| benschubert | since we have control over the plugins it's way easier | 09:18 |
| benschubert | Gah, seems there is finally a satisfactory way of testing plugins with core x) | 09:19 |
| *** phildawson_ has joined #buildstream | 09:21 | |
| *** phildawson has quit IRC | 09:23 | |
| coldtom | should https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_cas/cascache.py#L177 not be using localcas FetchMissingBlobs, or am i misunderstanding something about the architecture? | 09:27 |
| benschubert | juergbi: so doing a `bst show --format %{public}` on `base/alpine.bst` in `doc/examples/composition` before my change, shows that the expansion is made at the time of running the command as I see `%{libdir}` in the output. Thus if I were to expand them _before_ it is a breaking chang | 09:28 |
| benschubert | tristan: ^ too | 09:28 |
| juergbi | benschubert: yes, that's what I expected: https://irclogs.baserock.org/buildstream/%23buildstream.2020-05-27.log.html#t2020-05-27T14:20:00 | 09:31 |
| benschubert | right | 09:31 |
| juergbi | I'm not sure which is better | 09:32 |
| juergbi | I do like not having to special case splits, though | 09:32 |
| benschubert | so yeah, seems weird ot me that splits would be expanded but not the rest | 09:32 |
| benschubert | I'm just curious whether it would break something that is needed by someone | 09:32 |
| benschubert | our whole test suite passes with the change though | 09:34 |
| juergbi | the only downside I can think of is unnecessary rebuilds | 09:34 |
| juergbi | but it may well be insignificant | 09:35 |
| benschubert | yeah | 09:35 |
| benschubert | and it oculd not break something right? the variables would always be resolved to the same thing and are part of the artifact anyways or? | 09:37 |
| tristan | yeah this is what I understood from yesterday's chat as well | 09:37 |
| benschubert | Ok, I'll apply the fix and add a test then :) | 09:38 |
| juergbi | benschubert: that makes me wonder, what variable context was used when variables were expanded as part of running the integration command? | 09:38 |
| juergbi | was it the element where the integration commands were defined in or the element that was being assembled? | 09:39 |
| benschubert | https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/buildelement.py#L231 it's the element where it is defined AFAIK? | 09:39 |
| tristan | The element where the integration commands were defined I believe | 09:39 |
| tristan | the element itself is asked to integrate | 09:40 |
| benschubert | yeah, so that's resolved in the element | 09:40 |
| tristan | the reverse dependency asks it's dependencies to integrate | 09:40 |
| juergbi | right, good, so no change there | 09:40 |
| benschubert | are the variables stored in the cache key actually? | 09:40 |
| juergbi | no | 09:40 |
| benschubert | So we're actually fixing a bug x) | 09:41 |
| juergbi | yes, I think so | 09:41 |
| tristan | In a sense, at least that's what my gut is telling me :) | 09:41 |
| benschubert | Previously: | 09:41 |
| benschubert | - Assuming an element has an integration command with a variable not use anywhere else | 09:41 |
| benschubert | - Assuming another element is depending on it and gets build | 09:41 |
| benschubert | - Change the variable | 09:41 |
| benschubert | - No rebuild necessary. $ FAIL | 09:41 |
| tristan | benschubert, I don't think so at that level no | 09:42 |
| tristan | benschubert, because changing the variable will make the integration commands for that element expand differently, and the resolved integration commands should be part of the key right ? | 09:42 |
| tristan | Hmmm | 09:43 |
| benschubert | tristan: previously the non-expanded version was stored (I just checked) | 09:43 |
| benschubert | so nothing was keeping track of this change :) | 09:43 |
| tristan | Ok, sad | 09:43 |
| tristan | Heh | 09:43 |
| tristan | Good thing to fix | 09:43 |
| benschubert | Well, it will be fixed soon :D | 09:43 |
| benschubert | yeah I would not have wanted to have to debug that otherwise | 09:44 |
| tristan | benschubert, the unfortunate part is that the element declaring the integration command will be rebuilt itself, just because reverse deps will need to be rebuilt due to a changed integration command | 09:44 |
| tristan | To be honest, that was accounted for a long time ago | 09:44 |
| juergbi | yes, that's the downside. otherwise we'd have to make the integration commands part of the unique key of reverse dependencies | 09:45 |
| tristan | But when we made public data mutable by elements, we recognized a need to change that | 09:45 |
| benschubert | yeah, I'd rather keep the integration command in the element itself, not the rdeps | 09:45 |
| tristan | Right, but that would be too presumptuous | 09:45 |
| tristan | This is safer all around for "whatever element implementations decide to do with public data" | 09:45 |
| juergbi | actually, for things like `bst artifact checkout --integrate` it wouldn't even be enough | 09:45 |
| benschubert | and if you have a huge element, you could always have a compose/import element on top with the integration commands, and thus rebuild would be 'free' | 09:45 |
| juergbi | benschubert: this only really works if that element is used standalone | 09:46 |
| juergbi | as the integration commands generally need to run after everything has been staged | 09:46 |
| juergbi | (otherwise we'd do it as part of assemble) | 09:46 |
| benschubert | juergbi: what I meant is that if you want to avoid the new rebuild of the elemtn which integration command will change with my fix, having a separate element with the integration command will allow you not to have the rebuild | 09:47 |
| juergbi | no, that doesn't really work, afaict | 09:48 |
| benschubert | I am not sure why? | 09:48 |
| juergbi | you need to run the integration command across the whole dependency set | 09:48 |
| benschubert | but that's if one of your dependency had a change of integration command | 09:48 |
| juergbi | so you'd need a helper element for each combination of dependencies | 09:48 |
| benschubert | I don't think we are talking about the same thing here? | 09:49 |
| juergbi | ah, yes, I think you're right | 09:49 |
| juergbi | I thought you wanted to avoid running integration commands over and over | 09:50 |
| benschubert | ah no, not what I meant :) | 09:50 |
| tristan | What we need to do to optimize that is the whole ineptly named "artifact aliasing" thing | 09:50 |
| juergbi | I got it now | 09:50 |
| benschubert | Just avoiding the unnecessary rebuild :) | 09:50 |
| benschubert | awesome | 09:50 |
| tristan | Implement cut-offs of reverse dependency builds based on matching content hashes | 09:51 |
| juergbi | that can definitely help | 09:51 |
| benschubert | I'd love seeing cut-offs | 09:51 |
| juergbi | with remote execution you already get that more or less for free with the action cache | 09:51 |
| *** hasebastian has quit IRC | 09:51 | |
| juergbi | it still has to (virtually) stage everything, though | 09:51 |
| tristan | The aliasing thing is totally doable I'm convinced, just... have many fish in the pan now... | 09:53 |
| tristan | lets do 2.0 first ;-P | 09:53 |
| tristan | Damn I really don't get it, why is it that when I resolve https://gitlab.com/BuildStream/buildstream/, the badge *still* doesn't reflect 1.4.3 ?? | 10:09 |
| tristan | This used to work seamlessly | 10:09 |
| juergbi | tristan: it says 1.4.3 here | 10:14 |
| tristan | really ? | 10:17 |
| tristan | Aaaaaah | 10:17 |
| tristan | juergbi, my browser caching at work | 10:17 |
| tristan | If I close the page and open it again, it remains 1.4.2... if I hit refresh, I get the new one | 10:18 |
| benschubert | WSalmon, juergbi: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1943 here it is! | 10:24 |
| juergbi | ta | 10:25 |
| WSalmon | ooooo | 10:25 |
| *** slaf_ has joined #buildstream | 10:30 | |
| *** slaf has quit IRC | 10:32 | |
| tristan | tpreston, WSalmon opened this now fwiw: https://gitlab.com/BuildStream/buildstream/-/issues/1327 | 10:41 |
| tristan | A side effect of trying to get the ML 2.0 blocker items into the milestone list | 10:41 |
| tpreston | thanks tristan | 10:42 |
| tristan | While in bureaucratic mode, I wonder if https://gitlab.com/BuildStream/buildstream/-/issues/1127 should have been reopened to reflect it's unfinished-ness until !1943 lands... | 10:43 |
| tristan | but my better self says that really, that would just be overkill micromanagement not worth anyone's while | 10:43 |
| benschubert | tristan: and this part actually does work :) | 10:44 |
| benschubert | the problem is only with public data | 10:44 |
| tristan | Yeah :) | 10:44 |
| benschubert | and we already have an issue for this bug | 10:44 |
| tristan | True, anyway I think we will revisit the milestone with more concrete blocker lists once we nail all the big ticket items | 10:44 |
| benschubert | seems good | 10:46 |
| *** phildawson-ct has joined #buildstream | 10:52 | |
| *** phildawson_ has quit IRC | 10:53 | |
| tpreston | So I'm trying to convert my generic linux.bst to a plugin. I've started with a naive implementation but it errors on the (?) logic https://gitlab.com/celduin/bsps/bst-boardsupport/-/blob/9eb652c78be77d224dd147e0cb778ec842995c07/plugins/elements/linux.yaml#L8 | 10:55 |
| *** hasebastian has joined #buildstream | 10:55 | |
| tpreston | What's the right way to do logic in a plugin? | 10:55 |
| tpreston | I want to conditionally set kernel_arch, based on target_arch | 10:56 |
| WSalmon | I think you *might* be able to do this with https://gitlab.com/BuildStream/buildstream/-/blob/bst-1.4/buildstream/plugin.py#L244 configure, but it would be better to do this within the yaml section if posible i would have thought | 11:04 |
| tpreston | So I would implement a configure() and `if kernel_arch == "": kernel_arch = something based on target_arch` ? | 11:08 |
| tpreston | Ahh, target_arch is a project-level configuration. So a general plugin shouldn't be aware of this. | 11:10 |
| juergbi | that brings up an interesting point. some plugins definitely assume some variables to exist. e.g. prefix | 11:13 |
| juergbi | right now that's a variable in the default project.conf but we might actually remove that from the default | 11:13 |
| juergbi | (and leave it up to projects/includes) | 11:14 |
| juergbi | however, it still makes sense for plugins to use those | 11:14 |
| juergbi | which means that plugins would have to declare which variables they rely on | 11:14 |
| tpreston | juergbi: for now, is it ok to just expect certain variables to exist? | 11:18 |
| tpollard | target_arch is not default? | 11:18 |
| tpollard | it's freedesktop api | 11:18 |
| tpollard | piggybacking the builtin arch type | 11:19 |
| tpreston | That's right, so the linux plugin shouldn't be aware of it | 11:21 |
| tristan | juergbi, I think that the main error reporting will report the provenance of where a referenced and undeclared variable came from | 11:24 |
| tristan | juergbi, so long as we improve provenance information (i.e. make sure it prints well for subprojects and for plugins), I think the error reporting is intuitive enough that plugins need not additionally "declare" which variables they depend on | 11:25 |
| tristan | (the use of a variable is already a declaration of that dependency) | 11:25 |
| juergbi | tristan: I meant in the plugin documentation | 11:26 |
| *** narispo has quit IRC | 11:27 | |
| juergbi | to declare what it expects from a project | 11:27 |
| *** santix has joined #buildstream | 11:35 | |
| tristan | juergbi, Except that plugin documentation always includes a verbatim copy of the plugin defaults | 11:36 |
| tristan | But sure, it could | 11:36 |
| tristan | It would be good to automate though | 11:36 |
| *** santi has quit IRC | 11:36 | |
| tristan | I think we should refactor https://gitlab.com/BuildStream/buildstream/-/blob/master/doc/Makefile#L40 into a sphinx plugin that is similar to the sphinx-api-doc thing but does it specially for plugins | 11:37 |
| tristan | That'll be better for third party plugins to make consistent documentation, and could take care of automatically scanning default .yaml files for %{variables} and summarizing those requirements | 11:38 |
| tristan | benschubert, more plugin integration stuff, but not related to tests | 11:38 |
| tristan | ^^ | 11:38 |
| * tristan could probably allocate a day for that, help get this plugins show on the road | 11:39 | |
| * tristan hopes he doesn't get the usual headache from looking at sphinx code | 11:39 | |
| tristan | Ok FWIW, the main big ticket items which made Blocker status, or "Block on a proposal" status, are all on https://gitlab.com/BuildStream/buildstream/-/milestones/3 now | 11:40 |
| tristan | Few duplicates resolved in the process | 11:40 |
| tristan | maybe I should remove some irrelevant things which are too fine grained to consider at this point ? | 11:42 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/issues/318 for example, incomplete cache key tests, seems premature on that list | 11:42 |
| tristan | We can't even say for sure *how* that will be tackled | 11:42 |
| tristan | This shouldnt be on the milestone as I understand it: https://gitlab.com/BuildStream/buildstream/-/issues/38 | 11:43 |
| tristan | I'll fix that | 11:43 |
| tristan | juergbi, Is it safe to say that the assumptions around "Sandboxing" from the planning sessions: https://mail.gnome.org/archives/buildstream-list/2020-April/msg00013.html also apply to storage ? | 11:46 |
| juergbi | tristan: as in "buildbox[-casd] only"? | 11:47 |
| tristan | juergbi, i.e. for BuildStream 2.0, a project can "require the saving of extended file attributes and ability to set them in the sandbox", and if it can be supported in the current environment, it will be | 11:47 |
| tristan | Otherwise the requirement fails and BuildStream aborts | 11:47 |
| tristan | Right, we only discussed this in terms of sandboxing | 11:48 |
| tristan | However it needs to be guaranteed across the board | 11:48 |
| tristan | At least when it comes to file attributes and such | 11:48 |
| tristan | We need to be able to know for sure that the artifact will be stored with the information we required to be stored in it | 11:48 |
| juergbi | right, we currently don't have a way to query buildbox-casd what attributes it supports | 11:49 |
| tristan | I feel these sandboxing and storage are more and more hand in hand | 11:49 |
| juergbi | however, it should be similar to the sandboxing attributes in the end | 11:50 |
| juergbi | or rather, I expect those to also be configured in the `sandbox` dict | 11:50 |
| tristan | Hmmm, does this mean we have showstoppers with the v2 REAPI ? | 11:50 |
| juergbi | there are no uid/gid fields yet in the upstream REAPI | 11:50 |
| tristan | We can't very well say that this is guaranteed if it's not standardized, especially since we're saying we're not going to provide an artifact cache server | 11:50 |
| juergbi | however, it should be easy to get those upstream when we actually need them | 11:51 |
| juergbi | mode/permission bits are already supported upstream | 11:51 |
| tristan | extended attributes and file capabilities... | 11:51 |
| juergbi | xattr could already be supported with the string-based node properties | 11:51 |
| juergbi | I don't think we need special fields for those | 11:51 |
| juergbi | for file capabilities I'm not sure. string-based might make sense for that as well | 11:52 |
| tristan | If there are "string-based node properties", why would we need anything else ? | 11:52 |
| tristan | Just as a means of reducing the size of a CAS storage ? | 11:52 |
| juergbi | they are not as efficient | 11:52 |
| juergbi | yes | 11:52 |
| tristan | Right ok I get it | 11:52 |
| juergbi | also, buildbox needs to add support for each attribute, support in the protocol alone is not sufficient | 11:55 |
| juergbi | however, I don't think that would be really difficult for any of these attributes | 11:55 |
| juergbi | the main difficulty is still the sandboxing side | 11:56 |
| juergbi | but subuids should be the way to go on linux | 11:56 |
| juergbi | not sure whether file capabilities are possible right now | 11:56 |
| juergbi | maybe possible with buildbox-fuse, have to check | 11:57 |
| cphang | thanks tristan would it be possible to add https://gitlab.com/BuildStream/buildstream/-/issues/1318 to https://gitlab.com/BuildStream/buildstream/-/milestones/3 as well? This stops use of bst using a remote CAS from 1.93.2 onwards | 11:57 |
| cphang | sorry I mean 1.93.3 | 11:58 |
| tristan | commented here: https://gitlab.com/BuildStream/buildstream/-/issues/38#note_351035576 | 11:58 |
| juergbi | cphang: we still don't have an easy way to reproduce it locally, right? | 11:59 |
| tristan | cphang, my hope is to keep as much details/bug *out* of the milestone such that we can focus on the big ticket items as the milestone | 11:59 |
| tristan | it's not meant to be a micro-managing fine grained thing at this point | 11:59 |
| cphang | tristan, fair enough. makes sense thanks | 11:59 |
| cphang | juergbi, no we can only reproduce at load right now, it's hard to pin down otherwise. | 11:59 |
| tristan | cphang, once all the main "features" land, we will probably have monthly meetings to recap and repopulate with blockers | 12:00 |
| tristan | I think that's the approach we're hoping for... drill down into details deeper and deeper as we approach 2.0 | 12:00 |
| cphang | tristan, sounds good to me :) | 12:00 |
| tristan | :) | 12:00 |
| cphang | coldtom, WSalmon do you have any other details for juergbi to help reproduce #1318 ? | 12:00 |
| coldtom | i've never managed to repro locally, but i lack the bandwidth to push anything beefy over the network | 12:01 |
| tristan | Regarding #1318 at a glance... I think the fact that we only get "Unexpected error in RPC handling" could be considered a bug in itself | 12:02 |
| juergbi | coldtom: have you tried with a local buildbarn server? | 12:02 |
| tristan | That error message is so precisely detailed, telling me exactly what went wrong and being so (sarcastically) helpful in helping me fix the error | 12:02 |
| cphang | I think you've accurately captured my feelings on that issue tristan xD | 12:03 |
| juergbi | yes, most likely the issue is bad error handling | 12:03 |
| tristan | In the past, we've had obscure errors which don't often happen, and improved error reporting several rounds before actually finding out the root cause | 12:04 |
| coldtom | juergbi, not with more than a toy project i'm afraid, i can give that a shot after lunch | 12:04 |
| tristan | I would suggest a similar approach here :) | 12:04 |
| juergbi | coldtom: ok, great. clear steps how to reproduce this locally would definitely make this much easier to dig into | 12:04 |
| cphang | so I know traveltissues has put several MRs in buildbox along this direction | 12:04 |
| juergbi | yes, they are all merged, afaik | 12:05 |
| cphang | https://gitlab.com/BuildGrid/buildbox/buildbox-common/-/merge_requests/267 and https://gitlab.com/BuildGrid/buildbox/buildbox-common/-/merge_requests/268 iirc | 12:05 |
| WSalmon | i ran in to the memory issue with casd while trying to reproduce locally | 12:08 |
| juergbi | looking into this now | 12:09 |
| WSalmon | [00:00:03] FAILURE bootstrap/gcc-build-deps.bst: Failed to push artifact blobs with status UNKNOWN: Unexpected error in RPC handling | 12:09 |
| WSalmon | ooooo | 12:09 |
| WSalmon | i got it locally | 12:09 |
| WSalmon | i think | 12:09 |
| WSalmon | and casd is still alive but using 9.4Gb of ram | 12:10 |
| WSalmon | so dosent look like its OOM error | 12:10 |
| *** tristan has quit IRC | 12:13 | |
| juergbi | WSalmon: nice, that's with buildbarn also running locally? | 12:25 |
| WSalmon | yep | 12:26 |
| WSalmon | oh | 12:26 |
| WSalmon | no | 12:26 |
| WSalmon | i can give you a key so you can try it out | 12:26 |
| WSalmon | willsalmon/reproduce_grpc | 12:28 |
| WSalmon | that branch gets grpc errors quickly for me | 12:29 |
| WSalmon | they usually go bang by II < 150 which is quite fast, let me know if you can reproduce? | 12:43 |
| valentind | benschubert, I am looking at _variables.pyx in master. It seems we cache regex parsing of strings. But we do not cache the result of evaluation of variables. So that for example if it tries to resolve foo: 1%{bar}, bar: 2%{baz}, baz: 3, then It when evaluating "foo", it will find the value of bar. But it will not cache it, and when evaluating the value of bar, it will just recompute it. Am I correctly understanding the code? | 12:47 |
| valentind | I am thinking there was maybe a reason for that. But I do get why we cache the parsing phase then. | 12:47 |
| benschubert | valentind: which line exactly? | 12:48 |
| valentind | in _flatten | 12:48 |
| valentind | It goes through all definitions | 12:48 |
| valentind | And calls _expand_expstr | 12:49 |
| valentind | But _expand_expstr is given self._expstr_map, which only contains definitions. | 12:49 |
| *** hasebastian has quit IRC | 12:50 | |
| valentind | And I am thinking if it was given also flat, then it could just take the computed values from there directly instead of recomputing variables that were already evaluated. | 12:50 |
| benschubert | It's been a long time, but if I remember correctly, it's because the same expstr might be reuse in multiple context and have the variable resolved differently? I'm not quite sure anymore | 12:50 |
| valentind | benschubert, But it is one Variables instance per element. | 12:51 |
| valentind | It should always be the same value. | 12:51 |
| valentind | There is no way to change variables later than __init__. | 12:51 |
| *** hasebastian has joined #buildstream | 12:52 | |
| benschubert | valentind: the 'PARSE_CACHE' is global and shared | 12:53 |
| valentind | I am not suggesting to modify PARSE_CACHE. | 12:53 |
| valentind | I am saying while computing the flatten map, we can just reuse already computed values. Specially because have it in memory already. | 12:54 |
| valentind | In _flatten, the local variable "flat" will contain all those values. | 12:55 |
| benschubert | you mean in https://gitlab.com/BuildStream/buildstream/-/blob/master/src/buildstream/_variables.pyx#L116 using the flatten_map? | 12:55 |
| valentind | As well yes. | 12:56 |
| benschubert | mmh it might be possible | 12:56 |
| valentind | But also during flatten, every call to _expand_expstr should look into "self.flat" | 12:56 |
| benschubert | I'd definitely accept a PR if you manage to optimize it, it would be great :) | 12:56 |
| valentind | benschubert, What I wanted to know if there was a good reason not to do it. | 12:57 |
| valentind | But if there is no, then it gives me more freedom. | 12:57 |
| benschubert | I haven't written that part of the code originally | 12:57 |
| benschubert | but we have quite a few tests around variables now, and probably a 'show' on fsdk would give a good indication of whether something is breaking | 12:57 |
| benschubert | so yeah, if you manage to optimize that, it woul dbe a great win | 12:58 |
| valentind | benschubert, The point is not necessarily to optimize. I would like to change a bit the code to allow undefined variables if they are unused in some contexts (junction). | 12:58 |
| benschubert | what? | 12:58 |
| benschubert | why ? | 12:58 |
| valentind | Because some include files included from junctions define things that cannot be used in junctions. | 12:58 |
| valentind | And if in project.conf there is a variable defined depending on such a variable, then it will fail to flatten. | 12:59 |
| valentind | So in junction, we should not look at variables we do not use. | 12:59 |
| benschubert | can't you set a default variable in project.conf? | 12:59 |
| benschubert | That seems pretty error prone to me | 12:59 |
| valentind | That would create more errors. | 13:00 |
| valentind | Because they might be used. | 13:00 |
| valentind | I want it to fail if it is used in incomplete context. | 13:00 |
| valentind | Also priorities and includes are not always that obvious. If project.conf includes a.yml and junction.bst:b.yml. And then a.yml depends on a variable from junction.bst:b.bst. | 13:01 |
| benschubert | BRB, sorry | 13:02 |
| valentind | If you define a default value in a.yaml, when you actually merge with the junction, it might just keep that value instead of b.yml. | 13:02 |
| valentind | So in practice bad default values are not something we want. | 13:02 |
| valentind | benschubert, Actually there is caching. But that was not obvious to find. | 13:19 |
| valentind | In flatten: self._expstr_map[key] = expstr | 13:19 |
| valentind | But it does not cache variables it resolves in the wrong order. | 13:26 |
| benschubert | > f you define a default value in a.yaml, when you actually merge with the junction, it might just keep that value instead of b.yml. | 13:28 |
| benschubert | No, it won't there is a clear hierarchy of what takes precedent and element takes over project.conf :) | 13:28 |
| benschubert | > But it does not cache variables it resolves in the wrong order. | 13:28 |
| benschubert | Not sure I understand that part? | 13:28 |
| benschubert | valentind: so if I reword: you'd like variable resolution to be lazy and fail with unresolved dependency _only_ when the variable is used and not before, am I correct? | 13:29 |
| valentind | If foo depends on bar, and foo is flatten before bar, then it will computer bar twice. Where was if it was evaluated the other way, it would use cache value. | 13:29 |
| valentind | benschubert, Yes, but I also want to check for all definitions in other cases than junctions. | 13:30 |
| benschubert | valentind: so, inside an element, if the kind is 'junction', don't bail out on undefined variables, but still keep the current behavior in other cases? | 13:31 |
| benschubert | ok I got you now, sorry, it's been a mind twisting day :D | 13:32 |
| valentind | Yes, | 13:32 |
| benschubert | valentind: if I read everything correctly, what we could do is, remove the access to `flat`, provide a `__getitem__` interface on variables, and handle keyerrors there instead of during the `flatten`? And possible make this new behavior optional for only junctions | 13:39 |
| benschubert | since `subst` is already handling that case, the only other case is in `get_variable()` for the `Element` class | 13:40 |
| benschubert | would that make sense? | 13:40 |
| valentind | Yes, for 1.5, I had to add a method for get_variable. But for the rest it was using subst. Though 1.5 needed a full rewrite. | 13:41 |
| valentind | I think it will be less complex in master. | 13:41 |
| valentind | I just want to make sure cache works, even if we do not call flatten. | 13:42 |
| benschubert | valentind: please do benchmarks if you stop calling flatten :) | 13:45 |
| *** tristan has joined #buildstream | 13:48 | |
| traveltissues | can someone please have a look at https://gitlab.com/BuildStream/bst-plugins-experimental/-/merge_requests/114 | 14:31 |
| juergbi | WSalmon: fyi: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1944 | 14:36 |
| juergbi | this isn't a fix for the issue you're seeing but it halves the FindMissingBlobs load for push | 14:37 |
| WSalmon | nice :D | 14:39 |
| WSalmon | thanks juergbi | 14:40 |
| cphang | nice thanks juergbi | 14:40 |
| juergbi | there is still a similar issue in sandboxremote, though, the remote execution client | 14:40 |
| juergbi | it's a bit more complicated there as it has to cater for multiple remotes (pull blobs from artifact cache and push them to RE CAS) | 14:41 |
| juergbi | however, in the typical case of a unified CAS (or if all needed blobs are in the local cache) we should definitely also be able to optimize away the extra FindMissingBlbos | 14:41 |
| *** tpollard has quit IRC | 14:56 | |
| *** tpollard has joined #buildstream | 14:56 | |
| valentind | benschubert, Did some benchmarking. The best is actually to not flatten or cache anything. Instead to evaluate on demand. However we can check for missing variables and cycles. But generating the data structures for the variables is overkill. | 15:50 |
| valentind | There is only for "bst show -f '%{vars}'" where need to calculate all of them. | 15:51 |
| valentind | With 60 randomly generated variables, I go from ~ 40 seconds to ~ 1 second | 15:51 |
| valentind | With more my computer cannot cope. 100 variables and I get OOM. | 15:52 |
| benschubert | valentind: wut? I'm greatly surprised but that looks great, ping me on the MR please | 15:57 |
| *** jude has quit IRC | 16:06 | |
| *** tlater[m] has quit IRC | 16:07 | |
| *** jjardon[m] has quit IRC | 16:07 | |
| *** abderrahim[m] has quit IRC | 16:07 | |
| *** connorshea[m] has quit IRC | 16:07 | |
| *** Trevio[m] has quit IRC | 16:07 | |
| *** DineshBhattarai[m] has quit IRC | 16:07 | |
| *** pro[m] has quit IRC | 16:07 | |
| *** Demos[m] has quit IRC | 16:07 | |
| *** walterve[m][m] has quit IRC | 16:07 | |
| *** krichter[m] has quit IRC | 16:07 | |
| *** m_22[m] has quit IRC | 16:07 | |
| *** theawless[m] has quit IRC | 16:07 | |
| *** doras has quit IRC | 16:07 | |
| *** kailueke[m] has quit IRC | 16:07 | |
| *** SamThursfield[m] has quit IRC | 16:07 | |
| *** mattiasb has quit IRC | 16:07 | |
| *** dbuch[m] has quit IRC | 16:07 | |
| *** asingh_[m] has quit IRC | 16:07 | |
| *** awacheux[m] has quit IRC | 16:07 | |
| *** skullone[m] has quit IRC | 16:07 | |
| *** cgm[m] has quit IRC | 16:07 | |
| *** reuben640[m] has quit IRC | 16:07 | |
| *** albfan[m] has quit IRC | 16:07 | |
| *** tchaik[m] has quit IRC | 16:07 | |
| *** segfault1[m] has quit IRC | 16:07 | |
| *** dylan-m has quit IRC | 16:07 | |
| *** kailueke[m] has joined #buildstream | 16:08 | |
| *** phildawson_ has joined #buildstream | 16:15 | |
| *** phildawson-ct has quit IRC | 16:16 | |
| *** phildawson_ has quit IRC | 16:25 | |
| *** phildawson has joined #buildstream | 16:25 | |
| *** albfan[m] has joined #buildstream | 16:33 | |
| *** SamThursfield[m] has joined #buildstream | 16:33 | |
| *** theawless[m] has joined #buildstream | 16:33 | |
| *** abderrahim[m] has joined #buildstream | 16:33 | |
| *** tchaik[m] has joined #buildstream | 16:33 | |
| *** segfault1[m] has joined #buildstream | 16:33 | |
| *** tlater[m] has joined #buildstream | 16:33 | |
| *** dbuch[m] has joined #buildstream | 16:33 | |
| *** jjardon[m] has joined #buildstream | 16:33 | |
| *** doras has joined #buildstream | 16:33 | |
| *** mattiasb has joined #buildstream | 16:33 | |
| *** connorshea[m] has joined #buildstream | 16:33 | |
| *** reuben640[m] has joined #buildstream | 16:33 | |
| *** Demos[m] has joined #buildstream | 16:33 | |
| *** asingh_[m] has joined #buildstream | 16:33 | |
| *** Trevio[m] has joined #buildstream | 16:33 | |
| *** dylan-m has joined #buildstream | 16:33 | |
| *** walterve[m][m] has joined #buildstream | 16:33 | |
| *** m_22[m] has joined #buildstream | 16:33 | |
| *** pro[m] has joined #buildstream | 16:33 | |
| *** DineshBhattarai[m] has joined #buildstream | 16:33 | |
| *** skullone[m] has joined #buildstream | 16:33 | |
| *** awacheux[m] has joined #buildstream | 16:33 | |
| *** cgm[m] has joined #buildstream | 16:33 | |
| *** krichter[m] has joined #buildstream | 16:33 | |
| juergbi | WSalmon, cphang: I can no longer reproduce the issue with this buildbox-common branch: https://gitlab.com/BuildGrid/buildbox/buildbox-common/-/commits/juerg/grpc-retry | 16:36 |
| juergbi | have to look into a few details before opening a MR for this, but you could already try that if the issue is currently a blocker | 16:38 |
| juergbi | I'd recommend also using buildbox-casd from this branch https://gitlab.com/BuildGrid/buildbox/buildbox-casd/-/merge_requests/188 to avoid unhelpful error messages in case there are still issues | 16:38 |
| WSalmon | if i use the FD images what versions of the rest of buildbox do i want? 0.0.8? | 16:40 |
| WSalmon | master | 16:40 |
| juergbi | for the other two buildbox modules (fuse and run-bubblewrap) master is currently identical to 0.0.8 | 16:41 |
| juergbi | i.e., there are currently no compatibility issues between buildstream master and buildbox-* master | 16:41 |
| *** santix has quit IRC | 16:50 | |
| *** tpollard has quit IRC | 17:07 | |
| *** hasebastian has quit IRC | 17:48 | |
| *** Genesis8267 has joined #buildstream | 17:53 | |
| *** Genesis8267 has quit IRC | 17:53 | |
| *** santi has joined #buildstream | 18:56 | |
| *** santi has quit IRC | 19:07 | |
| *** Caroline2266 has joined #buildstream | 19:09 | |
| *** Caroline2266 has quit IRC | 19:10 | |
| *** Violet8291 has joined #buildstream | 20:13 | |
| *** Violet8291 has quit IRC | 20:15 | |
| *** Julia6672 has joined #buildstream | 21:02 | |
| *** radiofree has quit IRC | 21:04 | |
| *** Julia6672 has quit IRC | 21:04 | |
| *** benschubert has quit IRC | 21:31 | |
| *** Gabriella3593 has joined #buildstream | 22:02 | |
| *** Gabriella3593 has quit IRC | 22:04 | |
| *** cphang has quit IRC | 22:35 | |
| *** phoenix has joined #buildstream | 22:47 | |
| *** phoenix has quit IRC | 22:56 | |
| *** phoenix has joined #buildstream | 23:13 | |
| *** phoenix has quit IRC | 23:37 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!