| *** nimish2711 has joined #buildstream | 01:00 | |
| *** swick_ has joined #buildstream | 01:04 | |
| *** swick has quit IRC | 01:05 | |
| *** swick_ is now known as swick | 01:05 | |
| *** nimish2711 has quit IRC | 02:34 | |
| *** nimish2711 has joined #buildstream | 03:01 | |
| *** nimish2711 has quit IRC | 03:06 | |
| *** tristan has quit IRC | 04:56 | |
| gitlab-br-bot | tristanvb approved MR !1236 (aevri/str_e->master: app.py: str(e) instead of "{}".format(e)) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1236 | 05:09 |
|---|---|---|
| *** tristan has joined #buildstream | 05:21 | |
| gitlab-br-bot | tristanvb opened MR !1242 (tristan/remove-toplevel-makefile-1.2->bst-1.2: Removing toplevel Makefile) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1242 | 06:19 |
| gitlab-br-bot | tristanvb opened MR !1243 (tristan/move-conftest-into-tests->master: tests: Move conftest.py into tests/ subdirectory) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1243 | 06:28 |
| gitlab-br-bot | tristanvb opened issue #966 (tests load user's configuration) on buildstream https://gitlab.com/BuildStream/buildstream/issues/966 | 06:59 |
| *** swick has quit IRC | 06:59 | |
| gitlab-br-bot | tristanvb opened MR !1244 (tristan/fix-xdg-env-in-tests->master: Fix xdg env in tests) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1244 | 07:11 |
| gitlab-br-bot | marge-bot123 merged MR !1243 (tristan/move-conftest-into-tests->master: tests: Move conftest.py into tests/ subdirectory) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1243 | 07:13 |
| *** ChanServ sets mode: +o tristan | 07:26 | |
| tristan | coldtom, You might be interested to comment on #964 | 07:26 |
| gitlab-br-bot | Issue #964: UX: Development process, using IDE, index code https://gitlab.com/BuildStream/buildstream/issues/964 | 07:26 |
| tristan | Since you are working on the builder plugin | 07:26 |
| tristan | I recall there was much discussion in a previous meetup and a demo about ide integration, talking about the same kinds of features this reporter is bringing up | 07:27 |
| tristan | aevri, You might be interested in the above issue as well ^^^ | 07:27 |
| aevri | 👍 | 07:36 |
| gitlab-br-bot | tristanvb merged MR !1242 (tristan/remove-toplevel-makefile-1.2->bst-1.2: Removing toplevel Makefile) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1242 | 07:37 |
| tristan | and aevri proves that I really need to run hexchat from a flatpak :) | 07:37 |
| aevri | Hehe :) How come, worried about prank emojis? | 07:41 |
| * aevri navigates to the office via a vast network of deep underground tunnels | 07:42 | |
| *** tristan has quit IRC | 07:42 | |
| *** tristan has joined #buildstream | 07:52 | |
| gitlab-br-bot | marge-bot123 closed issue #966 (tests load user's configuration) on buildstream https://gitlab.com/BuildStream/buildstream/issues/966 | 08:02 |
| gitlab-br-bot | marge-bot123 merged MR !1244 (tristan/fix-xdg-env-in-tests->master: Fix xdg env in tests) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1244 | 08:02 |
| *** swick has joined #buildstream | 08:52 | |
| *** phil has joined #buildstream | 08:55 | |
| *** toscalix has joined #buildstream | 08:59 | |
| *** rdale has joined #buildstream | 09:23 | |
| gitlab-br-bot | marge-bot123 merged MR !1236 (aevri/str_e->master: app.py: str(e) instead of "{}".format(e)) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1236 | 09:24 |
| *** swick has quit IRC | 09:29 | |
| *** raoul_ has joined #buildstream | 09:34 | |
| *** nimish2711 has joined #buildstream | 09:42 | |
| benschubert | tristan: I modified !1070 with a simpler version, slightly less performant (will discover cache keys later in the case of non strict workspaced builds) but more readable and easier to refactor for the future. Could you have a look? I still need to address your other comments, which I hope to be able to come to this afternoon | 09:44 |
| gitlab-br-bot | MR !1070: WIP: Refactor _update_state() to be called only when needed https://gitlab.com/BuildStream/buildstream/merge_requests/1070 | 09:44 |
| *** ChanServ sets mode: +o tristan | 09:45 | |
| *** jonathanmaw has joined #buildstream | 09:45 | |
| tristan | benschubert, I think the other comments were mostly stylistic and easier to address | 09:45 |
| benschubert | tristan: yep :) hence I want to get the complex part right first :) | 09:46 |
| tristan | I'll take a look, but prefer that juergbi take a look :) | 09:46 |
| tristan | I have come to accept that you need to flatten the list in order to efficiently avoid duplicate visits | 09:46 |
| tristan | but | 09:46 |
| tristan | Also it caused me to reflect on Element.dependencies() which achieves this by caching a set of visited items during it's run | 09:47 |
| tristan | benschubert, it's orthogonal, but perhaps there are cycles to gain in that direction as well | 09:47 |
| benschubert | tristan: something like https://gitlab.com/BuildStream/buildstream/tree/bschubert/optimize-dependencies ? ;) | 09:48 |
| tristan | Also, regardless of flattening lists, I am much happier to have it all happen in __update_state_recursively() rather than holding on to the lists and also recursing | 09:48 |
| tristan | benschubert, yeah looks promising :) | 09:48 |
| tristan | not that I've looked at it, but that it exists :) | 09:49 |
| benschubert | tldr: I have a branch for this, but want the update_state first and will then benchmark the same changes over there and the use of the PQ ;) | 09:49 |
| tristan | Hmmm, can we re-assign !1222 to marge ? | 09:50 |
| gitlab-br-bot | MR !1222: Enable pylint on the tests https://gitlab.com/BuildStream/buildstream/merge_requests/1222 | 09:50 |
| benschubert | So basically if juergbi is fine with the changes, I'll cross-check my benchmarks, cleanup the PR and ask for a hopefully final review, good with you? | 09:50 |
| tristan | it initially failed because of a spurious error | 09:50 |
| benschubert | tristan: oh great, I'll do that | 09:50 |
| benschubert | tristan: speaking of linting, we have a "circular-import" error that is disabled in pylint because we have a lot of them. Fixing it would require to import things not from the BuildStream root (.) but directly from the sub packages (.plugins) for instance. Would you be ok with this change? The reason why I'd like to remove those is that if we change the ordering of imports, or add an import at the incorrect place, we can get very | 09:53 |
| benschubert | hard to debug errors | 09:53 |
| *** raoul_ is now known as raoul | 09:53 | |
| tristan | benschubert, Yeah, just looked at !1070 and it is much more comprehensive, added single comment (just about a comment) | 09:54 |
| gitlab-br-bot | MR !1070: WIP: Refactor _update_state() to be called only when needed https://gitlab.com/BuildStream/buildstream/merge_requests/1070 | 09:54 |
| benschubert | great, I'll cleanup all of this this afternoon then and open a (hopefully) final round of review | 09:54 |
| tristan | benschubert, I would prefer you get juergbi to look at it once, and after stylistic changes we should land it | 09:54 |
| benschubert | I will! | 09:54 |
| juergbi | I'm looking at it | 09:54 |
| tristan | For circular imports... I would love to have that fixed ! | 09:54 |
| tristan | benschubert, I have some thoughts on it, though | 09:55 |
| tristan | benschubert, Here is what I want.... (A) Everything outside of BuildStream core... including the plugins we distribute, only import things directly "from buildstream" | 09:55 |
| tristan | for utils it's a bit weird, but still we do "from buildstream import utils" | 09:56 |
| tristan | i.e. external things never dig deeper than "buildstream", and that is one of the ways that we advertize public API | 09:56 |
| tristan | benschubert, Internally I also have a similar preference about our sub packages | 09:56 |
| tristan | I.e. I want the `_scheduler` package to advertise everything that is needed outside of the scheduler in _scheduler/__init__.py | 09:57 |
| benschubert | tristan: so for example if I modified "element.py" to do "from .plugin import Plugin" instead of "from . import Plugin", that would be good, correct? | 09:57 |
| tristan | And I dont want anyone *outside* of the _scheduler/ directory to import deep from the _scheduler, they should do "from _scheduler import BuildQueue" for instance | 09:57 |
| tristan | plugin is adjacent so yes, I think that makes sense | 09:58 |
| benschubert | that's sensible, I'll ensure we meet those requirements when doing the changes :) | 09:58 |
| benschubert | thanks! | 09:58 |
| tristan | benschubert, I think that basically means that you avoid adjacent modules from triggering the package's __init__.py | 09:59 |
| tristan | if I'm not mistaken | 09:59 |
| benschubert | exactly | 09:59 |
| tristan | But yeah, my comments about packages advertizing to the outside world what should be used in their __init__.py is mostly a guideline to help ensure good encapsulation | 09:59 |
| tristan | benschubert, Thanks :) | 09:59 |
| benschubert | yep, the problem we currently have is having sibling packages importing from the parent :) so that's the fix I'm looking for! | 10:00 |
| tristan | Great :) | 10:00 |
| tristan | benschubert, if there are specific simple types that cause issues, that's what types.py is for | 10:01 |
| tristan | most of the stuff in there is really simple things which otherwise cause cyclic imports | 10:01 |
| benschubert | ok! | 10:01 |
| juergbi | benschubert: logic looks good to me now. thanks for your work on this | 10:10 |
| juergbi | commented with a tiny possible optimization but probably doesn't really matter | 10:10 |
| benschubert | juergbi: thanks a lot for all the input on it! | 10:11 |
| benschubert | I'll try and benchmark the optimization and will cleanup now | 10:11 |
| juergbi | benschubert: hm, we actually no longer use build and runtime reverse dependency lists separately | 10:14 |
| juergbi | so we could use a single list | 10:14 |
| juergbi | or do you think we'll need the split again soon anyway? | 10:14 |
| juergbi | (when splitting update_state) | 10:15 |
| gitlab-br-bot | marge-bot123 closed issue #941 (Enable pylint for tests) on buildstream https://gitlab.com/BuildStream/buildstream/issues/941 | 10:18 |
| gitlab-br-bot | marge-bot123 merged MR !1222 (bschubert/linter-for-tests->master: Enable pylint on the tests) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1222 | 10:18 |
| benschubert | juergbi: we might need to split soon again for update_state | 10:19 |
| benschubert | but I'm not sure | 10:19 |
| benschubert | I can merge them and we can split when needed | 10:19 |
| *** gokcennurlu has joined #buildstream | 10:21 | |
| juergbi | tristan: !1214 could use your input with regards to where to push sources to remote (CAS) source cache. i.e., separate queue (between fetch and build or after build? how to call new queue in UI?) or push sources as part of 'fetch'? | 10:26 |
| gitlab-br-bot | MR !1214: Remote source cache https://gitlab.com/BuildStream/buildstream/merge_requests/1214 | 10:26 |
| tristan | juergbi, I'll try to put some time towards that thanks | 10:28 |
| juergbi | ta | 10:28 |
| *** alatiera has joined #buildstream | 10:32 | |
| *** tpollard has quit IRC | 10:39 | |
| *** tpollard has joined #buildstream | 10:55 | |
| *** lachlan has joined #buildstream | 11:01 | |
| jonathanmaw | Since the WSL CI tests are taking a long time for developers, would a better solution be to have a test that can be started manually for non-master branches, and is automatically run only for master? | 11:16 |
| jonathanmaw | would it be okay for CI to fail in master if the WSL tests fail? | 11:19 |
| WSalmon | i dont think it undose your merge if it fails so it would be more attention grabing if it did fail, but a middle way could be to have it run only on master but be ok to fail? | 11:24 |
| tristan | jonathanmaw, The main problem with tests failing in master is that it can get overlooked on a day with many merges, and it's hard to pin it down to the responsible commit | 11:26 |
| tristan | jonathanmaw, On the other hand, I don't think it's particularly likely for commits which don't break other pipelines to break the WSL one | 11:26 |
| *** lachlan has quit IRC | 11:27 | |
| tristan | I guess the first point is less of a concern, as you can see the first breaking commit in the log | 11:31 |
| benschubert | As long as we have it run somewhere and report, I'm fine with it | 11:31 |
| * tristan agrees | 11:32 | |
| tristan | jonathanmaw, if it's causing pain, I think doing it only on master would be fine - not a scheduled CI though | 11:32 |
| * tristan would rather at least have the log of which merge initially introduced breakage, if it happens | 11:33 | |
| tristan | (or only on master and manually in branches) | 11:33 |
| * jonathanmaw isn't sure of the terminology. A scheduled CI is one that would happen at a specific time instead of in response to a specific merge? | 11:33 | |
| tristan | jonathanmaw, yeah, we have the nightly freedesktop-sdks on a schedule for instance | 11:34 |
| jonathanmaw | ah, ok | 11:34 |
| tristan | those arent associated to a specific commit, and run nightly because they take a long time | 11:34 |
| benschubert | running them on master whenever is a merge would be good enough tristan right? | 11:34 |
| tristan | Yeah | 11:34 |
| tristan | Making them manually triggerable when not on master is good too | 11:35 |
| tristan | In case you are specifically developping something that will effect WSL and want to trigger it | 11:35 |
| jonathanmaw | okay, non-master manual jobs, and automatic job on master only | 11:35 |
| jonathanmaw | should the job on master be allowed to fail? | 11:35 |
| tristan | Good question | 11:35 |
| benschubert | I would say no | 11:36 |
| tristan | Doesnt make much difference | 11:36 |
| tristan | The difference is that you will not collect coverage and you will not publish docs | 11:36 |
| benschubert | tristan: it would mark the build with a huge red cross, and send an email to the dev I think :) | 11:36 |
| tristan | Ok, lets say "no" then :) | 11:36 |
| tristan | I thought the red cross might still be there, but maybe its a warning | 11:37 |
| benschubert | It would be an orange thing, and I don't think that email are sent in that case | 11:37 |
| *** lachlan has joined #buildstream | 11:46 | |
| *** tristan has quit IRC | 11:55 | |
| *** lachlan has quit IRC | 12:08 | |
| gitlab-br-bot | aevri opened (was WIP) MR !1245 (aevri/dirname_basename->master: tests: str(datafiles) instead of a longer thing) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1245 | 12:21 |
| *** raoul has quit IRC | 12:34 | |
| gitlab-br-bot | pointswaves opened MR !1246 (pointswaves/softreset->master: Added doc's for workspace reset --soft) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1246 | 12:47 |
| *** lachlan has joined #buildstream | 12:47 | |
| *** lachlan has quit IRC | 12:52 | |
| jonathanmaw | benschubert: I'm a bit confused by the discussion about being allowed to fail. are you and tristan in favour of it allowing failure (orange icon), or not allowing failure (red cross)? | 12:53 |
| benschubert | jonathanmaw: red | 12:54 |
| jonathanmaw | excellent, I wrote https://gitlab.com/BuildStream/buildstream/merge_requests/1241/diffs and paused to think about whether I understood you correctly | 12:55 |
| *** lachlan has joined #buildstream | 13:00 | |
| mablanch | Would someone involved with the CI mind having a look at https://gitlab.com/BuildStream/buildstream/merge_requests/1239? | 13:01 |
| * jonathanmaw has a poke | 13:03 | |
| gitlab-br-bot | martinblanchard approved MR !1241 (jonathan/wsl-tests-manual->master: gitlab-ci: Make WSL tests only run automatically on master) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1241 | 13:22 |
| gitlab-br-bot | jonathanmaw merged MR !1241 (jonathan/wsl-tests-manual->master: gitlab-ci: Make WSL tests only run automatically on master) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1241 | 13:23 |
| *** lachlan has quit IRC | 13:24 | |
| *** raoul has joined #buildstream | 13:35 | |
| *** alatiera has quit IRC | 13:49 | |
| *** alatiera has joined #buildstream | 13:50 | |
| SotK | hey folks, I was asked to take a look into solving the issue that currently exists whereby anyone can assign their MR to marge and have it merged without necessarily going through review | 14:00 |
| SotK | I thought of maybe patching marge-bot to only allow a whitelisted group of people to assign to it, but then noticed that some MRs have required approvals set | 14:01 |
| SotK | is there some reason for not just setting a number of required approvals from said whitelisted group of people in the project configuration to force it on all MRs, rather than allowing the requester to opt in/out? | 14:02 |
| *** lachlan has joined #buildstream | 14:09 | |
| aevri | Hiya SotK, we had agreed that folks should be able to self-merge trivial changes, to keep small things cheap. I think that's probably why. | 14:26 |
| *** lachlan has quit IRC | 14:32 | |
| SotK | aevri: GitLab can be configured to allow self-approval by approvers who send patches, so we could keep that workflow for people in the whitelisted group of "people allowed to approve MRs" if we only set a hard requirement of 1 approval for merge | 14:32 |
| SotK | it does remove that workflow for people who aren't approvers though, which might or might not be an issue | 14:32 |
| SotK | I don't really see how we can only allow a specific set of people to approve merging (whether as approvals or by making marge reject assignments from some folk) and also allow anyone to self-merge their trivial changes | 14:34 |
| *** lachlan has joined #buildstream | 14:35 | |
| benschubert | SotK, aevri I like SotK proposition of allowing self approval for trusted people | 14:37 |
| aevri | Cool, I think if we restrict 'margers' to the list of 'mergers' (approvers?) then I get the feeling we'll be ok. Seems like we need tristan to comment :) | 14:40 |
| *** lachlan has quit IRC | 14:41 | |
| SotK | there's no need to actually restrict "margers" if you require at least 1 approval from an enforced set of "approvers", since marge just complains and re-assigns the patch if it doesn't have enough approvals to merge | 14:42 |
| SotK | which is effectively what it would do with a restricted list of "margers" anyway, just with a different error message | 14:42 |
| laurence | we changed the committer policy after GUADEC and gave blanket access to anyone who had a single patch. I remember reading that commit access should really have been given to a trusted set of people instead. We discussed a carbon copy of the subversion protocol and agreed to adopt it, but I never got around to submitting a patch to the CONTRIBUTING doc | 14:58 |
| laurence | Anyway I think that 'trusted list' of committers can correspond to the list of people who can approve an MR | 14:59 |
| laurence | I'll work on that patch soon | 14:59 |
| * aevri agrees | 15:00 | |
| aevri | Thanks laurence! | 15:00 |
| laurence | np! | 15:01 |
| SotK | sounds like a good plan | 15:01 |
| SotK | someone with access will need to change the settings for the projects we want to enforce that on too | 15:02 |
| laurence | indeed, and that may need to be a group level owner | 15:02 |
| * aevri stops to appreciate the improvement marge has been on our workflow | 15:03 | |
| *** lachlan has joined #buildstream | 15:40 | |
| *** lachlan has quit IRC | 15:53 | |
| gitlab-br-bot | raoul.hidalgocharman approved MR !1225 (juerg/partial-cas->master: Initial support for partial local CAS) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1225 | 15:57 |
| *** lachlan has joined #buildstream | 16:02 | |
| gitlab-br-bot | aevri opened issue #967 (Retrying failed builds is important, but difficult for new users) on buildstream https://gitlab.com/BuildStream/buildstream/issues/967 | 16:20 |
| gitlab-br-bot | raoul.hidalgocharman approved MR !1240 (aevri/element_loaderror_detail->master: element: keep original 'detail' when re-raising) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1240 | 16:29 |
| *** lachlan has quit IRC | 17:07 | |
| *** lachlan has joined #buildstream | 17:07 | |
| gitlab-br-bot | marge-bot123 merged 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:12 |
| *** lachlan has quit IRC | 17:34 | |
| *** raoul has quit IRC | 17:38 | |
| *** lachlan has joined #buildstream | 17:41 | |
| *** jonathanmaw has quit IRC | 18:01 | |
| *** toscalix has quit IRC | 18:21 | |
| gitlab-br-bot | BenjaminSchubert opened MR !1247 (bschubert/better-pytest-report->master: tests: when comparing lists/dicts, compare all at once) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1247 | 18:35 |
| *** nimish2711 has quit IRC | 18:57 | |
| *** nimish2711 has joined #buildstream | 18:58 | |
| *** lachlan has quit IRC | 19:42 | |
| *** lachlan has joined #buildstream | 19:49 | |
| *** nimish2711 has quit IRC | 19:54 | |
| gitlab-br-bot | BenjaminSchubert opened MR !1248 (bschubert/lint/logging-format->master: casserver.py: fix logging-format-interpolation error and enable) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1248 | 20:09 |
| *** lachlan has quit IRC | 20:52 | |
| *** lachlan has joined #buildstream | 20:59 | |
| *** lachlan has quit IRC | 21:10 | |
| *** alatiera has quit IRC | 21:58 | |
| gitlab-br-bot | cs-shadow approved MR !1248 (bschubert/lint/logging-format->master: casserver.py: fix logging-format-interpolation error and enable) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1248 | 22:07 |
| gitlab-br-bot | cs-shadow opened MR !1249 (chandan/remove-manifest-conftest->master: MANIFEST.in: Remove conftest.py include) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1249 | 22:15 |
| gitlab-br-bot | BenjaminSchubert approved MR !1249 (chandan/remove-manifest-conftest->master: MANIFEST.in: Remove conftest.py include) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1249 | 22:31 |
| *** nimish2711 has joined #buildstream | 23:48 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!