| *** toscalix has quit IRC | 02:03 | |
| *** tristan has joined #buildstream | 07:13 | |
| tristan | juergbi, around ? | 07:37 |
|---|---|---|
| juergbi | tristan: yes | 07:37 |
| tristan | So one thing which was missed in jmac's branch I want to add to that before landing and tying up a release, is that the new SandboxConfig fails to contribute to an element's cache key | 07:39 |
| tristan | I was writing up a quick patch to add on top for that... and I just wondered whether we should A.) Retain compatibility of cache key algorithm for this, in the case of default sandbox config... or B.) Just bump the artifact version and have a nicer cache key calculation, without any branching | 07:40 |
| tristan | juergbi, What I'm thinking is also... we have already added 'execution-environment' to the element cache key - with the thought in mind that we would eventually delegate this to sandboxes | 07:41 |
| tristan | So I'm thinking, add SandboxConfig.get_unique_key() and move over the os/arch stuff to SandboxConfig | 07:42 |
| juergbi | right, that makes sense | 07:43 |
| juergbi | don't know how painful cache key changes are for the average user | 07:43 |
| tristan | I guess I could retain compatibility with an added if in SandboxConfig.get_unique_key(), very low cost if | 07:43 |
| tristan | Yeah ok I'll roll with that for now and not bump the artifact version | 07:44 |
| * tristan hopes that os.uname() already returns a cached value from python | 07:45 | |
| juergbi | i.e., keep calling it execution-environment? | 07:45 |
| tristan | yeah if we keep calling it execution-environment in the unique key dict, it doesnt break anything | 07:45 |
| tristan | while being just... a bit unattractive hehe | 07:45 |
| juergbi | well, it's not really a bad fit | 07:45 |
| juergbi | we may need to think about canonical arch names but not an issue now | 07:47 |
| tristan | right, that doesnt change with this branch | 07:48 |
| tristan | juergbi, any preference of adding _get_cache_key() over _get_unique_key() to SandboxConfig ? | 07:55 |
| tristan | juergbi, I sort of prefer _get_unique_key() and let the hashing algo run only once | 07:56 |
| tristan | (or less times) | 07:56 |
| tristan | Hrrrrmmmmm | 08:01 |
| juergbi | tristan: you want to individually hash the execution environment dict? | 08:01 |
| juergbi | that would change the cache key, though | 08:01 |
| tristan | good point | 08:01 |
| tristan | in fact, i exactly *dont* want to do that | 08:01 |
| tristan | just that I see project and context are doing that (with nothing to say, on top of it) | 08:02 |
| tristan | anyway | 08:02 |
| tristan | My Hrrrrmmmmm, is deb source; this does something weird | 08:02 |
| juergbi | project/context: conceptually it makes sense avoiding recalculating the hash all the time | 08:03 |
| juergbi | but it's indeed pointless right now | 08:03 |
| tristan | Right, ah good point they are less plentiful than sandbox configs, which are per element | 08:03 |
| tristan | So, deb source is not doing try/except in preflight for `import arpy` | 08:04 |
| tristan | and reporting a sensible SourceError at preflight | 08:04 |
| tristan | also, the test case is not conditionalized on HAVE_ARPY | 08:04 |
| tristan | without that; BuildStream grows a sort-of hard dependency on arpy, which is wrong | 08:04 |
| * tristan will open an issue for that and install arpy locally in the meanwhile | 08:05 | |
| tristan | looks like at *least* it wont explode at load time when running a pipeline that doesnt use the deb source | 08:07 |
| tristan | a bit strange that it doesnt explode, but seems not to | 08:07 |
| gitlab-br-bot | buildstream: issue #317 ("New deb source arpy dependency is too hard") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/317 | 08:13 |
| tristan | eh, maybe I'll just take care of #317 now, deb source addition also missing from NEWS, I wanna make a release today and rather not with the arpy hard dep | 08:34 |
| juergbi | tristan: doesn't the ImportError handler in the plugin loader already take care of this? | 08:38 |
| juergbi | Error loading pipeline: Failed to load Source plugin 'deb': No module named 'arpy' | 08:38 |
| juergbi | doesn't look like a bad error message to me | 08:38 |
| juergbi | I forgot about conditionalizing the tests | 08:40 |
| gitlab-br-bot | buildstream: issue #318 ("Cache key test is incomplete") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/318 | 08:41 |
| tristan | juergbi, relying on that would be... weird honestly - if that is what the user is seeing, the user is seeing something incorrect which needs fixing. | 08:42 |
| juergbi | I don't see the issue in this error message, tbh | 08:43 |
| tristan | juergbi, i.e. an unhandled exception (an exception which has not been transformed to a BstError), is a bug, and the user should see a stack trace, either directly on the terminal, or in a BUG message | 08:43 |
| juergbi | arpy is a hard dependency of the deb source plugin, so why can't we bail out at plugin load time? | 08:43 |
| tristan | We could bail out at load time, but it would be better to not do so with an unhandled exception | 08:44 |
| juergbi | it may not have been intentional that the exception is handled but i don't see an issue with the end effect | 08:44 |
| tristan | juergbi, I dont disagree, however if there is ambiguity in the policy around this, that is more of a problem to me | 08:46 |
| tristan | I dont think we have precedent for plugins which blind import something that is not covered by BuildStream's hard dependencies | 08:46 |
| juergbi | ostree with unix backend? | 08:47 |
| tristan | Does that happen ? | 08:47 |
| tristan | That would be a bug, I think | 08:47 |
| tristan | Do we see a stack trace for it ? | 08:47 |
| juergbi | I haven't tried | 08:48 |
| juergbi | but theoretically, you can use the unix backend on linux | 08:48 |
| juergbi | with an element that has an ostree source | 08:48 |
| tristan | Not legally | 08:48 |
| juergbi | ok but ostree could theoretically be available on other systems | 08:48 |
| tristan | That is a hack we have for tests in the absence of a non-linux testing platform | 08:48 |
| juergbi | or you could simply attempt to build a project that uses an ostree source on a non-linux system | 08:49 |
| juergbi | error message should make sense then as well | 08:49 |
| tristan | juergbi, Ok well - if we have policy that python imports at toplevel can bail out at plugin load time, so be it | 08:50 |
| tristan | This should be explicitly handled somewhere, which I figure it must be if you are not seeing a stack trace | 08:50 |
| juergbi | the plugin loader catches ImportError | 08:50 |
| juergbi | plugincontext.py:load_plugin | 08:51 |
| tristan | load_plugin() has except there yes | 08:51 |
| tristan | just looked at that | 08:51 |
| juergbi | maybe it was only meant for import error of the plugin itself | 08:51 |
| juergbi | however, I don't see any harm using it for dependencies the same way | 08:52 |
| tristan | to be honest, that except has a long history, and I feel that represents more of an unhandled error, than an informative message telling the user how to rectify the problem which BuildStream encountered | 08:52 |
| tristan | While could allow it as a matter of robustness, it seems nicer to have a more explicit error for plugin dependencies | 08:53 |
| tristan | A programmer, especially one familiar with python, might understand: Error loading pipeline: Failed to load Source plugin 'deb': No module named 'arpy' | 08:54 |
| juergbi | ok, fair enough | 08:54 |
| juergbi | theoretically, we should also fix ostree, then | 08:54 |
| juergbi | otherwise attempting to use it on non-linux you get: | 08:54 |
| juergbi | Error loading pipeline: Failed to load Source plugin 'ostree': No module named 'gi' | 08:55 |
| tristan | Indeed | 08:55 |
| tristan | There are other options, like policy of conditional installation of plugins, but that seems unreasonable | 08:55 |
| tristan | I.e. that would be a nightmare for distros eventually packaging BuildStream | 08:55 |
| juergbi | and we should probably change the ImportError handler to only catch the error if it fails to load the plugin itself | 08:55 |
| juergbi | a distro might move the plugin to a separate distro package | 08:56 |
| juergbi | probably better not to, though | 08:56 |
| tristan | Yeah that's what I mean, it would be: A.) Just as difficult for us to handle this at install time as to fail gracefully as a matter of policy... B.) Much more unneeded work downstream | 08:57 |
| juergbi | I'm wondering whether we could provide a more sensible error message in plugincontext | 08:59 |
| juergbi | I don't really like the idea of forcing plugins to not have any 3rd party imports at the top leve | 08:59 |
| tristan | I dont think that I expected that error to be raised due to a missing dependency | 09:00 |
| tristan | rather, it may happen for a syntax error | 09:00 |
| tristan | not sure what conditions can cause that import error to occur | 09:00 |
| juergbi | sure, but maybe we can figure out the reason for the import error and provide suitable error message or BUG as appropriate | 09:01 |
| juergbi | to make plugin author life slightly simpler | 09:01 |
| tristan | Right, we might turn that into raising another unhandled error `from e`, which is more informative to the plugin author | 09:02 |
| juergbi | don't know how much of a concern this is in practice, were just some random thoughts | 09:02 |
| tristan | if they get a stack trace | 09:02 |
| juergbi | for something like syntax error, yes | 09:03 |
| tristan | I added all of those PluginErrors to test for our loading of invalid plugins | 09:03 |
| juergbi | and for import error of dependency (if we can detect that), we could supply a user-friendlier message how to install the missing dependency and not burden the plugin author having to do this themselves | 09:03 |
| tristan | I.e. AttributeError and TypeError below, come with some test cases of badly written plugins | 09:04 |
| tristan | It may be that we just want to make PluginError no longer inherit from BstError | 09:04 |
| juergbi | right, that's nice | 09:04 |
| tristan | It adds some informative context of what BuildStream saw, and still gives the plugin author the full trace | 09:04 |
| tristan | Right, we could enhance that error a bit... | 09:05 |
| tristan | juergbi, except that for ostree we probably want something better hehe | 09:06 |
| juergbi | yes, gi is trickier :/ | 09:06 |
| juergbi | so maybe it's not worth it after all, at least for now | 09:06 |
| juergbi | btw: ModuleNotFoundError is a subclass of ImportError, so we could have some distinction here. and there is a name attribute so that we could check whether the name is the plugin itself or a dependency | 09:07 |
| tristan | The ImportError will provide some interesting members we could use to do something generic and helpful, like "This plugin failed to import module `foo`, please install it with pip or using your distribution package manager" | 09:07 |
| juergbi | yes, that's what I was thinking | 09:07 |
| juergbi | not really urgent, though | 09:08 |
| tristan | We wouldnt be able to really distinguish this for say... a package of plugins which imports something like _downloadablefilesource.py from . | 09:08 |
| juergbi | well, there is a path attribute, not sure how that's populated | 09:08 |
| tristan | but eh... | 09:08 |
| juergbi | anyway | 09:08 |
| tristan | yeah, that's going a bit too far anyway | 09:08 |
| juergbi | the urgent thing is to add the missing test skip | 09:08 |
| juergbi | imo, improving the error message for people actually using deb is not a release blocker | 09:09 |
| juergbi | i.e., we could consider fixing that in a more generic way later | 09:09 |
| juergbi | up to you | 09:09 |
| gitlab-br-bot | buildstream: merge request (jmac/build-uid-2->master: Sandbox configuration key to specify uid/gid for sandbox) #318 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/318 | 09:11 |
| tristan | juergbi, Agree | 09:11 |
| tristan | that it's not a release blocker | 09:11 |
| juergbi | are you already handling the test skip or shall I? | 09:11 |
| tristan | I hadn't gotten to it yet :) | 09:12 |
| tristan | was just adding a cache key test for build-uid / build-gid | 09:12 |
| tristan | now merged that part... | 09:12 |
| tristan | juergbi, I dont mind doing it, it's trivial | 09:12 |
| juergbi | indeed, as you like | 09:13 |
| tristan | juergbi, but if you do it, I'll try to draft something from our conversation regarding plugin load errors | 09:13 |
| juergbi | ok, i'll handle this, then | 09:13 |
| juergbi | tristan: do you want to keep #317 open for error message improvement or track that separately? | 09:21 |
| gitlab-br-bot | buildstream: merge request (juerg/arpy->master: Skip deb tests if arpy is not available) #338 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/338 | 09:26 |
| tristan | juergbi, lets track that separately | 09:35 |
| tristan | strange, it looks as though that test case never even needed `import arpy` in the first place | 09:37 |
| * tristan wonders why the linter didnt catch that | 09:37 | |
| gitlab-br-bot | buildstream: issue #317 ("New deb source arpy dependency is too hard") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/317 | 09:38 |
| gitlab-br-bot | buildstream: issue #317 ("New deb source arpy dependency is too hard") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/317 | 09:38 |
| gitlab-br-bot | buildstream: merge request (juerg/arpy->master: Skip deb tests if arpy is not available) #338 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/338 | 09:38 |
| juergbi | we still haven't enabled unused import linter, unfortunately | 09:38 |
| juergbi | hopefully we can do this soon | 09:38 |
| juergbi | don't know whether jennis is pjlanning to work on this or not | 09:38 |
| tristan | ah | 09:39 |
| tristan | Issue tracker is getting biggish... but it would be nice to have an issue for such todo items | 09:40 |
| tristan | whenever landing something that needs future work | 09:40 |
| juergbi | indeed, let me open one | 09:40 |
| gitlab-br-bot | buildstream: issue #319 ("Improve error handling at plugin load time") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/319 | 09:48 |
| * tristan missed out on listing plugins which could do better... such as deb and ostree | 09:50 | |
| tristan | I'll rectify that in #319 now... | 09:50 |
| gitlab-br-bot | buildstream: merge request (juerg/imports->master: Remove unused imports and enable unused-import checker) #339 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/339 | 09:58 |
| tristan | juergbi, nice ! | 10:00 |
| tristan | oh docs broke with that ? | 10:01 |
| tristan | https://gitlab.com/BuildStream/buildstream/-/jobs/59287321 | 10:01 |
| tristan | I think that is easy to fix | 10:02 |
| tristan | juergbi, that just needs an update to doc/source/invoking.rst | 10:04 |
| gitlab-br-bot | buildstream: issue #320 ("pylint: Enable arguments-differ checker") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/320 | 10:04 |
| juergbi | oh, right, let me fix that | 10:04 |
| gitlab-br-bot | buildstream: merge request (juerg/imports->master: Remove unused imports and enable unused-import checker) #339 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/339 | 10:06 |
| tristan | meh, technically it can be imported directly from buildstream._frontend (we do that for the main entry point, it's in __init__.py), but I dont particularly care enough | 10:09 |
| tristan | juergbi, if you feel pedantic enough to want to change that... you can, but I really dont mind | 10:10 |
| tristan | otherwise merge | 10:10 |
| juergbi | ah, right. can also be done in runcli.py | 10:11 |
| juergbi | i'll quickly fix that up | 10:11 |
| * tristan has a NEWS update for deb, and adding the deb source to docs index; pending | 10:14 | |
| tristan | I'll wait for your thing to merge and rebase on top of that first | 10:14 |
| gitlab-br-bot | buildstream: merge request (juerg/imports->master: Remove unused imports and enable unused-import checker) #339 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/339 | 10:14 |
| juergbi | ta | 10:14 |
| *** valentind has joined #buildstream | 10:15 | |
| tristan | juergbi, any missing NEWS items you would add ? | 10:22 |
| tristan | I think the big things, now that I've added deb, are all accounted for | 10:22 |
| juergbi | pylint is probably too internal to be NEWS-worthy | 10:23 |
| * tristan was thinking that | 10:23 | |
| tristan | Did we land the element specific build dirs ? it's something that... nobody will notice, though :) | 10:24 |
| tristan | until it becomes useful in a user visible feature | 10:24 |
| gitlab-br-bot | buildstream: merge request (juerg/imports->master: Remove unused imports and enable unused-import checker) #339 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/339 | 10:24 |
| juergbi | tristan: not landed yet, but should be ready for re-review | 10:24 |
| juergbi | !310 | 10:24 |
| tristan | ah | 10:25 |
| juergbi | btw: I'm wondering whether we should hold off the release due to #316 | 10:25 |
| juergbi | difficult without a reproducer, though | 10:25 |
| tristan | that is annoying | 10:25 |
| juergbi | interesting that it happened for compose element. i thought it would be a workspace issue | 10:26 |
| tristan | and workaroundable with --strict | 10:26 |
| tristan | I dont want to hold off on the release for that, it has been around for the greater part of 1.1.1 I think | 10:26 |
| juergbi | yes. if it doesn't happen frequently, it may not be too bad for a devel snapshot | 10:26 |
| tristan | we just started hitting it | 10:27 |
| tristan | To me it's making a clean slate for the next sprint, I'm going to focus more on flatpaky stuff | 10:27 |
| juergbi | ok | 10:27 |
| *** ChanServ sets mode: +o tristan | 10:40 | |
| *** tristan changes topic to "/BuildStream 1.1.2 is out ! | https://gitlab.com/BuildStream/buildstream | Docs: https://buildstream.gitlab.io/buildstream | IRC logs: https://irclogs.baserock.org/buildstream | Roadmap: https://wiki.gnome.org/Projects/BuildStream/Roadmap" | 10:40 | |
| *** tristan has quit IRC | 12:25 | |
| *** tristan has joined #buildstream | 12:30 | |
| *** milloni has joined #buildstream | 13:29 | |
| *** milloni has quit IRC | 13:32 | |
| *** milloni has joined #buildstream | 13:34 | |
| *** milloni has quit IRC | 13:39 | |
| *** milloni has joined #buildstream | 13:43 | |
| *** Prince781 has joined #buildstream | 13:58 | |
| *** Prince781 has quit IRC | 14:19 | |
| *** Prince781 has joined #buildstream | 14:21 | |
| *** Prince781 has quit IRC | 14:45 | |
| *** mohan43u has quit IRC | 15:39 | |
| *** mohan43u has joined #buildstream | 15:39 | |
| *** Prince781 has joined #buildstream | 16:50 | |
| *** tristan has quit IRC | 19:56 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!