| *** swick has quit IRC | 01:06 | |
| *** swick has joined #buildstream | 01:09 | |
| *** nimish2711 has quit IRC | 02:17 | |
| *** swick has quit IRC | 02:18 | |
| *** swick has joined #buildstream | 02:22 | |
| *** nimish2711 has joined #buildstream | 03:05 | |
| *** nimish2711 has quit IRC | 03:11 | |
| *** tristan has joined #buildstream | 04:55 | |
| *** benschubert has quit IRC | 05:17 | |
| *** tristan has quit IRC | 07:28 | |
| *** tristan has joined #buildstream | 08:43 | |
| *** toscalix has joined #buildstream | 09:09 | |
| *** benschubert has joined #buildstream | 09:19 | |
| gitlab-br-bot | jennis opened MR !1235 (jennis/track_is_overworking->master: Don't track unchanged refs) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1235 | 09:19 |
|---|---|---|
| *** rdale has joined #buildstream | 09:20 | |
| *** alatiera has joined #buildstream | 09:26 | |
| benschubert | tristan: concerning https://gitlab.com/BuildStream/buildstream/merge_requests/1222#note_151827563 my only ocncern is that for people not familiar with the tests, creating a new file and seeing a warning about "cli" not being imported my throw them off :) I'm happy to change to disabling locally though :) | 09:30 |
| *** raoul has joined #buildstream | 09:35 | |
| *** tpollard has joined #buildstream | 09:36 | |
| *** jonathanmaw has joined #buildstream | 09:41 | |
| *** swick has quit IRC | 09:48 | |
| juergbi | Kinnison: I think you're misreading the table. you're commenting on a January/February difference | 09:48 |
| juergbi | or am I confused? | 09:49 |
| Kinnison | juergbi: oh am I? | 09:49 |
| Kinnison | doh | 09:49 |
| * Kinnison notifies the list of his incompetence | 09:50 | |
| raoul | I also was thinking that | 09:50 |
| * Kinnison sighs | 09:51 | |
| Kinnison | For some reason I was reading it as a twitter-style most-recent-at-the-top and entirely misread the date column as a result | 09:51 |
| * Kinnison is going to blame the fact that his magic is currently leaking out of a hole in his left arm | 09:51 | |
| *** ChanServ sets mode: +o tristan | 10:17 | |
| tristan | benschubert, I understand, I think that a new developer writing a new test will hit the linter, and will see that other tests have disabled these - and that this (possibly frequent) experience will be less evil than the (possibly infrequent) experience of scratching your head and wondering why behavior is different, for instance asking yourself why the linter is not catching the unused imports which you think it should be catching | 10:17 |
| tristan | benschubert, i.e. silent success feels more evil than loud false positives | 10:18 |
| tristan | if these were equal in weight, using the standard comments still saves us from having to carry the linter plugin at all, and having a simpler, more predictable setup also has value | 10:19 |
| benschubert | tristan: sure, I will remove the linter, and add the comments | 10:20 |
| lchlan | jonathanmaw: Hi, will you have some time today to do some reviews of benchmarking changes? | 10:31 |
| jonathanmaw | lchlan: probably, how urgently do they need looking at? | 10:32 |
| jonathanmaw | ah, this is a hypothetical, not a "right now" | 10:32 |
| lchlan | jonathanmaw: Not immediately urgent - but one of them is now of reasonable importance. | 10:34 |
| lchlan | https://gitlab.com/BuildStream/benchmarks/commits/lachlanmackenzie/AddTagToCleanupStage | 10:35 |
| jennis | We have the --option option, which takes OPTION VALUE, should we expect this to be able to override aliases defined in the project.conf? | 10:57 |
| tpollard | cli has precedence over .conf | 10:58 |
| tpollard | not sure about aliases specifically though | 10:58 |
| jennis | Ahh Kinnison, has suggested a string option, set this as your alias value and then you can override that | 10:58 |
| tpollard | sounds sensible | 11:00 |
| *** phildawson_ has joined #buildstream | 11:00 | |
| *** phildawson has quit IRC | 11:02 | |
| csoriano | hey all, I'm trying to build and generate a tarball for a gnome library called gnome-autoar | 11:05 |
| csoriano | for that, I did bst build, then entered a shell | 11:05 |
| csoriano | a build shell with bst shell --build core-deps/gnome-autoar.bst | 11:05 |
| csoriano | here, I expect make and make install to work as expected | 11:05 |
| csoriano | however with make install I get a "read-only" file system | 11:05 |
| csoriano | what's the expected workflow there? | 11:05 |
| tpollard | csoriano: do you just want a tarball of the built output? | 11:06 |
| csoriano | tpollard: for now yes, but I would also be interested in trying it for development of the library itself | 11:07 |
| tpollard | csoriano: bst artifact checkout has a --tar option :) | 11:10 |
| jennis | You can't define a string option, hmrf | 11:12 |
| csoriano | tpollard: ah, so it would be bst build --tar? | 11:12 |
| jennis | csoriano, `bst build core-deps/gnome-autoar.bst` and then, once it's built, `bst artifact checkout --tar sometarball.tar.gz core-deps/gnome-autoar.bst` | 11:14 |
| csoriano | gotcha, thanks! | 11:14 |
| csoriano | how about if I want to develop it? | 11:14 |
| jennis | bst workspace open core-deps/gnome-autoar.bst | 11:15 |
| csoriano | the make install fails, that was my main issue | 11:15 |
| jennis | then repeat the built and tarball checkout | 11:15 |
| csoriano | since it's a read-only system | 11:15 |
| jennis | so you've not been able to `bst build` it at all yet, I'd assume? | 11:16 |
| csoriano | I did | 11:16 |
| csoriano | fwiw I'm following https://wiki.gnome.org/Newcomers/BuildSystemComponent | 11:16 |
| jennis | So it just failed in the build shell? | 11:16 |
| csoriano | it says "To speed up development, you might wish to manually build your project rather than relying on bst build to do it for you. You may enter a build shell inside the build directory:" | 11:17 |
| csoriano | bst shell --build sdk/gtk+-3.bst | 11:17 |
| csoriano | (in my case it will be gnome-autoar) | 11:17 |
| csoriano | then there I do the regular make; make install; | 11:17 |
| csoriano | make install fails because it tries to install in a read-only filesystem | 11:17 |
| tristan | csoriano, I dont think make install with an empty DESTDIR ever worked | 11:18 |
| csoriano | what's the expected workflow there? | 11:19 |
| tristan | edit/compile/test with bst shell is still painfully slow | 11:19 |
| tristan | and mcatanzaro worked around this by playing inside a build shell | 11:19 |
| tristan | csoriano, the expected thing would be `bst workspace open`... and then `bst build` + `bst shell` after the build completes | 11:20 |
| csoriano | alright, so I guess that wiki documentation is slightly wrong | 11:20 |
| tristan | then the shell has it installed | 11:20 |
| csoriano | right, I tried "bst build" after the first build too, but it took around 10 minutes since seems it fetches all deps again right? | 11:21 |
| tristan | It's a bad example to do that with GTK+ :-S | 11:21 |
| tristan | I mean, something that you need not install, you might launch many times within the same shell | 11:21 |
| tristan | Or, you might use LD_LIBRARY_PATH tricks in there | 11:21 |
| csoriano | I see... that can be changed from the wiki too | 11:21 |
| csoriano | wait, I'm talking about libraries, you need to install those | 11:22 |
| csoriano | gnome-autoar is a library for example | 11:22 |
| csoriano | it expects to be installed where things can use it | 11:22 |
| tristan | csoriano, Well, `bst build` need not do "--track-all" every time; so you shouldn't techinically need to refetch things until you want new versions of them | 11:22 |
| *** nimish2711 has joined #buildstream | 11:22 | |
| csoriano | ah, that should work indeed | 11:22 |
| tristan | It's mostly the large amount of seconds it takes to launch a shell which is quite annoying for an edit/compile/test cycle | 11:23 |
| csoriano | you need to "bst shell" every time you build? | 11:23 |
| tristan | (can maybe be minutes if there are a lot of integration commands after staging too) | 11:23 |
| tristan | Yes, the new shell will stage the new combination of outputs, including the workspaced build | 11:24 |
| tristan | it's painfully slow to do, though | 11:24 |
| jonathanmaw | lchlan: reviewed benchmarks MR 27 | 11:24 |
| csoriano | I see... | 11:24 |
| tristan | We need to make integration commands only run when needed (that is a hard problem); staging is rather fast enough | 11:25 |
| tristan | Caching should be part of the solution | 11:25 |
| * csoriano wonders whether this is a good fit for currently contributing to GNOME libraries | 11:26 | |
| gitlab-br-bot | raoul.hidalgocharman opened MR !1238 (raoul/source-key-fix->master: Source key fix) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1238 | 11:27 |
| csoriano | and the trick with "bst shell" that you talked about kinda works? | 11:27 |
| csoriano | I wonder if we could document that better, and use that until caching and such is in place | 11:28 |
| tristan | I'm trying to think if we could have a read/write slash in that shell; that might help, but also slows things down | 11:30 |
| tristan | the thing about read-write slash is we need a fuse layer there | 11:30 |
| *** phildawson_ has quit IRC | 11:34 | |
| *** phildawson_ has joined #buildstream | 11:35 | |
| csoriano | let me try a few bst builds + bst shell, maybe it's not as slow | 11:42 |
| csoriano | in general I think it's better a slow process and a half-time broken process (as we had with jhbuild) | 11:42 |
| tristan | csoriano, I tend to agree and hope you have a better experience; mcatanzaro was developing webkit after all, which is really one of the highest bars to pass | 12:01 |
| tristan | even with incremental builds, its damn expensive and time consuming :-S | 12:02 |
| WSalmon | for developing a libary i thought the best way to go was to make a workspace then build some app that depends on it with weak cache keys set, then make the change in workspace and rebuild then run the app you are testing your lib with with bst shell ? | 12:02 |
| tristan | But I do suspect that we have too many integration commands inherited from freedesktop-sdk | 12:02 |
| tristan | WSalmon, yeah (that's called non-strict mode) | 12:03 |
| *** raoul_ has joined #buildstream | 12:03 | |
| tristan | WSalmon, if you were at the last GUADEC you would recall that Michael Catanzaro pointed out that it was just too slow | 12:04 |
| tristan | WSalmon, and he's right, when it comes to developing Epiphany web browser and hacking on WebKit builds... it does as you would expect, but launching the shell can take minutes | 12:04 |
| *** raoul has quit IRC | 12:04 | |
| tristan | Which... is damn annoying when you just added a printf() statement and want to know why something broke "right now" | 12:05 |
| tristan | I guess it's more similar to "Go rebuild your module on this build server which targets the right architecture, and then telnet over to the test hardware on your desk and put the binary on the machine, and restart the service" | 12:06 |
| tristan | rather than edit/compile/test on your host (in terms of speed) | 12:06 |
| tristan | WSalmon, I believe that with the optimizations done in master... and maybe some other effort to reduce how many integration commands we run... we will never be *as fast* as edit/compile/test on your host... BUT the features and security that we add will make it attractive enough for people to not mind that it takes a bunch of seconds more | 12:08 |
| tristan | Thats the bar to aim for I think | 12:08 |
| * tristan thinks that WSalmon was indeed at GUADEC now that he thinks of it :D | 12:09 | |
| WSalmon | tristan, you were righ the first time | 12:09 |
| WSalmon | were we talking about caching the intergration in a similar way to how we cache the sources? so if you put the dep you are going to mess with last it can cache up to that point? we do some funny stuff with source's order. did that work just get pushed back or is there a issue with it? | 12:11 |
| WSalmon | *not cached source, but the way we stage them is order dependent i think | 12:12 |
| tristan | WSalmon, I have an idea that we could have a test session active, and we could keep that sysroot up to date "as we build", so we would not have to run integration commands and stage things again when running `bst shell`, we would just get the last cached thing | 12:13 |
| tristan | but there are a lot of wholes in that plan | 12:13 |
| tristan | One idea was to run integration commands *after* a build instead of before, which can actually improve performance when say, many things depend on the same stack | 12:14 |
| WSalmon | dose the first fall under clever fuse magic we should do in the future? | 12:14 |
| tristan | However, whenever something depends on anything different, then integration would still have to happen before the build *also*, making things all around slower | 12:14 |
| tristan | WSalmon, Well... I think/hope that staging is not the bottleneck - running the integration commands is the heaviest | 12:15 |
| tristan | I suspect that with all the optimizations, even with full on fuse/BuildBox in master; staging will be faster than with 1.2.x | 12:16 |
| juergbi | staging is definitely faster when using buildbox-fuse as backend. however, build time will be slightly worse due to the FUSE overhead | 12:17 |
| tristan | WSalmon, So maybe we don't even need to cache an actual filesystem tree; but caching the results of integration commands (in whatever form) is rather what I was hoping could improve things | 12:17 |
| juergbi | integration should be a lot faster, though, due to faster FUSE implementation | 12:17 |
| tristan | It might be wise to have a cross-junction feature which says "This is my junction point" and take an integrated sysroot to depend on, only re-running the cross-junctioned integration commands at a later, explicit point in time | 12:18 |
| tristan | I don't know | 12:19 |
| tristan | (idea is not really junction related if you think about it, but still it's not a good idea) | 12:20 |
| tristan | Because `bst shell` wants to "run" things, while many of the integration commands are not necessary for a build environment, they become important for running | 12:20 |
| *** nimish2711 has quit IRC | 12:34 | |
| *** lachlan has joined #buildstream | 12:45 | |
| *** nimish2711 has joined #buildstream | 12:51 | |
| gitlab-br-bot | aevri opened (was WIP) MR !1236 (aevri/str_e->master: app.py: str(e) instead of "{}".format(e)) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1236 | 12:55 |
| *** nimish2711 has quit IRC | 13:01 | |
| *** nimish2711 has joined #buildstream | 13:02 | |
| *** raoul_ has quit IRC | 13:16 | |
| jonathanmaw | lchlan: thumbs-up for merge request 27, but I can't add an approval, and pushing the merge button won't work until merge conflicts are resolved | 13:20 |
| tristan | aevri, any reason to not just pass the provenance through _get_loader() as I did with _load_file() and friends ? | 13:33 |
| aevri | It generates a bunch of fuss - have you seen my patch for that? | 13:34 |
| * tristan looks :) | 13:34 | |
| aevri | :) | 13:34 |
| aevri | I've done it both ways, for your viewing convenience | 13:34 |
| tristan | err, same MR ? | 13:35 |
| aevri | yeah, earlier commit reverted | 13:35 |
| tristan | ah yes | 13:35 |
| aevri | I said that in the MR too :D | 13:35 |
| aevri | Also, I think in most other places folks seem to have re-raised - I found them when trying to replace with the context manager. | 13:36 |
| tristan | aevri, it's a bit wordy I admit | 13:36 |
| tristan | aevri, Is there not any chance that we screw things up with this context manager ? | 13:36 |
| tristan | I mean, what if we have recursed more than once here ? | 13:37 |
| aevri | You'd get multiple things added to the context in that case, as it unwound the stack | 13:37 |
| aevri | I imagine that would be closer what we actually want | 13:37 |
| tristan | Yeah obviously we'd want to protect against that ever happening | 13:38 |
| aevri | My objection to my own MR would be that it introduces some mystery | 13:38 |
| *** lachlan has quit IRC | 13:38 | |
| tristan | don't know if you saw my last comment https://gitlab.com/BuildStream/buildstream/merge_requests/1233#note_151985984 yet | 13:38 |
| aevri | I'm not sure - if you are including something that includes something that includes something, I'd like the full stack to make it in there | 13:38 |
| tristan | I don't think so, and I doubt that the compiler is a shining example of the most human readable and user friendly error messages :) | 13:39 |
| tristan | aevri, i.e. I think that we know for sure that regardless of how you included something; the error of including that thing would happen anyway | 13:40 |
| tristan | if that is the case, then I would hope that I only get the information about the error | 13:41 |
| aevri | For this context manager, I'm thinking you should only use it when you would otherwise have fed something down to be formatted into the error | 13:41 |
| aevri | I agree with your counter-examples - that's probably not the sort of thing we'd want | 13:41 |
| tristan | I think that passing down something else to be formatted into the error is an anti pattern, not something to encourage - well, in the case of the Loader it's all internal knowledge so that seems okay | 13:42 |
| tristan | aevri, i.e. I specifically only think it's not okay when you are giving a module information it shouldnt need to know about, or have access to | 13:42 |
| aevri | +1 I think we agree about not passing things down | 13:42 |
| tristan | can break encapsulation | 13:42 |
| WSalmon | juergbi, tristan, if i soft reset a workspace, and then build the element then I think it should do a build rather than just saying that its cached and only doing the full rebuild (inc configuration commands) the next time bst notices you have made a change in the workspace. i think the reset should act as a file change would, as you have done the reset for a reason, maybe your getting funny results and realised you should have soft reset for a | 13:43 |
| WSalmon | old change, etc.... | 13:43 |
| WSalmon | have i missed something? | 13:44 |
| aevri | Yeah that would be a hard break, I think a softer kind of breakage happens when things start growing parameters to raise information upwards | 13:44 |
| laurence | reminder: the monthly team meeting is starting in 15 minutes over on #buildstream-meetings | 13:45 |
| tristan | aevri, so I actually think that for the stop-gap fix, I prefer the more wordy/noisy commit - it is explicit and we see what is going on (perhaps some different formatting could make it less messy but that's not such a concern for formatting a nice error) | 13:45 |
| tristan | aevri, my earlier question about recursion more than we need to, was actually specifically about this _get_loader() patch | 13:45 |
| tristan | aevri, I think it is actually possible that you get multiple provenances from different levels of junctions with this re-raise thing | 13:45 |
| aevri | tristan, yeah I did see the recursion there too | 13:46 |
| aevri | Is it not better that you get nested provenance in the error? | 13:46 |
| tristan | No | 13:46 |
| tristan | aevri, not unless you are explicitly catching it in some way and making the error better, I think | 13:46 |
| aevri | So if it was somehow nested 10 deep, we'd only get the last bit of provenance - that seems less useful to me | 13:47 |
| tristan | In this case, you would have foo.bst [line 1 column 10]: junction.bst:bar.bst [line 4 column 4]: something about the sub-sub project baz.bst not having a project.conf | 13:47 |
| aevri | https://www.irccloud.com/pastebin/EtKvUA7L/ | 13:49 |
| tristan | aevri, we want to know where the error originated, and what the project.conf-less project is | 13:49 |
| aevri | Damn, wrong button - not sure how that shows in your client | 13:49 |
| tristan | aevri, I feel like in that specific case where if it is hard to find, it would be better for the code to know about it and provide a stack in the detail | 13:50 |
| aevri | Yeah, we could do that | 13:50 |
| tristan | aevri, but prepended provenances into an error string is really bad form | 13:50 |
| aevri | An alternative I considered would have used __cause__ to figure that stuff out. | 13:51 |
| tristan | sounds like a fancy pythonic thing :) | 13:51 |
| aevri | tristan, I think the formatting might be bad, but surely the extra information is good? | 13:51 |
| tristan | interesting question | 13:52 |
| aevri | Yeah, basically whenever you 'raise Blah from e' then your new exception gets 'e' as the __cause__ | 13:52 |
| tristan | aevri, there is also the question of whether the sub-sub project is coaleased with an existing junction in the toplevel project | 13:52 |
| aevri | You can follow that chain all the way back if you want. I didn't find a guarantee that we're "allowed" to do that by the standard yet though. | 13:53 |
| aevri | tristan, not quite sure what you mean there by coaleased | 13:53 |
| tristan | aevri, i.e. when you depend on a project "foo" and your subproject depends on the same project "foo", you use a junction of the same name to ensure that they are the same version of the same project | 13:54 |
| tristan | aevri, in that case, the toplevel knows what that sub-sub-project is, even if it was indirectly reached | 13:54 |
| aevri | Ah, a bit like the dreaded diamond dependency. | 13:54 |
| tristan | but that is rather unimportant | 13:54 |
| tristan | Well, not really, I think the diamond is only dreaded in OO inheritance models :) | 13:55 |
| tristan | in software they occur everywhere | 13:55 |
| tristan | (I mean in build graphs) | 13:55 |
| aevri | I think there's always the philosophical question of whether it's the same base thingy though, and fun ensues if it isn't :) | 13:56 |
| tristan | aevri, So to cut this short: I agree that it can be useful information, I do think though that mechanical prepending of provenance through the stack does not really speak to the user | 13:56 |
| tristan | it would be nicer to always format errors which speak to the user directly | 13:56 |
| aevri | Cool, I agree with that if we talking about applying it wider than I have in the MR. | 13:57 |
| aevri | I 'fixed' all the occurrences I found, so I'm not sure it would go much wider. | 13:57 |
| aevri | I agree about speaking to the user directly. | 13:58 |
| aevri | If there were only some way to tack on useful things as it bubbles up. Perhaps I'll propose something that surely looks nice to the user, i.e. we don't get a long chain of 'a: b: c: d: thing'. Does that sound agreeable? | 13:59 |
| laurence | meeting starting now | 14:00 |
| laurence | over on #buildstream-meetings | 14:00 |
| laurence | nb all ^ | 14:00 |
| *** nimish2711 has quit IRC | 14:02 | |
| *** nimish2711 has joined #buildstream | 14:02 | |
| tristan | aevri, As a side note, I think you might be stepping on how provenance was changed | 14:02 |
| tristan | aevri, note that provenance now *also* includes the junction through which it was included | 14:02 |
| tristan | aevri, maybe *that* can be extended so that there is a single provenance that is recursion friendly | 14:03 |
| aevri | I'm thinking: | 14:03 |
| aevri | something about the sub-sub project baz.bst not having a project.conf | 14:03 |
| aevri | This was loaded from (most recent last): | 14:03 |
| aevri | junction.bst:bar.bst [line 4 column 4] | 14:03 |
| aevri | foo.bst [line 1 column 10] | 14:03 |
| aevri | tristan: Yeah maybe, will add that to the list | 14:03 |
| aevri | tristan: thanks | 14:04 |
| *** raoul_ has joined #buildstream | 14:04 | |
| *** nimish2711 has quit IRC | 14:07 | |
| *** nimish2711 has joined #buildstream | 14:07 | |
| *** lachlan has joined #buildstream | 14:12 | |
| *** nimish2711 has quit IRC | 14:22 | |
| *** nimish2711 has joined #buildstream | 14:23 | |
| *** nimish2711 has quit IRC | 14:28 | |
| *** nimish2711 has joined #buildstream | 14:28 | |
| *** cs-shadow has joined #buildstream | 14:34 | |
| *** nimish2711 has quit IRC | 14:38 | |
| *** nimish2711 has joined #buildstream | 14:38 | |
| *** nimish2711 has quit IRC | 14:43 | |
| *** nimish2711 has joined #buildstream | 14:44 | |
| benschubert | tristan: looking at https://gitlab.com/BuildStream/buildstream/merge_requests/1070#note_151834877, while I agree that reusing things is much better, in that case this mixes things up and puts a constraint here that has no meaning / wrong meaning for other stuff likes Sources. | 14:48 |
| *** nimish2711 has quit IRC | 14:49 | |
| tristan | benschubert, You are adding 4 bytes on every instantiated element, while a counter already exists - I think it makes sense to use the existing counter unless we have a reason for it to be incompatible | 15:02 |
| tristan | benschubert, I do see your point, but if clarity is all that is called for, we could also add a separate internal function on Element which just returns self._get_unique_id() | 15:02 |
| tristan | the implementation could change at some further point if needed ? | 15:03 |
| benschubert | - 4 bytes + an indefinite number of bytes, ints are not 4 bytes in python :) | 15:03 |
| benschubert | Mmh I would rather not add a new function, function calls have a cost :) But yeah ok, i'll make a good comment on top of Plugin to say it's used in elements so the ordering matters. Good for you? | 15:03 |
| tristan | Has the same drawbacks too afaics; if you run multiple pipelines instantiated in the same session, the second pipeline will not start at "1" or "0" either :) | 15:04 |
| tristan | benschubert, yeah it *definitely* needs a big comment in plugin.py making it very clear that it is relied upon in this way | 15:04 |
| * tristan thinks that was part of his comment on the issue | 15:04 | |
| benschubert | ok, so if you're fine with this I'll go for this. I'll rework it slightly too first | 15:05 |
| * tristan has not caught up with the dozens of other comments | 15:05 | |
| juergbi | benschubert: regarding the first commit, that does include indirect runtime dependencies (due to using the default recurse=True) | 15:05 |
| juergbi | maybe it still breaks something, though | 15:05 |
| benschubert | juergbi: ah right | 15:05 |
| benschubert | juergbi: the PR is quite complex already and there's a lot of stuff moving. Would be simpler to just merge the two commits, otherwise It's tricky to know where to put the stuff | 15:06 |
| tristan | I would definitely not like to include the pivot in mainline history | 15:07 |
| tristan | The history that something else was tried doesnt need to land in mainline | 15:07 |
| juergbi | I don't see it as pivot but as incremental step | 15:07 |
| * tristan has to go | 15:07 | |
| juergbi | which means the two commits are each simpler than the combined one | 15:07 |
| tristan | juergbi, Perhaps, if you like | 15:07 |
| juergbi | but I won't fight this. not such a big deal | 15:07 |
| tristan | Me neither actually - I do see it as a pivot | 15:08 |
| benschubert | juergbi: tristan I'll see what's simpler for me to handle while addressing the comments then | 15:08 |
| benschubert | and merge or not at the end | 15:08 |
| * tristan has been trying to make sure that we only do single level recursion though one simple function and we dont store recursive lists | 15:08 | |
| tristan | I.e. we want a graph we can recurse in both ways over, then we can build on that for other reasons | 15:08 |
| juergbi | have you two had a chance to look at my suggested simplification? | 15:08 |
| * tristan late... gotta go... | 15:09 | |
| benschubert | I'll add fixups commits for every comment as it's simpler than modifying commits. Will squash at the end | 15:09 |
| juergbi | tristan: true, saving the recursive deps is somewhat of a pivotal point | 15:09 |
| benschubert | juergbi: for the no weak_cache_key? | 15:09 |
| juergbi | benschubert: I mean https://gitlab.com/BuildStream/buildstream/merge_requests/1070#note_151862404 | 15:09 |
| benschubert | juergbi: ah | 15:11 |
| benschubert | so | 15:11 |
| *** tristan has quit IRC | 15:12 | |
| benschubert | juergbi: why do you have a "__reverse_key_updated" ? | 15:13 |
| benschubert | __reverse_deps_updated sorry | 15:13 |
| juergbi | to only trigger it when the ready state actually changed | 15:13 |
| benschubert | well, this can only happen once | 15:14 |
| benschubert | so why do we need to check? | 15:14 |
| *** nimish2711 has joined #buildstream | 15:15 | |
| juergbi | the change in readiness can only happen once but the same element might be in the priority queue multiple times over a whole session, afaict | 15:15 |
| benschubert | juergbi: but then, we know if something has changed by checking the key before and after _update_state() | 15:15 |
| benschubert | also "rdep.__remaining_runtime_deps_cache_keys_to_discover -= 1" needs to happen in _update_state | 15:16 |
| juergbi | that won't detect the case where the key is known for a while already but remaining runtime drops just dropped to 0 | 15:16 |
| benschubert | since we are calling update_state in the pipeline, not recursively | 15:16 |
| *** swick has joined #buildstream | 15:16 | |
| juergbi | well, the tests pass for my branch | 15:16 |
| juergbi | do you have a scenario in mind that would break, or suspect where it would be slower? | 15:17 |
| benschubert | then we have a bug in the tests | 15:17 |
| juergbi | with the pipeline call, do you mean the initial update state? | 15:18 |
| benschubert | yes | 15:18 |
| juergbi | but that will anyway go over all elements | 15:18 |
| benschubert | tests/frontend/workspace.py::test_build_all, if we opened a workspace on elem2.bst, I think that would break | 15:18 |
| juergbi | ah, I see what you mean | 15:18 |
| benschubert | juergbi: basically, we need to tell our rdeps we are ready as soon as the cache_key is discovered | 15:18 |
| csoriano | once a workspace is open, is it needed to close it and open again after a build? | 15:19 |
| juergbi | csoriano: no, incremental build is supposed to work | 15:19 |
| csoriano | juergbi: however, bst build needs to be executed outside that shell right? | 15:19 |
| *** nimish2711 has quit IRC | 15:19 | |
| juergbi | yes, you need to exit the bst shell but not close the workspace | 15:20 |
| csoriano | so you would have two terminals, one with the workspace open inside a bst shell, other doing the bst build | 15:20 |
| csoriano | I see, thanks | 15:20 |
| *** nimish2711 has joined #buildstream | 15:20 | |
| juergbi | the open shell will not be updated | 15:20 |
| csoriano | ah wait, the workspace open is done only once right? | 15:21 |
| csoriano | so it downloads the git repo and sets somewhere in a config file that instead of the module frm buildstream it should use the local one | 15:22 |
| juergbi | yes, you normally open the workspace only once | 15:22 |
| csoriano | ok, thanks | 15:22 |
| juergbi | and you can keep it open as long as you want to work on that element | 15:22 |
| benschubert | juergbi: do you see my point? I'm not sure how to test it though | 15:23 |
| juergbi | I'm not sure it really breaks anything, but maybe it would block some jobs longer than necessary | 15:24 |
| juergbi | wondering whether it would make sense to trigger the recursive update state during pipeline initialization instead | 15:25 |
| benschubert | juergbi: if you try the test before and open a workspace on elem2, I think it blocks everything | 15:25 |
| benschubert | the update_state will go through everything, I would not call it recursively here | 15:25 |
| benschubert | in addition, I think it would make splitting update_state after harder | 15:25 |
| *** lachlan has quit IRC | 15:26 | |
| juergbi | benschubert: hm, but the equivalent of update_state_recursively with the priority queue prefilled with the pipeline elements would work, wouldn't it? | 15:26 |
| benschubert | juergbi: that would be slower, as we would try to add everything everytime to the queue | 15:26 |
| juergbi | no, not everytime, only during pipeline init | 15:27 |
| csoriano | juergbi: but I need to close and open a bst shell everytime I build right? | 15:27 |
| benschubert | I mean for every iteration, every reverse dep would try to be added | 15:27 |
| juergbi | benschubert: we could split the function in two parts, one for the loop body and one for the rest | 15:27 |
| juergbi | the latter we could make a static method and add a parameter for the initial priority queue contents | 15:28 |
| juergbi | well, don't even have to split the method for that | 15:28 |
| juergbi | anyway, for pipeline init we call the method with all pipeline elements | 15:28 |
| benschubert | juergbi: we would still need to check everytime we need to add something and we would fail | 15:28 |
| juergbi | otherwise we only call it with the relevant element as starting point | 15:28 |
| juergbi | what do you mean with "everytime we need to add something"? | 15:30 |
| benschubert | the update_recursively adds every element that could be updated to the list | 15:30 |
| benschubert | so for each element, for each rdep, we would try adding it during the first run | 15:30 |
| benschubert | and I don't want that | 15:30 |
| juergbi | i.e., you expect significant overhead due to that? | 15:31 |
| juergbi | or would it conflict with future work? | 15:31 |
| benschubert | yep | 15:31 |
| benschubert | both | 15:31 |
| juergbi | hm :/ | 15:32 |
| *** raoul_ has quit IRC | 15:32 | |
| benschubert | because for all non-build operations, we are doing this anyways and this will slow down every invocation of "show" for instance | 15:32 |
| benschubert | and I'd rather not do that | 15:32 |
| benschubert | having buildstream awefully slow for build is one thing, having it slower for any user interaction is worse | 15:32 |
| benschubert | so I would take the complexity hit there, rather than slow down stuff | 15:33 |
| benschubert | however, we can actually do it non recursively in _update_state i think | 15:33 |
| juergbi | it's just that the branch makes the already complex thing even more complex :/ | 15:35 |
| benschubert | juergbi: but allows for simpler simplification | 15:35 |
| benschubert | by having clearly defined change points | 15:35 |
| benschubert | meaning we can remove update_state and update-state_recursively with less effort AND gradually | 15:35 |
| juergbi | well, given that remaining deps is relevant in two places, I'm not so convinced about that yet | 15:35 |
| juergbi | and first adding more complexity makes it even harder to reason about further changes | 15:36 |
| benschubert | I disagree that the code is more complex with the changes, because we now have clearly defined boundaries | 15:36 |
| benschubert | and we can know what changes where | 15:36 |
| benschubert | and thus which action to take | 15:36 |
| benschubert | which is not the case when we are calling _update_state in every queue | 15:37 |
| juergbi | yes, I'm definitely in favor of that | 15:37 |
| juergbi | can we maybe measure the performance hit of what I suggested above? | 15:38 |
| juergbi | I think not having unclear different invocations of update_state_recursively and non-recursive would help | 15:38 |
| *** nimish2711 has quit IRC | 15:40 | |
| *** nimish2711 has joined #buildstream | 15:40 | |
| benschubert | juergbi: what I don't like with your change is that calling _update_state would result in a different behavior than update_state_recursively | 15:41 |
| benschubert | which it shouldn't | 15:42 |
| benschubert | and there are cases where we still need to not call it recursively (fetch_done) | 15:42 |
| juergbi | benschubert: that's the case with your branch as well, isn't it? | 15:42 |
| benschubert | juergbi: the behavior is the same on my branch | 15:42 |
| juergbi | update_state() doesn't trigger update_state() on reverse deps | 15:42 |
| benschubert | juergbi: yes | 15:42 |
| juergbi | while update_state_recursively() does | 15:42 |
| juergbi | no | 15:43 |
| benschubert | but calling update_state only will not break the counting | 15:43 |
| benschubert | which it would with your change | 15:43 |
| juergbi | my current patch might not decrement the counter early enough (still not sure whether it's an issue) | 15:44 |
| juergbi | however, with the suggested change above, the idea would be that pipeline also calls update_state_recursively | 15:44 |
| benschubert | and I don't think that should be done, because looping over elements and updating the states is the behavior that we need there | 15:46 |
| benschubert | I understand it would be simpler | 15:47 |
| benschubert | but it would add some overhead | 15:47 |
| benschubert | juergbi: however, we can actually remove the recursion | 15:49 |
| benschubert | gah no we can't | 15:50 |
| *** tristan has joined #buildstream | 15:50 | |
| benschubert | my concern in joining both at the same place is that they have different triggering mechanisms and different effects | 15:50 |
| benschubert | they are conceptually different | 15:51 |
| *** ChanServ sets mode: +o tristan | 15:59 | |
| tristan | Coffee shop closed and went and obtained some dinner, I see there is a big conversation here | 15:59 |
| juergbi | benschubert: I'd be happy with two separate recursions if they were indeed orthogonal. but right now in the MR remaining deps counter is used in both recursions | 15:59 |
| csoriano | apart from build, can I run the tests? For example, I would check whether my tests are good with the changes I did | 16:00 |
| csoriano | basically, I'm not sure how to go from "bst build" to do more commong things like parameters to the build, tests, etc. | 16:01 |
| tristan | csoriano, For that you can use a --build shell (or you could add a `make check` to the element .bst file) | 16:01 |
| tristan | csoriano, make check commands were not added as a default in the hopes that we can parallelize this kind of test with reverse dependency builds | 16:02 |
| csoriano | gotcha, thanks | 16:05 |
| tristan | Actually, looking at __update_state_recursively(), it looks like again we are deviating from a simple function which handles one level at a time, I thought the point of the PQ was to order which deps get recursed into first; not to move the whole problem of building this list from one place to another | 16:07 |
| benschubert | tristan: not sure I get your point | 16:08 |
| tristan | benschubert, enqueue_skiplevel() is building a queue which builds a list of what it's going to iterate into, and it appears to be recursive itself | 16:09 |
| tristan | I mean, wasnt the whole point of this pivot, to move the code towards something where we have a comprehensive one-level recursion which walks the tree ? | 16:09 |
| tristan | or graph rather | 16:10 |
| *** nimish2711 has quit IRC | 16:10 | |
| benschubert | tristan: yes | 16:10 |
| *** nimish2711 has joined #buildstream | 16:11 | |
| benschubert | however, we have a skip level iteration to do when runtime_only deps are involved | 16:11 |
| benschubert | and they can involve more stuff | 16:11 |
| tristan | benschubert, So how come __update_state_recursively() doesnt call __update_state_recursively() ? | 16:11 |
| benschubert | because we can flatten and avoid recursive update | 16:11 |
| benschubert | which is more efficient | 16:12 |
| tristan | :-S | 16:12 |
| benschubert | ? | 16:12 |
| * tristan thought the point of this PQ was just to decide the order of which direct rdep to recurse into, not to build the whole list | 16:13 | |
| benschubert | tristan: recursion is just a loop | 16:13 |
| benschubert | using the PQ, we ensure we update elements only once | 16:13 |
| *** raoul_ has joined #buildstream | 16:14 | |
| tristan | I see what you're doing, it's not terrible; but I'm not sure how well this scales for other operations we might want to do reverse recursively either; also it's late and I dont have the energy | 16:15 |
| tristan | I just worry (a lot) that we are sacrificing simplicity too much | 16:15 |
| benschubert | " how well this scales for other operations we might want to do reverse recursively" what do you mean? | 16:15 |
| tristan | benschubert, I mean it's a *lot* of code for the custom specific case of doing reverse dependency recursion for discovering the cache key | 16:16 |
| benschubert | tristan: how would you do it otherwise? | 16:17 |
| tristan | benschubert, in the abstract, this is just a graph, now we are adding recursion in both directions, maybe there are other cases, will every case need this much code ? | 16:17 |
| benschubert | tristan: the other way around should just not exist, but that requires major rework. we need to take intermediate step or we won't be able to end up in a sane position | 16:17 |
| *** nimish2711 has quit IRC | 16:18 | |
| tristan | There are two directions, iteration from target to leafs will always have relevance, now we add reverse iteration | 16:19 |
| benschubert | tristan: yes, reverse iteration is the only thing that should matter after building the pipeline anyways correct? | 16:21 |
| tristan | Nope | 16:21 |
| benschubert | In an ideal pipeline, not speaking about what we have now | 16:21 |
| tristan | A plugin has various reasons to iterate over its dependencies | 16:21 |
| tristan | Like observing public data exposed by those | 16:21 |
| tristan | Staging | 16:21 |
| tristan | Etc | 16:21 |
| tristan | That is always going to be relevant | 16:22 |
| benschubert | fair enough | 16:22 |
| benschubert | then we need both | 16:22 |
| benschubert | tristan: I'm not sure why you are opposed to us keeping the reverse dependencies and iterating over those | 16:24 |
| tristan | Ok so, with regular target -> leaf iteration, we already ensure that we only traverse every node once, this is rather straight forward already | 16:24 |
| tristan | In reverse we cannot do that without a list | 16:24 |
| tristan | I should accept that I suppose | 16:24 |
| benschubert | exact | 16:24 |
| csoriano | tristan: you sure it's "bst artifact checkout --tar sometarball.tar.gz core-deps/gnome-autoar.bst"? It says bst artifact doesn't exists | 16:25 |
| csoriano | (opening a shell and then make dist or ninja dist also work fwiw) | 16:25 |
| tristan | benschubert, Is there a way that we can condense this into a generic loop which takes a callable which returns true/false depending on whether it's branches should be followed ? | 16:25 |
| tristan | Or would it not make sense ? | 16:25 |
| tristan | csoriano, use `bst checkout --tar` instead | 16:26 |
| tristan | csoriano, people are working on BuildStream2 now, where we have changed some APIs around | 16:26 |
| tristan | with master it is `bst artifact checkout` | 16:27 |
| benschubert | tristan: I think this would be sub optimal for multiple reasons: | 16:27 |
| benschubert | 1) for what we are doing currently, we would need to run twice, once with build_rdeps once with run_rdeps, which is less flexible than doing manually | 16:27 |
| benschubert | 2) once we manage to split the _update_state, the logic will also be different for every entry | 16:27 |
| benschubert | 3) the only other place where I see reverse deps useful is for a push-based pipeline, which would only require direct rdeps | 16:27 |
| benschubert | Does that make sense? | 16:27 |
| tristan | csoriano, I don't recall if the filename works for .tar.gz, I think it does automatically because python | 16:27 |
| tristan | csoriano, anyway I think it does, but we explicitly support `-` for streamed output if that's useful | 16:28 |
| * tristan hopes we can get ostree to commit from tarball over stdin one day... | 16:29 | |
| csoriano | not sure I understood that, you mean that the filename might not work with .tar.gz? | 16:29 |
| tristan | benschubert, fair enough, I'll stop interrupting the conversation you had at this point because I honestly cannot meaningfully contribute | 16:30 |
| tristan | csoriano, Right, I mean maybe it will still give you a tarball, not a gzipped tarball | 16:30 |
| benschubert | tristan: thanks for the insights | 16:30 |
| benschubert | tristan: I'm just not sure in which direction to head anymore x) will take a break and re-read all the discussions | 16:30 |
| csoriano | oh | 16:30 |
| * tristan has been at it for like 12hours today due to the late night meeting | 16:30 | |
| tristan | so I'm a bit numb sorry | 16:30 |
| csoriano | oh :D | 16:31 |
| csoriano | https://www.irccloud.com/pastebin/KC27O8lN/ | 16:31 |
| csoriano | so I guess this is the issue you meant? | 16:31 |
| tristan | no that looks much worse :'( | 16:31 |
| * tristan cries | 16:32 | |
| benschubert | csoriano: what command did you run? And what version of buildstream? | 16:32 |
| *** raoul_ has quit IRC | 16:32 | |
| *** phil has joined #buildstream | 16:32 | |
| csoriano | bst checkout --tar core-deps/gnome-autoar.bst gnome-autoar-0.2.4.tar.gz | 16:32 |
| csoriano | [csoriano@localhost gnome-build-meta]$ bst --version | 16:32 |
| csoriano | 1.2.4 | 16:32 |
| *** phildawson_ has quit IRC | 16:33 | |
| tristan | yeah that looks right, I will have to add a test and fix it | 16:33 |
| tristan | missing artifacts are the worst | 16:33 |
| csoriano | wanna me to report an issue? | 16:33 |
| tristan | csoriano, Actually, it is very possible that this bug is fixed by the branch we are discussing right now | 16:33 |
| csoriano | and this is upstream buildstream, not gnome-build-meta right? | 16:33 |
| tristan | very very possible | 16:33 |
| csoriano | ah ok | 16:33 |
| tristan | csoriano, Do you run bst from git ? | 16:34 |
| tristan | or via pip/package manager ? | 16:34 |
| benschubert | tristan: 1.2.4 should be pip/package manager, otherwise we would have the sha no? | 16:35 |
| csoriano | tristan: fedora | 16:35 |
| tristan | benschubert, if you checkout a tag it will be the same | 16:35 |
| tristan | sha is only if it is not exactly at the tag | 16:35 |
| csoriano | + bst-external from pip3 | 16:35 |
| csoriano | I followed the guide basically | 16:35 |
| tristan | csoriano, Ah right, I was going to suggest trying it with https://gitlab.com/BuildStream/buildstream/tree/tristan/fix-workspace-build-all | 16:35 |
| tristan | Yeah, I am fairly certain you are hitting https://gitlab.com/BuildStream/buildstream/issues/919 | 16:36 |
| tristan | different symptom of the same problem | 16:36 |
| benschubert | gah, my PR really touches many things x) | 16:36 |
| tristan | I have a patch that works, but benschubert's is better | 16:37 |
| tristan | csoriano, I bet that if you close the workspace, and point the element to a committed change, it will work around the problem | 16:38 |
| tristan | I know, it's not ideal :-/ | 16:39 |
| csoriano | I will go with the shell make dist for now, I was checking wheter I should put the bst checkout stuff in the wiki, but looks like better wait for builstream 2 | 16:39 |
| tristan | csoriano, bst checkout --tar will work with 1.2.5 which will drop in like, a week | 16:40 |
| tristan | csoriano, bst 2 has not release date, not a rewrite, but a huge development effort | 16:40 |
| tristan | csoriano, explanation here: https://blogs.gnome.org/tvb/2019/03/04/buildstream-news-and-2-0-planning/ | 16:41 |
| csoriano | I see | 16:41 |
| csoriano | anyway, I reworked most of https://wiki.gnome.org/Newcomers/BuildSystemComponent if someone here could do a look there and try to follow it and do further corrections that would be nice | 16:41 |
| csoriano | specially for the expected workflow for common things | 16:41 |
| * tristan will check tomorrow :) | 16:42 | |
| csoriano | nice, cheers | 16:42 |
| tristan | csoriano, Really thanks for taking a look at the wiki and trying it out - I'm going to be spending my time making GNOME VM images with gnome-build-meta so will update wiki more once I get something half decent | 16:42 |
| csoriano | np! | 16:43 |
| *** tchaik[m] has joined #buildstream | 16:49 | |
| benschubert | juergbi: I might have a simpler solution without having to count | 16:54 |
| benschubert | might be a tad slower | 16:54 |
| * juergbi likes simple :) | 16:55 | |
| *** alatiera has quit IRC | 17:05 | |
| *** phil has quit IRC | 17:06 | |
| *** raoul_ has joined #buildstream | 17:06 | |
| benschubert | juergbi: juergbi https://gitlab.com/BuildStream/buildstream/merge_requests/1070/diffs?commit_id=54b60ad2030a42b6186866d85f2ae4f99e5aa477 what do you think? | 17:06 |
| benschubert | all tests pass | 17:06 |
| benschubert | tad slower | 17:06 |
| benschubert | but much simopler | 17:07 |
| benschubert | and only slower when running workspaces stuff | 17:07 |
| benschubert | and still easy to rework | 17:07 |
| *** alatiera has joined #buildstream | 17:09 | |
| juergbi | benschubert: I definitely like the reduced complexity | 17:13 |
| juergbi | but still thinking whether it will always work | 17:13 |
| juergbi | the case where a workspace is a runtime-only dependency of a build-only dependency | 17:13 |
| juergbi | building the workspace will trigger update state in the runtime reverse dependency | 17:14 |
| juergbi | however, that would not bubble up to the build-only reverse dependency because the cache key of that middle element wouldn't be newly discovered | 17:14 |
| juergbi | should probably create a test case for this | 17:16 |
| juergbi | but I thought that was already covered | 17:16 |
| juergbi | maybe I'm just confused ;) | 17:16 |
| benschubert | that will be tests/frontend/workspace.py::test_build_all | 17:16 |
| gitlab-br-bot | martinblanchard opened (was WIP) MR !1239 (mablanch/629-remote-execution-test->master: Add remote execution tests to the CI pipeline) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1239 | 17:19 |
| benschubert | juergbi: with the second parameter | 17:21 |
| benschubert | Does that seem good or am I missing something? | 17:21 |
| juergbi | it's not quite that test due to the stack in the middle | 17:25 |
| juergbi | trying to modify and test | 17:27 |
| benschubert | sure! | 17:27 |
| benschubert | If you can find a test case that fail, please send a commit ! | 17:27 |
| juergbi | will do | 17:28 |
| gitlab-br-bot | raoul.hidalgocharman opened issue #965 (Artifact as a Proto: protos and service) on buildstream https://gitlab.com/BuildStream/buildstream/issues/965 | 17:34 |
| gitlab-br-bot | aevri opened MR !1240 (aevri/element_loaderror_detail->master: element: keep original 'detail' when re-raising) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1240 | 17:37 |
| gitlab-br-bot | aevri opened (was WIP) MR !1237 (aevri/junction_load_provenance->master: loader: re-raise _get_loader LoadError with prefix) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1237 | 17:39 |
| juergbi | benschubert: https://gitlab.com/BuildStream/buildstream/commit/9bcbc2bbb6fb4aabf50e22807cda74b5fc114f65 | 17:44 |
| juergbi | fails with your simplification but passes on master, your previous version, and my branch | 17:44 |
| benschubert | gah ok thanks! | 17:45 |
| juergbi | maybe it should be a different test case, not sure | 17:45 |
| benschubert | juergbi: it seems like it might be better | 17:45 |
| benschubert | but that's already good I'll get that in my branch and try fixing | 17:45 |
| juergbi | ta | 17:46 |
| benschubert | thanks! | 17:46 |
| benschubert | juergbi: fixed version at !1070, your opinion? | 17:51 |
| gitlab-br-bot | MR !1070: WIP: Refactor _update_state() to be called only when needed https://gitlab.com/BuildStream/buildstream/merge_requests/1070 | 17:51 |
| juergbi | that was quick | 17:52 |
| gitlab-br-bot | jonathanmaw opened MR !1241 (jonathan/wsl-tests-manual->master: gitlab-ci: Make WSL tests only run on master) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1241 | 17:52 |
| benschubert | juergbi: it's a "simple fix" will do too much work, but not too much and maximum one level too deep | 17:54 |
| juergbi | benschubert: hm, what about going a bit more into the direction of my simplification with a single trigger | 17:55 |
| juergbi | i.e., what if we change ready_for_runtime to also cover the availability of the cache key | 17:56 |
| juergbi | and thus make it that 1-bit hash we discussed last week | 17:56 |
| benschubert | juergbi: afaik it already is since it will only be populated is __cache_key is not None :) | 17:57 |
| juergbi | true | 17:57 |
| *** lachlan has joined #buildstream | 17:57 | |
| juergbi | so couldn't we use this as the only trigger? | 17:57 |
| juergbi | element.__ready_for_runtime != old_ready_for_runtime: | 17:57 |
| juergbi | drop the cache key change check? | 17:57 |
| benschubert | juergbi: my only question is: for "weak_cache_key". We _can_ discover stuff before right? So we might be able to unblock weak_cache_keys earlier with my current version. Correct? | 17:58 |
| juergbi | yes, it's possible that we would unblock reverse dependencies later in non-strict mode in certain cases | 17:59 |
| benschubert | which cases? | 17:59 |
| juergbi | actually, maybe none | 18:00 |
| juergbi | as you can't build reverse dependencies anyway before the strong cache key is available | 18:00 |
| juergbi | you could calculate the weak cache key earlier but that doesn't gain you much | 18:01 |
| juergbi | tbh, overall I'm also less worried about slightly suboptimal scheduling for non-strict build plans | 18:01 |
| *** jonathanmaw has quit IRC | 18:02 | |
| benschubert | because they are not the default and, that would only be kicking in in addition to a workspace, correct? | 18:02 |
| juergbi | yes. it's difficult to estimate whether there would be any significant impact | 18:03 |
| juergbi | the combination of workspace and non-strict build plan is definitely not an odd use case, though | 18:04 |
| juergbi | another aspect is that we can hopefully get rid of the workspace cache key recalculation after build anyway long term | 18:04 |
| benschubert | juergbi: ok, then let's try the single trigger, I benchmark tonight and if our "normal" use case is not badly affected we go for this, ok? | 18:05 |
| juergbi | sounds good to me | 18:06 |
| benschubert | juergbi: however, __cache_key does change sometime right? | 18:08 |
| benschubert | it can be weak or strict or strong, right? | 18:08 |
| juergbi | no, I don't think so | 18:09 |
| juergbi | it's always a 'strong' cache key | 18:09 |
| juergbi | in strict mode, it is always equivalent to the strict cache key | 18:09 |
| juergbi | technically, it can change, but it should not be observable | 18:09 |
| benschubert | So techinically: | 18:10 |
| benschubert | while queue: | 18:10 |
| benschubert | element = queue.pop() | 18:10 |
| benschubert | if not element.__ready_for_runtime: | 18:10 |
| benschubert | element._update_state() | 18:10 |
| benschubert | if element.__ready_for_runtime: | 18:10 |
| benschubert | for rdep in element.__reverse_dependencies_for_build: | 18:10 |
| benschubert | queue.push(rdep.__node_id, rdep) | 18:10 |
| benschubert | for rdep in element.__reverse_dependencies_for_runtime: | 18:10 |
| benschubert | queue.push(rdep.__node_id, rdep) | 18:10 |
| benschubert | Should work right? It does not :/ | 18:10 |
| juergbi | can't have the conditional on update_state | 18:10 |
| juergbi | e.g., cached state update etc. needs to be run also if cache key is already available | 18:10 |
| juergbi | would be nice to split out 'cached' state update. maybe this would make the above work | 18:11 |
| juergbi | but that's something for another branch | 18:11 |
| *** lachlan has quit IRC | 18:11 | |
| benschubert | mmh I'm not sure how I should fix it then | 18:11 |
| juergbi | doesn't this check work? | 18:12 |
| juergbi | element.__ready_for_runtime != old_ready_for_runtime: | 18:12 |
| * juergbi has to run out to buy some food | 18:13 | |
| benschubert | juergbi: and removing the first shortcut? | 18:15 |
| *** lachlan has joined #buildstream | 18:15 | |
| juergbi | yes, we can't make update_state conditional on this | 18:17 |
| juergbi | we could make the queue.push for runtime reverse deps conditional on cache key being available | 18:18 |
| benschubert | pushed a new version where test pass | 18:20 |
| benschubert | let me know if that's good for you and i'll cleanup and benchmark | 18:20 |
| *** pointswaves_ has joined #buildstream | 18:22 | |
| *** pointswaves_ has quit IRC | 18:23 | |
| *** pointswaves has joined #buildstream | 18:23 | |
| *** raoul_ has quit IRC | 18:25 | |
| *** nimish2711 has joined #buildstream | 18:26 | |
| pointswaves | Tristan juergbi i have been doing some doc stuff in my own time and i cant get the pipe line to pass on the free runners, can you add my personal account to developers so i can run the pipelines with the fancy runners? https://gitlab.com/pointswaves/buildstream/pipelines | 18:32 |
| *** lachlan has quit IRC | 18:32 | |
| *** pointswaves has quit IRC | 18:36 | |
| *** pointswaves has joined #buildstream | 18:37 | |
| *** pointswaves_ has joined #buildstream | 18:38 | |
| *** pointswaves has quit IRC | 18:40 | |
| *** toscalix has quit IRC | 18:41 | |
| *** pointswaves_ has quit IRC | 18:43 | |
| *** pointswaves_ has joined #buildstream | 18:43 | |
| *** nimish2711 has quit IRC | 18:48 | |
| *** pointswaves has joined #buildstream | 18:50 | |
| juergbi | pointswaves: done | 18:51 |
| *** pointswaves_ has quit IRC | 18:51 | |
| pointswaves | juergbi, thanks | 18:54 |
| *** pointswaves has quit IRC | 18:57 | |
| *** alatiera has quit IRC | 19:03 | |
| *** pointswaves has joined #buildstream | 19:15 | |
| benschubert | tristan: !1222 is ready normally, can you have a (hopefully) last look? | 19:34 |
| gitlab-br-bot | MR !1222: Enable pylint on the tests https://gitlab.com/BuildStream/buildstream/merge_requests/1222 | 19:34 |
| *** pointswaves has quit IRC | 20:31 | |
| *** rdale has quit IRC | 20:40 | |
| *** slaf_ has joined #buildstream | 22:00 | |
| *** slaf_ has joined #buildstream | 22:00 | |
| *** slaf has quit IRC | 22:02 | |
| *** slaf_ is now known as slaf | 22:02 | |
| *** swick_ has joined #buildstream | 23:16 | |
| *** swick has quit IRC | 23:17 | |
| *** swick_ is now known as swick | 23:17 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!