| *** mohan43u has quit IRC | 04:49 | |
| *** mohan43u has joined #buildstream | 06:55 | |
| *** tristan has quit IRC | 07:41 | |
| *** tristan has joined #buildstream | 07:45 | |
| *** ChanServ sets mode: +o tristan | 07:45 | |
| tristan | valentind, I need to write up a proper commit message for !1954 | 07:57 |
|---|---|---|
| tristan | This changes loader.py, _project.py, _includes.py and _frontend/widget.py but doesn't explain what has changed in all of these files :-S | 07:58 |
| * tristan tries to parse " We need to process includes without options to load options included in separate files. " | 08:06 | |
| *** benschubert has joined #buildstream | 08:06 | |
| *** jonathanmaw has joined #buildstream | 08:08 | |
| tristan | I think the goal is in fact to process the options before doing composition, which happens when merging includes | 08:08 |
| tristan | hrrrrm, internal _includes.py::Includes._include_file() returns this super funky 3-tuple, and doesnt even document what it is returning :-S | 08:11 |
| *** santi has joined #buildstream | 08:44 | |
| *** jonathanmaw has quit IRC | 08:46 | |
| tristan | Ok so, looks like !1954 is interestingly _only_ broken for project.conf, but already works as expected for element yaml | 08:51 |
| tristan | I'm not particularly fond of _frontend/widget.py saying "if project.options:" <--- this makes it appear as if it were somehow acceptable to be printing the project options before they were resolved | 08:53 |
| tristan | valentind, is there any reason we should have OptionPool.load_cli_values() have an 'ignore_unknown' option ? | 09:03 |
| valentind | YEs | 09:04 |
| tristan | It looks like you added this in July 2018, well before we figured out that options need to be resolved in the context of the project they are defined in | 09:04 |
| valentind | Because options might be defined in included files | 09:04 |
| tristan | I think that CLI values can only be specified for the toplevel project | 09:04 |
| valentind | Yes, but it can be in an include file at top level. | 09:05 |
| tristan | otherwise we'd need an explicit semantic to ensure that we have correct specificity | 09:05 |
| tristan | Oh, options themselves being defined in an included file :-S | 09:05 |
| tristan | valentind, So we validate that on the second round for project.conf but not the first right ? | 09:06 |
| tristan | I suppose we have test cases for this | 09:06 |
| tristan | (undefined options specified by the cli) | 09:06 |
| tristan | We must | 09:06 |
| tristan | valentind, While I have you, what is the purpose of changing widget.py in !1954 ? | 09:07 |
| tristan | If it's just defensive paranoia, it's misplaced; displaying the correct options in the heading is imperative of course (a stack trace is better here than a silent/incorrect success) | 09:08 |
| valentind | If I remember correctly, it is that it is trying to use fields that are not yet defined. | 09:09 |
| tristan | That said, I have it on my TODO list to fix the UI formatting of the heading (we have a horrible bug where the heading gets printed multiple times at startup instead of one coherent heading) | 09:09 |
| tristan | Right, that should not happen, if it does; that in itself is a bug which shouldn't really be worked around | 09:09 |
| tristan | Again, the heading being printed too early (and especially multiple times (!)) is also a bug | 09:09 |
| valentind | Well, it looked to me the way widget intruded to those objects was a hack already. | 09:10 |
| valentind | Ah maybe not in master. | 09:10 |
| tristan | If the frontend is accessing non-public stuff, then public stuff needs crafting in the right places; there are still trace amounts of this nonsense in master (but less indeed) | 09:11 |
| valentind | It is not too early, really. It is that it never needed to load those options. | 09:11 |
| tristan | Not something to care about for bst-1 branch I'd say though | 09:11 |
| valentind | Thi happends when doing a "bst show" on a junction element. I think. | 09:11 |
| tristan | bst show doesnt print a heading afaik | 09:12 |
| tristan | no it doesnt, only the element list | 09:12 |
| valentind | Right. There are some commands when given just a junction element, it would fail. | 09:13 |
| tristan | Ummm | 09:13 |
| valentind | Anyway, that was caught by some test. So if you remove the hack, you can find where the problem is by running the tests. | 09:14 |
| tristan | So you're saying that when we have a session which loads a junction element for the purpose of the session (like source track/fetch), we don't completely load the project; and we print a bogus heading in the UI ? | 09:14 |
| valentind | Not a bogus. Just missing project options. That were not used anyway by the junction because you cannot use options in junctions. | 09:15 |
| tristan | That's a bogus heading I'm pretty sure | 09:15 |
| *** jonathanmaw has joined #buildstream | 09:16 | |
| tristan | Used by the junction or not, it's still "the project settings" being printed, so we should ensure that the project(s) gets fully loaded after loading only a junction before the session starts | 09:16 |
| tristan | ugh, that's a mess, test/format/junctions.py fails this because it's performing activities on cross-junction elements, so the toplevel project is not fully loaded in order to reach the target elements of interest | 09:21 |
| tristan | and this is failing *because* of the horribly buggy nature of printing multiple headings "on demand" as projects get loaded | 09:22 |
| tristan | Probably best to keep this hack in place first, then fix the headings to happen once after Stream says "Ok all is loaded", and remove the paranoid `if` after that | 09:23 |
| tristan | The project.conf clones in _project.conf are a bit weird, lets try to understand that so that we can write something like " We need to process includes without options to load options included in separate files. " | 09:24 |
| tristan | valentind, Do you have a more detailed explanation for that sentence on hand ? | 09:24 |
| tristan | I think what we're doing is cloning the toplevel project.conf and performing option processing on it once before applying includes to it | 09:25 |
| valentind | It was exactly what we talked before about. | 09:32 |
| valentind | project.conf might include files with "options:" | 09:32 |
| valentind | So we need to first process includes ignoring option processing to be able to load the options. | 09:33 |
| tristan | Hmmmm | 09:37 |
| tristan | So that comment is unrelated to the patch | 09:38 |
| valentind | Where is that? | 09:39 |
| tristan | valentind, reading the code from that patch, I'm seeing "... freaky project_conf.clone() passing this around to a new self._load_options() function ..." with that comment above, thinking... that comment must explain this cloning that's going on | 09:39 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/merge_requests/1954 | 09:39 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/merge_requests/1954/diffs#5de28618ea7bb36c58e2b7dda21be3f35ffc64bd_697_694 | 09:39 |
| tristan | I'm just trying to figure out what this is really doing so I can get a proper commit message and maybe at least comment on the code to make it clear what/why this is happening | 09:40 |
| valentind | It is not the cloning. | 09:40 |
| valentind | The comment is for the 3 lines. I put an empty line to delimitate it. | 09:41 |
| tristan | yeah, that cloning is what needs explaining I think | 09:41 |
| valentind | Because it is modified by load_options and it will be an incomplete one. | 09:42 |
| tristan | I split up your test case to include an exact same test case (using parametrize()) where the include is done on an element | 09:42 |
| tristan | And when doing the include on an element, the test passes without the whole patch | 09:43 |
| tristan | Which makes me wonder if there is a more clear way of doing this, like, can we load the options once and pass it alone to _include.py and have _include.py take care of resolving options ? | 09:44 |
| valentind | I would have to see the test to know what is missing. | 09:44 |
| tristan | I guess this is all mostly because we allow options to be declared in separate files | 09:44 |
| tristan | valentind, I mean the same test from the exact same patch on 1954, it does 2 includes in project.conf and has a manual element.bst | 09:45 |
| valentind | I think to make it really clear we should abandon yaml. | 09:45 |
| tristan | I added a second test which is a replica, and moves the same include to the element.bst instead of project.conf, and none of this juggling of cloned nodes is needed | 09:46 |
| tristan | test passes out of the box | 09:46 |
| tristan | Maybe we need to also have a test for this with a junction element for good measure | 09:46 |
| valentind | Maybe it actually works to not clone. And there is not way to make a test that breaks. But I think it makes the code more fragile and a small modification could probably break it. | 09:48 |
| valentind | But let me see. | 09:49 |
| valentind | tristan, Ah right, this is a yaml node. | 09:50 |
| valentind | Yes, include will modify it. | 09:50 |
| valentind | tristan, So what do you pass instead of project_conf_for_options? | 09:51 |
| valentind | Do you pass project_conf_first_pass? If so, that might work maybe. | 09:52 |
| valentind | But if you pass self._project_conf it does not. | 09:52 |
| valentind | The failing test would also include some configuration (but not options) through a junction. | 09:52 |
| valentind | Then this configuration would totally disappear. | 09:52 |
| valentind | This is because, at first pass, when seeing includes through junctions, this is just ignored but the '(?)' is still removed. | 09:53 |
| tristan | valentind, The test you added does break without the patch, but not if you perform the test on an element instead of on the project.conf | 09:56 |
| valentind | This should mostly fix project.conf includes. Not elements I think. | 09:57 |
| tristan | i.e. element.bst files don't suffer this special casing | 09:57 |
| valentind | Yes, it is expected that a (?) in the element would have not been affected by the bug this is trying to fix. | 10:01 |
| valentind | Only from project.conf | 10:02 |
| tristan | Whoa interesting | 10:02 |
| tristan | When the element is a junction, the test breaks | 10:02 |
| valentind | But junctions cannot use (?) | 10:03 |
| valentind | Can they? | 10:03 |
| valentind | Ah yes. | 10:03 |
| valentind | They cannot include, but can use (?) | 10:04 |
| tristan | Oh they cannot ? | 10:04 |
| * tristan doesn't see that anywhere in https://docs.buildstream.build/master/elements/junction.html, or recall it in any previous version of that document | 10:05 | |
| valentind | I do think they can include. In theory they could, but they cannot include through other junctions. So I think the make it less confusing we just disabled includes on junctions. | 10:05 |
| tristan | But junctions can use configuration which is inherited from adjacent junctions, I think they can use variables which are resolved via includes to other projects | 10:06 |
| tristan | This is a strange limitation, how was this decided and where is it documented ? | 10:07 |
| tristan | https://docs.buildstream.build/master/format_intro.html#include also does not mention this | 10:08 |
| tristan | I think that's a bug too :-S | 10:08 |
| valentind | No, it is a problem if we allow junctions to include through adjacent junctions. Otherwise if two junctions use each other, you can never complete them. | 10:10 |
| tristan | Of course | 10:10 |
| tristan | That would be expected, and I would expect a coherent circular reference explanation to be reported there | 10:10 |
| valentind | Then after you have to make loading order quite complex, because it is not just the dependencies. It would also be the include graph. | 10:10 |
| tristan | It already is | 10:11 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/issues/1359 this was a bug where I overlooked this | 10:12 |
| tristan | includes across junctions triggering loads was missed in my initial tests | 10:12 |
| tristan | We discussed this last week actually | 10:12 |
| valentind | That is projects. Not elements. | 10:13 |
| valentind | I am talking about adding (@) in a junction element. | 10:13 |
| tristan | Right, subproject loads get triggered by (A) Dependencies, (B) Plugin origins, (C) Element dependencies | 10:13 |
| tristan | Right | 10:13 |
| valentind | Not in the project.conf a junction is pointing to. | 10:13 |
| tristan | Okay | 10:13 |
| tristan | Things are already "quite complex", one thing that would make things clearly a bit less complex, would be to disallow declaring options outside of project.conf | 10:16 |
| valentind | I would be fine with it. | 10:16 |
| tristan | I'm not sure there is any justification for allowing that, project options and plugin origins should be restricted with project.conf | 10:16 |
| tristan | Includes from junctions I think is fair game, most important is that we have the right error messages triggered when circular things happen or unresolvable scenarios arise | 10:17 |
| tristan | that's pretty inline with what we already do | 10:18 |
| tristan | (with elements and variables) | 10:18 |
| tristan | benschubert, any chance of landing https://gitlab.com/BuildStream/buildstream/-/merge_requests/1973 today ? | 10:19 |
| benschubert | tristan: I'll do a first round of review in 30-60 minutes, sorry for the delays, it's been quite busy lately | 10:23 |
| tristan | "To enable design management, you'll need to meet the requirements. If you need help, reach out to our support team for assistance." | 10:26 |
| tristan | Who needs that, what kind of crazy is gitlab trying to sell today :-S | 10:26 |
| *** tristan has quit IRC | 10:41 | |
| *** tristan has joined #buildstream | 11:19 | |
| *** ChanServ sets mode: +o tristan | 11:19 | |
| *** radiofree has joined #buildstream | 11:32 | |
| radiofree | hi, has anyone seen this type of error when running two simultaneous copies of bst (on different folders) https://gitlab.com/celduin/buildstream-bazel/abseil-hello-fork/-/jobs/648568310 | 11:33 |
| tristan | concurrent BuildStream on the same host hasn't been very well supported :-S | 11:34 |
| radiofree | looks like an issue in the cas backend creating "/bst_cache/buildstream/logs/_casd" | 11:34 |
| radiofree | it usually works fine for me when the /.cache/buildstream/ is already there, it's only when initialising it from scratch i see this type of thing | 11:35 |
| tristan | in the bst1 days, it was mostly working except for cache expiry, which had session related knowledge baked in (i.e. which elements will be required for this session, shall not be purged from the cache) | 11:35 |
| radiofree | i think i'll just move the cache somewhere persistent so i'm less likely to see it | 11:35 |
| tristan | for bst2, I *think* it should be fixed with the new expiry approach | 11:35 |
| tristan | juergbi, any thoughts on this ? can we say that concurrent bst shall be supported in bst2 ? | 11:36 |
| tristan | interesting that this only happens when initially creating the cache | 11:37 |
| radiofree | i'll try and come up with an example *not* a bazel rules_buildstream, that happens to easily trigger it because i have two of them (one building an arm toolchain and one building an x86 toolchain), so bazel runs those jobs in parallel | 11:38 |
| juergbi | casd still uses a form of session expiry. not that easy to support concurrent sessions with a single cache directory | 11:38 |
| juergbi | that particular error can probably be fixed without a big issue, however, expiry is the main problem | 11:38 |
| juergbi | so until we fix that, I wouldn't recommend running two bst sessions at the same time | 11:39 |
| radiofree | ok! | 11:40 |
| juergbi | if you're sure expiry won't be triggered (no or very high quota and plenty of available disk space), it should work fine but in general it's not | 11:40 |
| juergbi | a prerequisite would be to support buildstream using an already running casd instance | 11:41 |
| tristan | That particular error looks like it only needs an `exists_ok=True` in os.makedirs() | 11:42 |
| juergbi | yes | 11:44 |
| juergbi | we could add support for opting out of expiry, which should make concurrent sessions safe, however, I don't know whether that would actually be useful in practice | 11:46 |
| juergbi | radiofree: in your particular case, an option could be to build arm+x86 in a single bst session. however, that might not be what you want with regards to bazel integration | 11:47 |
| radiofree | would a "bst show" or something be enough to spin up the cas? i could do that in the before section, but the point of the pipeline is to demonstrate a build so would rather not do the bst build outside of that | 11:50 |
| radiofree | it's not 100%, this one passed https://gitlab.com/celduin/buildstream-bazel/abseil-hello-fork/-/jobs/648568307 and it is doing exactly the same thing (at least initially) | 11:51 |
| tristan | bst show should definitely work with a concurrent build session I would think, even on the same project | 11:51 |
| radiofree | i am trying to limit the bst build to be dependent on the cpu requirement though, which should fix it (i.e there's no point building an x86 toolchain for an arm build) | 11:51 |
| tristan | this is something I've found myself doing a lot | 11:52 |
| radiofree | ok, will add that! | 11:52 |
| tristan | That said, I'm also interested (not immensely but still) in concurrent local build sessions | 11:52 |
| tristan | My usecase typically consists of (A) Running a build which will take all day with a limited number of cores allocated to it... (B) Testing changes to another project at the same time | 11:53 |
| tristan | I've done this before "at my own risk" knowing that I have enough space in my cache and knowing what could go wrong | 11:53 |
| tristan | but it's certainly in the realm of use cases "that happen" | 11:54 |
| radiofree | there's certainly appetite for that when using rules_buildstream in a bazel project (i.e define buildstream artifacts as bazel dependencies) since bazel will run off and run as many of them in parallel as it can (dependent on the number of --jobs) | 11:54 |
| tristan | hmmm yes, this looks very weird to me, running buildstream from bazel appears like the opposite of the use case I would imagine | 11:55 |
| tristan | I think of BuildStream as the orchestration tool and bazel as the build unit | 11:55 |
| tristan | still would be interesting to make work | 11:55 |
| radiofree | the usecase here is to build the toolchain you use to build everything else in the bazel monorepo (which could be an entire software stack) but bst in a known, reproducible, way, rather than relying on some random tarball from the internet/team-members laptop | 11:59 |
| radiofree | i wouldn't fancy sending the patch to gcc for "port gcc build to bazel" | 12:00 |
| tristan | benschubert, updated patch with a bit of splitting | 12:02 |
| tristan | benschubert, the only other thing really is the Variables.subst() signature change, we *could* separate that but it seems to be rather meaningless work, it also makes the rewrite patch subpar (looks like we're rewriting variables to have the same silly mistake in it) | 12:03 |
| tristan | in the bst-1 port, I've changed Variables.subst() to take a mandatory additional `Provenance` argument, which amounts to pretty much the same correction | 12:04 |
| benschubert | tristan: seems good thanks! Agreed, now seems a good compromise and already easier to review! | 12:07 |
| tristan | I think for this, it's better to just read _variables.pyx and glance over at and older copy for comparisons, than to observe a diff ;-) | 12:08 |
| tristan | a diff would look horribly confusing :) | 12:08 |
| tristan | s/over at and/over at a/ | 12:08 |
| tristan | Of course, I've left out all of the micro-optimizations from the slower path in the name of brevity, I'll keep that branch around locally in case we want to revisit that some day | 12:10 |
| *** jonathanmaw_ has joined #buildstream | 12:58 | |
| *** jonathanmaw has quit IRC | 12:59 | |
| benschubert | tristan: seems good :) | 14:34 |
| benschubert | tristan: finally finished the review, sorry for the delay | 16:04 |
| benschubert | a few 'nit', feel free to ignore them | 16:04 |
| benschubert | looks really good | 16:04 |
| *** jonathanmaw_ has quit IRC | 17:17 | |
| *** santi has quit IRC | 18:06 | |
| *** benschubert has quit IRC | 20:07 | |
| *** mohan43u has quit IRC | 22:20 | |
| *** mohan43u has joined #buildstream | 22:26 | |
| *** mohan43u has quit IRC | 22:33 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!