| *** traveltissues has joined #buildstream | 07:46 | |
| *** bochecha has joined #buildstream | 09:30 | |
| *** rdale has joined #buildstream | 09:32 | |
| benschubert | Is anyone awaiting on reviews? | 09:36 |
|---|---|---|
| *** santi has joined #buildstream | 09:44 | |
| *** mohan43u has quit IRC | 09:57 | |
| *** tpollard has joined #buildstream | 10:00 | |
| *** phildawson_ has joined #buildstream | 10:10 | |
| *** lachlan has joined #buildstream | 10:28 | |
| *** phildawson_ has quit IRC | 10:32 | |
| *** toscalix has joined #buildstream | 10:33 | |
| *** phildawson_ has joined #buildstream | 10:34 | |
| *** phildawson_ has quit IRC | 10:37 | |
| *** phildawson_ has joined #buildstream | 10:37 | |
| *** lachlan has quit IRC | 10:46 | |
| *** tiagogomes has joined #buildstream | 10:52 | |
| *** tiagogomes has quit IRC | 10:54 | |
| *** lachlan has joined #buildstream | 10:54 | |
| *** tiagogomes has joined #buildstream | 10:58 | |
| *** toscalix has quit IRC | 10:58 | |
| *** phildawson_ has quit IRC | 11:00 | |
| *** lachlan has quit IRC | 11:07 | |
| *** phildawson_ has joined #buildstream | 11:17 | |
| *** tme5 has joined #buildstream | 11:24 | |
| tme5 | does marge work? | 11:26 |
| tlater[m] | tme5: Marge has been a little out of it lately | 11:27 |
| tlater[m] | I've still not had the time to sit down and talk to her about it | 11:28 |
| tme5 | then could someone with the powers please merge !1765 when they get a moment | 11:30 |
| gitlab-br-bot | MR !1765: Make Git source plugin clone submodules recursively https://gitlab.com/BuildStream/buildstream/merge_requests/1765 | 11:30 |
| benschubert | Sure I'll do this, thanks tme5 ! | 11:32 |
| gitlab-br-bot | BenjaminSchubert closed issue #1162 (Git source doesn't support recursive submodules) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1162 | 11:33 |
| gitlab-br-bot | BenjaminSchubert merged MR !1765 (tmewett/recursive-git->master: Make Git source plugin clone submodules recursively) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1765 | 11:33 |
| tme5 | thanks ben | 11:33 |
| tpollard | benschubert: I've been prodding your 3.8 branch. Turning coverage off for now & adding a timeout/newer container for buildbox I've got it to output some tests errors | 12:04 |
| tpollard | They seem to be repeatable | 12:04 |
| benschubert | tpollard: yep there should be 5 of them. I'm more concerned about the coverage failure :/ | 12:05 |
| tpollard | I'm currently getting 4, all from shutil | 12:05 |
| benschubert | ah right, that seems correct | 12:05 |
| tpollard | https://gitlab.com/BuildStream/buildstream/-/jobs/380395708/raw | 12:06 |
| tpollard | on the release notes for 3.8 it mentions changes to copytree api | 12:06 |
| benschubert | Ok that should not be too hard to fix | 12:09 |
| benschubert | do you have any idea around the coverage problem though? :/ | 12:10 |
| *** narispo has quit IRC | 12:13 | |
| *** narispo has joined #buildstream | 12:13 | |
| benschubert | Has someone tried to use buildstream master for anything recently? I'm getting loads of errors in the usbprocesses | 12:22 |
| * tlater[m] did last Friday | 12:22 | |
| benschubert | handle: <Handle Job._parent_child_completed(4278, 0)> | 12:22 |
| benschubert | Traceback (most recent call last): | 12:22 |
| benschubert | File "/usr/lib/python3.6/asyncio/events.py", line 145, in _run | 12:22 |
| benschubert | self._callback(*self._args) | 12:22 |
| benschubert | File "/home/bschubert15/gitlab/buildstream/src/buildstream/_scheduler/jobs/job.py", line 455, in _parent_child_completed | 12:22 |
| benschubert | self._process.close() | 12:22 |
| benschubert | AttributeError: '_AsyncioSafeForkAwareProcess' object has no attribute 'close' | 12:22 |
| benschubert | Exception in callback Job._parent_child_completed(4279, 0) | 12:22 |
| benschubert | handle: <Handle Job._parent_child_completed(4279, 0)> | 12:22 |
| benschubert | Traceback (most recent call last): | 12:22 |
| benschubert | Would seem like something I would have caused previously? | 12:22 |
| tlater[m] | Didn't see anything in particular, but I was just working on the integration project | 12:22 |
| tlater[m] | Odd, I'd expect mypy to see that kind of problem | 12:23 |
| benschubert | It's untyped at that place | 12:25 |
| tlater[m] | :| | 12:25 |
| *** lachlan has joined #buildstream | 12:26 | |
| benschubert | ah no got it. I't `self._process.close()` that was added when replacing the queue by a pipe | 12:26 |
| *** narispo has quit IRC | 12:27 | |
| *** narispo has joined #buildstream | 12:27 | |
| tpollard | & that's 3.7+ specific? | 12:28 |
| tpollard | seems to be :/ | 12:28 |
| tlater[m] | Wait, but that means someone has succeeded in running buildstream on 3.8 | 12:28 |
| tpollard | new to 3.7 | 12:28 |
| tlater[m] | Oh | 12:28 |
| tlater[m] | Right | 12:29 |
| benschubert | ah you can now clsoe on 3.7? | 12:29 |
| tpollard | https://docs.python.org/3.7/library/multiprocessing.html#multiprocessing.Process.close | 12:29 |
| tlater[m] | I guess the test suite just never hits that then? | 12:29 |
| benschubert | oh wow | 12:29 |
| benschubert | Ah no it went unnoticed since it's jsut printed on the screen, and nothing catches that | 12:30 |
| tlater[m] | Oh, right | 12:30 |
| benschubert | juergbi: ^ also :) | 12:30 |
| benschubert | Do we want to monkeypatch a 'close' method on python 3.5 and 3.6 or not close at all? | 12:30 |
| tpollard | I should have spotted that, I remember hitting it with the separate process work | 12:30 |
| benschubert | oh well :) | 12:31 |
| tpollard | benschubert: in terms of coverage, not found a solution yet. although 5.0 came out of beta on saturday so I'll try that.... | 12:32 |
| tlater[m] | benschubert: If monkeypatching is simple enough, I think that would be ok | 12:32 |
| tlater[m] | But we should have some sort of a collection ground for patches like that | 12:32 |
| juergbi | oops | 12:32 |
| tlater[m] | So that we can remove them as we get rid of support for old versions | 12:33 |
| *** lachlan has quit IRC | 12:33 | |
| *** mohan43u has joined #buildstream | 12:41 | |
| *** lachlan has joined #buildstream | 12:46 | |
| tme5 | is it valid to use variables an element sources configuration? | 12:46 |
| tme5 | i can't seem to get them to expand in a URL | 12:47 |
| tlater[m] | tme5: I think the reason I didn't just implement a patch for my issue when I came up with that was precisely that variables aren't expanded there | 12:49 |
| tlater[m] | But I don't know off the top of my head where to check for that | 12:49 |
| *** lachlan has quit IRC | 12:50 | |
| tme5 | why not 😭 | 12:51 |
| tlater[m] | tme5: That's been lost to the annals, I'm sure if you searched the chat history long enough you could find me and tristan talking about that | 12:52 |
| tme5 | i would say, maybe we can just add a project: alias | 12:52 |
| coldtom | there was an issue for it i think? https://gitlab.com/BuildStream/buildstream/issues/1127 perhaps? | 12:53 |
| *** lachlan has joined #buildstream | 12:53 | |
| tlater[m] | tme5: I don't know if I like a default alias | 12:53 |
| tlater[m] | I'd rather figure out why we don't have expanding variables, since that's something we want anyway | 12:54 |
| tlater[m] | At which point this feature becomes trivial :) | 12:54 |
| tme5 | "Not exposing the variables in sources is mostly just an oversight, I don't see anything dangerous about this especially due to the relationship between the Element and Source" | 12:55 |
| tme5 | from van berkom | 12:56 |
| *** lachlan has quit IRC | 12:57 | |
| *** narispo has quit IRC | 12:57 | |
| *** narispo has joined #buildstream | 12:59 | |
| tme5 | i'll probably reopen and assign myself to #1127 then | 13:04 |
| gitlab-br-bot | Issue #1127: Allow sources to access their element's variables https://gitlab.com/BuildStream/buildstream/issues/1127 | 13:04 |
| *** narispo has quit IRC | 13:19 | |
| *** narispo has joined #buildstream | 13:19 | |
| gitlab-br-bot | tmewett reopened issue #1127 (Allow sources to access their element's variables) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1127 | 13:20 |
| benschubert | tpollard: I've tried too and have even more problems. However I'd agree that moving to coverage 5.0 first is a good idea! | 13:24 |
| benschubert | tme5: right, I was planning onworking on this but never managed to get a satisfactory solution in terms of performance :( Good luck with that! | 13:25 |
| *** toscalix has joined #buildstream | 13:44 | |
| *** santi has quit IRC | 13:45 | |
| *** santi has joined #buildstream | 13:49 | |
| *** toscalix has quit IRC | 13:52 | |
| tpollard | benschubert: same failures | 13:59 |
| *** toscalix has joined #buildstream | 14:00 | |
| * tpollard keeps trying | 14:02 | |
| *** phildawson-ct has joined #buildstream | 14:03 | |
| tpollard | I think it's coming from them switching from json to sql | 14:05 |
| *** phildawson_ has quit IRC | 14:05 | |
| valentind | jjardon, benschubert, could you tell me about the issue you are having with collect_manifest? Is there merge request or branch that fails to build? | 14:07 |
| jjardon | valentind: https://gitlab.com/BuildStream/buildstream/issues/1241 | 14:09 |
| jjardon | valentind: the schedule to build fdsdk with bst master is failing | 14:09 |
| *** phildawson-ct has quit IRC | 14:10 | |
| coldtom | just need to update the bst-plugins-experimental commit used i think | 14:10 |
| valentind | This is a weird error | 14:10 |
| valentind | Ah no, it is "Format version 1 is too old for requested version 2" | 14:11 |
| valentind | Right, it does not use the right version of bst-plugins-experimental | 14:11 |
| benschubert | valentind: my issue was that both PRs have quite different code and was wondering how come :) | 14:13 |
| valentind | be5ac19 is too old. | 14:13 |
| valentind | benschubert, Yes, but this is because the code was already different. | 14:13 |
| benschubert | valentind: gotcha, thanks | 14:14 |
| valentind | Someone else has submitted the plugin to bst-plugins-experimental, then during the review process, lots of changes were done, and they did not backport it to bst-external. | 14:14 |
| *** tme5 has quit IRC | 14:15 | |
| valentind | In my merge request it is really the same feature. I just also fixed formatting in the documentation that was wrong. The rest should be the same. | 14:15 |
| benschubert | Do you expect it to become a problem? As in, should we open an issue and fix it or just keep it like that? | 14:15 |
| valentind | Let's fix that if it really becomes a problem. | 14:16 |
| valentind | It is not like I expect we will add new features in that plugin every month. | 14:16 |
| valentind | But maybe next time we push something from bst-external to bst-plugins-experimental, we should fix first things in bst-external before we put it in bst-plugins-experimental. | 14:17 |
| *** phildawson-ct has joined #buildstream | 14:17 | |
| valentind | And the only differences should be API related. | 14:17 |
| benschubert | Ok, so bst-plugins-experimental is the 'canonical' version and we should update the other instead of the reverse? (That is if we ever need to do it) | 14:18 |
| valentind | No. bst-plugins-experimental is not canonical. It is not well tested enough. | 14:19 |
| *** tme5 has joined #buildstream | 14:19 | |
| tme5 | benschubert, performance? interesting, i wouldn't have imagined this would be that bad? seeing as variable substitution happens everywhere else already? | 14:19 |
| valentind | If a plugin is in one repo, no matter which one, when adding it in the other one, if we find things to fix that are not related to API, then the repo source of that plugin should be fixed first. | 14:20 |
| benschubert | tme5: it required me to change a bit how we instantiate all plugins and that was what killed me. A nicer implementation might not have problems at all :) | 14:21 |
| valentind | Because what ever repo it comes from, it is probably the repo from which it is more tested. | 14:21 |
| tme5 | ahh ok | 14:22 |
| *** toscalix has quit IRC | 14:24 | |
| *** lachlan has joined #buildstream | 14:31 | |
| *** lachlan has quit IRC | 14:37 | |
| *** lachlan has joined #buildstream | 14:38 | |
| *** lachlan has quit IRC | 14:41 | |
| *** lachlan has joined #buildstream | 14:48 | |
| *** lachlan has quit IRC | 14:58 | |
| benschubert | tlater[m]: around those problems with python3.5/3.6 What about a '_compat.py' that we import in __init__ that adds all our monkeypatches? And we would ensure each patch specifies which versions it targets? | 15:06 |
| tlater[m] | benschubert: Yep, that's what I was thinking | 15:07 |
| tlater[m] | I wonder if there are generic dependencies that do such a thing | 15:07 |
| benschubert | well, I'd rather handle this ourselves :) | 15:08 |
| tlater[m] | Yeah, probably best | 15:08 |
| tlater[m] | It probably won't be too common. | 15:09 |
| benschubert | I sure hope so :'D | 15:09 |
| *** lachlan has joined #buildstream | 15:09 | |
| tlater[m] | Famous last words ;p | 15:09 |
| *** lachlan has quit IRC | 15:12 | |
| tlater[m] | Are we 3.6 yet? | 15:17 |
| tlater[m] | No :( | 15:17 |
| tlater[m] | No NamedTuple for me :( | 15:17 |
| tlater[m] | Let alone the nice default type syntax | 15:17 |
| tlater[m] | Oh, wait, no, NamedTuple is ok, just not the version syntax | 15:19 |
| tlater[m] | \o/ | 15:19 |
| Kinnison | NamedTuple is SLOW | 15:20 |
| Kinnison | just to warn you | 15:20 |
| tlater[m] | Kinnison: oh, boo | 15:23 |
| Kinnison | We tried them back when we were rewriting the yaml handling | 15:23 |
| Kinnison | turns out they're super-slow | 15:23 |
| tlater[m] | Probably not a good idea to handle all references into CAS through them, then | 15:23 |
| Kinnison | Probably not :-( | 15:24 |
| benschubert | Cython in the worst case ;) | 15:24 |
| * tlater[m] once again is disappointed that python isn't rust | 15:24 | |
| tlater[m] | I mean, at this point I'm even annotating my types exactly the same way | 15:25 |
| *** lachlan has joined #buildstream | 15:27 | |
| bochecha | tlater[m]: and then you're disappointed that mypy doesn't catch so many issues because the libraries you use don't have type hints? | 15:29 |
| bochecha | and you spend way too long writing stubs? | 15:29 |
| tlater[m] | bochecha: Hah, I don't even trust mypy to begin with because my codebase is old and we've only just started using type hints, so 99% of the time they're just useless | 15:29 |
| bochecha | and you still have to add a bunch of "# type: ignore" comments here and there because mypy is still a work in progress? | 15:29 |
| bochecha | oh wait no, that's me 😭️ | 15:30 |
| bochecha | to be fair, mypy did find a few bugs in my code which otherwise might have just crashed the app in production | 15:30 |
| tlater[m] | Optional[x] is probably my favorite so far | 15:30 |
| tlater[m] | So many Nones | 15:31 |
| tlater[m] | That said, I'd appreciate a Tython fork | 15:31 |
| bochecha | heh, Optional[x] is in fact the first thing mypy told me | 15:31 |
| tlater[m] | It's what it took to get javascript manageable | 15:31 |
| *** lachlan has quit IRC | 15:31 | |
| bochecha | who knew os.cpu_count() could return None in some cases | 15:32 |
| bochecha | I didn't, but mypy did :) | 15:32 |
| *** phildawson_ has joined #buildstream | 15:48 | |
| *** phildawson-ct has quit IRC | 15:49 | |
| *** lachlan has joined #buildstream | 15:52 | |
| *** lachlan has quit IRC | 15:56 | |
| *** lachlan has joined #buildstream | 16:01 | |
| *** lachlan has quit IRC | 16:05 | |
| *** lachlan has joined #buildstream | 16:12 | |
| *** lachlan has quit IRC | 16:19 | |
| gitlab-br-bot | BenjaminSchubert opened MR !1773 (bschubert/fix-resource-deletion->master: _compat.py: Add module to handle version compatibility in python) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1773 | 16:24 |
| benschubert | tlater[m]: ^ | 16:24 |
| tlater[m] | ta benschubert | 16:24 |
| tlater[m] | benschubert: Just a small nit, otherwise LGTM | 16:27 |
| benschubert | fair point :) | 16:30 |
| *** santi has quit IRC | 16:48 | |
| *** santi has joined #buildstream | 17:03 | |
| *** lachlan has joined #buildstream | 17:15 | |
| *** lachlan has quit IRC | 17:22 | |
| *** phildawson_ has quit IRC | 17:28 | |
| *** phildawson_ has joined #buildstream | 17:34 | |
| benschubert | tme5: also we don't have variables everywhere: e.g. build/runtime-depends are missing | 17:37 |
| tme5 | i've been looking at it more and i don't think i'm qualified to make this change | 17:39 |
| tme5 | there's this whole thhing of MetaSource which seems to in some way decouple Sources and Elements | 17:40 |
| *** lachlan has joined #buildstream | 17:48 | |
| *** lachlan has quit IRC | 17:52 | |
| tlater[m] | Is there a bug here? If we call .get_digest() before .get_directory() we'll get a None here, no? https://gitlab.com/BuildStream/buildstream/blob/master/src/buildstream/storage/_casbaseddirectory.py#L76 | 17:53 |
| * tlater[m] wonders if there's any reason we don't just compute the digest at initialization | 17:54 | |
| juergbi | tlater[m]: the invariant is that always exactly one of digest or buildstream_object is set | 18:03 |
| juergbi | when performing modifications, we don't want to recalculate the digest on every change, that would be a lot of overhead | 18:03 |
| tlater[m] | juergbi: It doesn't look like it's guaranteed that one of the two is set just looking at the class | 18:04 |
| tlater[m] | Is that upheld everywhere else? | 18:04 |
| juergbi | it should be. well, both are allowed to be None in case of an empty directory | 18:05 |
| tlater[m] | Looks like a lot of how these entries are created lives in CasBasedDirectory, not in the actual class | 18:07 |
| tlater[m] | Fair enough | 18:07 |
| tlater[m] | This does make it difficult to pass around an immutable sub-portion of it though | 18:07 |
| tlater[m] | Because the digest must be part of it | 18:07 |
| tlater[m] | But it's not actually immutable | 18:07 |
| *** santi has quit IRC | 18:08 | |
| juergbi | hm, yes, sounds tricky as we also need to make sure we don't add unnecessary overhead | 18:08 |
| juergbi | the immutable object makes sense for the case where the digest field is valid, but not when the (mutable) buildstream_object is populated | 18:09 |
| tlater[m] | Yup, but that duality can't be avoided | 18:09 |
| juergbi | maybe it would be better to have two separate classes with a shared interface | 18:10 |
| tlater[m] | FrozenIndexEntry? | 18:11 |
| juergbi | however, that wouldn't allow the `get_directory()` approach to convert between the two | 18:11 |
| juergbi | so it might require changes in multiple places | 18:11 |
| tlater[m] | It would still allow it, you'd just need to call `get_directory()` when you make the entry "frozen" | 18:11 |
| juergbi | get_directory() returns a mutable object. which wouldn't make sense to call on a frozen index entry | 18:12 |
| tlater[m] | Ah, right | 18:12 |
| tlater[m] | But when it's a directory that's inherently frozen, right? Even if we had a second class with the same interface, we wouldn't be able to have directories in it. | 18:13 |
| tlater[m] | s/frozen/mutable/ | 18:13 |
| juergbi | right | 18:14 |
| juergbi | tlater[m]: a completely different approach might be possible | 18:14 |
| juergbi | I think we only need the diff to apply it exactly once to another directory | 18:14 |
| juergbi | we could provide this as a single operation in the (internal) API | 18:14 |
| juergbi | that way we wouldn't have to care about potential mutations between diff and apply | 18:15 |
| tlater[m] | i.e., not compute a diff at all but simply move the directory on top of the other? | 18:15 |
| juergbi | it would still have to apply file-based changes. and actually, we could still create a diff internally | 18:15 |
| tlater[m] | Well, yes | 18:16 |
| juergbi | the main point would be to not expose the diff / IndexEntry objects at all outside this file | 18:16 |
| juergbi | which would make the mutability aspect much less of an issue | 18:16 |
| tlater[m] | Hm, are you sure we don't need the diff? | 18:16 |
| juergbi | not 100% | 18:17 |
| tlater[m] | I recall traveltissues' algorithm specifying that it would need to look at the diff | 18:17 |
| tlater[m] | Let me double check... | 18:17 |
| tlater[m] | juergbi: https://mail.gnome.org/archives/buildstream-list/2019-October/msg00000.html, btw | 18:17 |
| tlater[m] | I don't think we ever actually use the delta there | 18:19 |
| juergbi | except to apply it | 18:19 |
| tlater[m] | Yeah | 18:19 |
| tlater[m] | Ok, in that case that can work | 18:20 |
| tlater[m] | Shame about that immutable internal class though, it made the code a lot neater | 18:20 |
| juergbi | yes | 18:22 |
| *** tme5 has quit IRC | 18:26 | |
| *** lachlan has joined #buildstream | 18:28 | |
| *** rdale has quit IRC | 18:34 | |
| *** traveltissues has quit IRC | 18:42 | |
| *** phildawson_ has quit IRC | 18:54 | |
| *** lachlan has quit IRC | 18:58 | |
| *** lachlan has joined #buildstream | 19:09 | |
| *** lachlan has quit IRC | 19:15 | |
| *** lachlan has joined #buildstream | 19:40 | |
| *** lachlan has quit IRC | 19:49 | |
| *** lachlan has joined #buildstream | 19:55 | |
| *** lachlan has quit IRC | 20:09 | |
| *** narispo has quit IRC | 20:26 | |
| *** narispo has joined #buildstream | 20:26 | |
| *** lachlan has joined #buildstream | 20:56 | |
| *** lachlan has quit IRC | 21:03 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!