| *** persia has quit IRC | 01:05 | |
| *** persia has joined #buildstream | 01:05 | |
| *** tristan has quit IRC | 06:13 | |
| *** tristan has joined #buildstream | 06:26 | |
| *** benschubert has joined #buildstream | 07:23 | |
| *** toscalix has joined #buildstream | 07:26 | |
| *** ChanServ sets mode: +o tristan | 07:34 | |
| tristan | benschubert, About provenance... you bring to mind a corner case which I think we still don't handle well | 07:34 |
|---|---|---|
| benschubert | tristan: oh? | 07:34 |
| *** bochecha has joined #buildstream | 07:34 | |
| tristan | I think relevant issues are https://gitlab.com/BuildStream/buildstream/issues/591 and https://gitlab.com/BuildStream/buildstream/issues/253, which were "fixed" but still leave a gap iirc | 07:34 |
| tristan | When the user is required to specify something in the yaml (i.e. no default was provided to node_get(), implying that a missing value is an error)... and that has no default value inherited from other composited yaml | 07:36 |
| benschubert | I see, I'll make sure those work | 07:36 |
| tristan | We end up getting the provenance of the dictionary where it is expected to be added, either the plugin's .yaml file or the default project configuration | 07:36 |
| tristan | I think there is no concept currently to make those "work" | 07:36 |
| benschubert | I'll probably ping you in a few hours about node_sanitize stuff btw :) | 07:37 |
| benschubert | tristan: yeah, ensuring it's the user-facing file | 07:37 |
| benschubert | I'll add tests and ensure I cover this | 07:37 |
| tristan | yeah, I wonder if it requires some API :-/ | 07:37 |
| benschubert | I'll see :) | 07:37 |
| tristan | regarding your specific question from last night... there may be some unhandled cases which arise specifically because of the possibility of compositing YAML with raw dictionaries | 07:38 |
| tristan | crafted in code | 07:38 |
| tristan | or that is the origin of why synthetic provenances exist at all | 07:38 |
| tristan | benschubert, perhaps the entire use case can be eliminated, or... we need to consider "What is the provenance of something that was set in code" and make that somehow valid | 07:39 |
| tristan | one thing I can think of is calls to Element.set_public_data() | 07:39 |
| tristan | if another plugin reads public data which was dynamically set... and asserts some expectation on that data... then what should the error message say ? | 07:40 |
| benschubert | yep, agreed, I would really like to not have to show users 'oh you have an error line 123 in element.bst, it should be a string', and 'yeah that's a string... ' | 07:41 |
| benschubert | I think this should say that it's coming from a synthetic node, which means a programing mistake | 07:41 |
| tristan | provenance might be the project & plugin.py file (so the user can know that the code in such and such plugin loaded by such and such project wrote out incorrect yaml) | 07:41 |
| tristan | interestingly, we have enough context at Element.set_public_data() time to fabricate more sensible provenance | 07:42 |
| tristan | but then, causing that to be preserved through artifact metadata is tricky | 07:42 |
| benschubert | yeah, I'm not sure I really like the idea | 07:42 |
| tristan | It's certainly something I would like as a feature | 07:43 |
| tristan | I mean... if we have say the debian packaging plugin read some invalid key that should be an int but is not... then I would like to know the element name, project and plugin which caused that node to be generated for later consumption by the reading plugin | 07:44 |
| tristan | rather than "This public data member is not what I expected... go figure out where it came from yourself" :) | 07:44 |
| benschubert | agreed | 07:45 |
| benschubert | is it only in public data? | 07:45 |
| tristan | Of course... this is quite advanced error handling territory, fixing it would be a vast improvement, but not fixing it now is probably okay :) | 07:45 |
| benschubert | I'll make sure I don't prevent fixing it later | 07:46 |
| tristan | I believe that is the only valid place where code generates data that gets into the nodes yes | 07:46 |
| tristan | benschubert, there is another case we should not care too much about | 07:46 |
| tristan | benschubert, I suspect we have various test cases which composite YAML nodes, and that is rather fine that the synthetic provenances are just dummies | 07:46 |
| benschubert | right, yeah I'm not too worried about those | 07:48 |
| benschubert | Btw, if you want to have a look, I have 3 more PRs to the new-node-api branch: https://gitlab.com/BuildStream/buildstream/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&target_branch=bschubert%2Fnew-node-api It's starting to take shape | 07:48 |
| tristan | yeah I noticed, they look easy to review, I'll jump on them now and try to get them out of the way | 07:48 |
| benschubert | thanks! | 07:49 |
| *** alexandrufazakas has joined #buildstream | 08:00 | |
| tristan | benschubert, while reviewing your patch I spot a couple of other exceptions where we manually modify the nodes | 08:32 |
| tristan | benschubert, those are basically the "automatic variables", one of which is obviously `max-jobs` | 08:32 |
| tristan | of course it's quite unrealistic that we would raise an error on these automatic variables, so their provenance is probably not important... just pointing out that these exist :) | 08:33 |
| valentind | Was the bastion restarted? | 08:34 |
| benschubert | tristan: good catch! | 08:34 |
| benschubert | thanks for the reviews | 08:34 |
| valentind | I see there are issues with selinux | 08:34 |
| valentind | jjardon, or lchlan ? | 08:35 |
| valentind | No, it is a Debian, it should be some other issue. | 08:36 |
| *** raoul has joined #buildstream | 08:37 | |
| valentind | It might be on the docker machines. | 08:39 |
| valentind | Switching to coreos to see. | 08:39 |
| alexandrufazakas | What do I need to do to set the default directory for bst as None, instead of os.getcwd()? | 08:45 |
| alexandrufazakas | I changed the click.option to `default=None` | 08:45 |
| alexandrufazakas | But printing the kwargs in the cli constructor still has 'directory' to CWD :/ | 08:45 |
| gitlab-br-bot | BenjaminSchubert opened issue #1058 (Rework node provenance for synthetic nodes) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1058 | 08:57 |
| *** rdale has quit IRC | 09:01 | |
| *** phildawson has joined #buildstream | 09:03 | |
| *** jonathanmaw has joined #buildstream | 09:06 | |
| tpollard | alexandrufazakas: how did you install buildstream? | 09:13 |
| alexandrufazakas | uhh | 09:13 |
| alexandrufazakas | the recommended way on the wiki | 09:13 |
| alexandrufazakas | tpollard: So I think using pip3 | 09:15 |
| gitlab-br-bot | BenjaminSchubert merged MR !1426 (bschubert/node-api-keys->bschubert/new-node-api: Cleanup `MappingNode` key/values/items accessor) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1426 | 09:16 |
| gitlab-br-bot | jennis opened issue #1059 (Regression: Unhelpful error message on missing variables) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1059 | 09:19 |
| jennis | tristan, benschubert ^ related to your discussion this morning | 09:20 |
| benschubert | that's a lot of things to fix, thanks! | 09:20 |
| jennis | I went back to pre-yaml-rework and the bug still existed | 09:21 |
| *** rdale has joined #buildstream | 09:21 | |
| valentind | Finally the builders work. | 09:27 |
| tristan | jennis, yeah it's been forever, thanks for summarizing ! | 09:28 |
| tristan | jennis, some cases were "fixed" but indeed mostly workarounds for the underlying issue | 09:28 |
| jennis | You're welcome | 09:29 |
| tristan | interesting that it happens with 'url', I guess that is set nowhere at all | 09:29 |
| benschubert | yeah :/ | 09:30 |
| benschubert | I'll ensure my branch fixes those things before merging | 09:30 |
| tlater[m] | alexandrufazakas: The reason tpollard asked is because you probably need to install it with -e for it to use your updated version | 09:33 |
| alexandrufazakas | tlater: Makes sense, but thing is I'm running a test | 09:33 |
| tpollard | yes sorry, thanks tlater[m] got distracted | 09:33 |
| tlater[m] | alexandrufazakas: Fair enough, changing the click default argument should work though. Maybe you're in the wrong directory or something? | 09:34 |
| alexandrufazakas | tlater: not sure what you mean by beig in the wrong directory | 09:35 |
| alexandrufazakas | I'll check out a new branch and try on that from scratch | 09:35 |
| * alexandrufazakas shrughs | 09:35 | |
| tlater[m] | Just a random guess, in case you have multiple buildstream directories | 09:35 |
| tlater[m] | I have a whole collection, quite a number named "temp" | 09:35 |
| tlater[m] | I even have a buildstream directory called alexandrufazakas | 09:36 |
| alexandrufazakas | Ah, yeah, I was thinking that might be what you meant. I only have one | 09:36 |
| alexandrufazakas | haha | 09:36 |
| alexandrufazakas | https://pastebin.com/udpWtejR | 09:40 |
| alexandrufazakas | So this is what I have, nothing else, and this is the output of that print | 09:40 |
| alexandrufazakas | https://pastebin.com/x0CgR6Vj | 09:41 |
| alexandrufazakas | Shouldn't directory be None? :/ | 09:41 |
| tristan | jennis, not exactly the same issue I think | 09:41 |
| tristan | jennis, just commented, interesting find though with `url` :-/ | 09:41 |
| tristan | jennis, I suspect that your `url` issue must be some kind of regression, but it's possible that it went unnoticed for this long | 09:44 |
| tristan | maybe not a recent regression | 09:44 |
| tlater[m] | Ah, alexandrufazakas, the Cli fixture sets the project directory by default | 09:45 |
| alexandrufazakas | tlater: Uhh, cli fixture? | 09:46 |
| tlater[m] | Find runcli.py and look in there - there's a way to disable that. | 09:46 |
| tlater[m] | cli.run() :) | 09:46 |
| tlater[m] | You'll want to use cli.configure() | 09:47 |
| jennis | tristan, thanks for looking further into it | 09:52 |
| *** lachlan has joined #buildstream | 09:52 | |
| jennis | So it sounds like you're saying if I commented out another (I'm not sure which) variable, I could have received the appropriate error message? | 09:53 |
| jennis | Or am I misinterpreting this? | 09:53 |
| valentind | tristan, bst-1.2 fails | 09:53 |
| valentind | tests/artifactcache/expiry.py::test_expiry_order | 09:54 |
| valentind | I need to renew the certificate for the documentation pages. And for that I need the build to work. | 09:56 |
| valentind | Could I just disable this test? | 09:57 |
| tristan | valentind, sounds like a spurious error | 09:58 |
| tristan | valentind, try running again at least | 09:58 |
| valentind | It fails all the time on the builders | 09:58 |
| tristan | :-/ | 09:58 |
| tristan | That is bad | 09:58 |
| valentind | They are all red. | 09:58 |
| tristan | And since when did this start happening ? | 09:58 |
| valentind | Looks like something to do with disk space or something. | 09:58 |
| valentind | I do not know. | 09:58 |
| tristan | I see, seems not related to any commit | 09:58 |
| valentind | For some time. Seems the documentation did not get updated for a while. | 09:59 |
| tristan | valentind, is there a way in pytest we can not disable it but turn the failure into a warning ? | 09:59 |
| valentind | Well if the test fails in some condition, it sounds to me the test is wrong. | 09:59 |
| tristan | might be better | 09:59 |
| tristan | valentind, maybe the test needs to be fixed, not sure - but there are of course some minimal requirements of the system | 10:00 |
| tristan | valentind, I think at some point we changed those tests to virtualize the size of the cache | 10:00 |
| tristan | with unittest.mock() | 10:00 |
| tristan | not sure if that is in bst-1.2 | 10:00 |
| valentind | I do not see any straight forward way to make it a warning. | 10:02 |
| valentind | I would really like to update the certificate. | 10:02 |
| valentind | And it is Friday. | 10:02 |
| valentind | I will make a merge request with skip. | 10:02 |
| valentind | We can make something better another time. | 10:02 |
| tristan | valentind, do what you have to | 10:03 |
| tristan | valentind, agreed | 10:03 |
| tristan | jennis, I do suspect you are misinterpreting that... I think | 10:03 |
| tristan | jennis, which part of my comment are you talking about ? | 10:04 |
| jennis | tristan, sorry, that was a response to your IRC comments, I said that before I read the issue | 10:04 |
| jennis | The issue has made things clearer | 10:04 |
| tristan | oh | 10:04 |
| jennis | issue comment* | 10:04 |
| jennis | Thanks | 10:04 |
| gitlab-br-bot | valentindavid opened MR !1436 (valentindavid/disable-test_expiry_order->bst-1.2: tests/artifactcache/expiry.py: Disable test failing on builders) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1436 | 10:06 |
| tristan | jennis, I think the lack of line/column information might mean that it is the toplevel provenance of the file autotools.yaml | 10:06 |
| tristan | jennis, it's a similar error I get if I comment out the `name:` from a project.conf | 10:06 |
| tristan | jennis, Note that if I comment out `name:` from a project.conf, the reason why it reports the correct project.conf... iirc, is that that call to node_get() happens on the project.conf *before* compositing it against the defaults | 10:07 |
| tristan | if we were to composite first and ask for the name later, then we would get the wrong file provenance | 10:07 |
| jennis | Yes, the fact that its reporting an error in autotools.yaml instead of hello.bst makes it clear that composition is the problem | 10:08 |
| tristan | Ummmm | 10:08 |
| gitlab-br-bot | valentindavid opened issue #1060 (Fix tests/artifactcache/expiry.py::test_expiry_order on bst-1.2) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1060 | 10:08 |
| benschubert | tristan: then I think we are not doing the provenance business correctly if we need to check before composition :) | 10:09 |
| tristan | jennis, right so that is the issue with url... somewhere in composition, the provenance of the dictionary is not being preserved | 10:09 |
| tristan | benschubert, I think that is indeed a workaround for this specific issue | 10:09 |
| tristan | jennis, clearly the dictionary comes from hello.bst, and that dictionary did not exist at all in the underlying autotools.yaml... but somewhere in composition that provenance got lost | 10:10 |
| tristan | oddly, there is a lot of strict tests on provenance and composition | 10:11 |
| tristan | but even with that, we obviously havent got everything covered :-/ | 10:11 |
| tristan | Maybe the most checking we do is with list append/prepend, need more provenance asserting in there | 10:12 |
| jennis | Yes the dictionary exists, but it'll get composited without url | 10:13 |
| jennis | Then we have this big composited node where we say "give me the url", which doesn't exist in this node | 10:14 |
| jennis | So we should be checking before composition, otherwise, we need to be clever and try to link back and figure out where url should have been | 10:14 |
| benschubert | jennis: or reverse the composition (use composite_and_move) and when compositing not adding empty values | 10:15 |
| benschubert | that might work | 10:15 |
| gitlab-br-bot | valentindavid merged MR !1436 (valentindavid/disable-test_expiry_order->bst-1.2: tests/artifactcache/expiry.py: Disable test failing on builders) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1436 | 10:42 |
| *** lachlan has quit IRC | 10:44 | |
| tlater[m] | Becky: | 10:52 |
| tlater[m] | Argh | 10:52 |
| tlater[m] | Becky: For reference, I think we'll want to end up doing something like this with your patch: https://pythonprogramminglanguage.com/multiple-inheritance/ | 10:52 |
| tlater[m] | Because really, what we're looking at is a form of multiple inheritance | 10:52 |
| tlater[m] | Our tar plugin wants to simultaneously be a downloadable file, but also a local file, that does tar things | 10:52 |
| tlater[m] | But for now, let's try overriding `__new__` and see whether that works :) | 10:53 |
| Becky | tlater[m] okay thankyou I'll have a look into that :) | 10:53 |
| *** bochecha has quit IRC | 10:56 | |
| *** lachlan has joined #buildstream | 10:56 | |
| alexandrufazakas | From what I've gathered the cli.run() sets the directory to project, but if -C is passed that is overwritten with the -C argument? | 10:59 |
| alexandrufazakas | And uhh, what's the difference between the cli in _frontend/cli.py and testing/runcli.py? | 11:02 |
| tlater[m] | alexandrufazakas: testing/runcli.py is set of functions that allow you to invoke buildstream | 11:04 |
| tlater[m] | cli.py defines the command line interface of buildstream | 11:04 |
| tlater[m] | runcli.py is essentially a library to call cli.py :) | 11:04 |
| tlater[m] | alexandrufazakas: You've got the -C thing wrong, I think | 11:05 |
| tlater[m] | Look at line 341 in runcli.py | 11:05 |
| alexandrufazakas | Yeah, that's why I was asking | 11:05 |
| alexandrufazakas | I actually commented that out | 11:05 |
| tlater[m] | If you run something like `cli.run(project=project, bla bla bla)`, it will set -C to project | 11:05 |
| alexandrufazakas | To see what happens | 11:05 |
| alexandrufazakas | Yeah | 11:05 |
| alexandrufazakas | so if you do | 11:06 |
| tlater[m] | In that case --directory should be set to the default | 11:06 |
| alexandrufazakas | `cli.run(project=project, '-C', '/tmp/foo') | 11:06 |
| tlater[m] | Which is None in your patch, I think? | 11:06 |
| alexandrufazakas | somewhere you set the --directory to project | 11:06 |
| alexandrufazakas | and then you set it to /tmp/foo | 11:06 |
| alexandrufazakas | tlater: Yes, that's what I did | 11:06 |
| tlater[m] | That should work, actually, yes | 11:06 |
| alexandrufazakas | I would set the --directory to None by default | 11:06 |
| alexandrufazakas | And I commented that line out | 11:06 |
| tlater[m] | And what happens? | 11:07 |
| alexandrufazakas | But then the first test doesn't work,s ince I do something like os.getcwd() and it creates project.conf in my directory | 11:07 |
| alexandrufazakas | Which is something like ~/Documents/buildstream/ | 11:07 |
| alexandrufazakas | Instead, it should use the temporary directory that is passed to the project parameter | 11:07 |
| tlater[m] | Yeah, you're going to be need a bit more crafty | 11:07 |
| alexandrufazakas | Which I no longer use | 11:08 |
| alexandrufazakas | Oh, okay | 11:08 |
| alexandrufazakas | If it all makes sense, it's alright | 11:08 |
| alexandrufazakas | I kinda wanted to make sure it isn't me who's going some wrong way | 11:08 |
| tlater[m] | What I'd suggest is writing a test that does `cli.run(project=None, args=["init", os.path.join(tmpdir, "foo")])` | 11:08 |
| tlater[m] | Also, I think that cli.run is just a little broken atm, because str(None) returns 'None' | 11:09 |
| tlater[m] | So you'll need to fix that :) | 11:09 |
| alexandrufazakas | isn't that a python thing? | 11:09 |
| alexandrufazakas | I just did that in python3 too | 11:09 |
| tlater[m] | What do you mean? | 11:09 |
| alexandrufazakas | Isn't str(None) supposed to be 'None'? | 11:10 |
| tlater[m] | Yep, it definitely is | 11:10 |
| alexandrufazakas | Oh | 11:10 |
| alexandrufazakas | You're just saying I just be careful about it | 11:10 |
| tlater[m] | But we unconditionally turn None into 'None' | 11:10 |
| tlater[m] | And use that as the project dir | 11:10 |
| alexandrufazakas | Well, that could be worked around | 11:10 |
| alexandrufazakas | by defaulting --directory to '' | 11:11 |
| alexandrufazakas | That works fine with os.path.join() | 11:11 |
| tlater[m] | alexandrufazakas: Look at line 325 in runcli.py | 11:11 |
| alexandrufazakas | And it still gets the job done. I think | 11:11 |
| tlater[m] | I'd dislike that | 11:11 |
| tlater[m] | There's a very distinct difference between no argument, and an argument | 11:12 |
| tlater[m] | This way you'd test whether giving bst -C '' would give you a reasonable result | 11:12 |
| tlater[m] | You would not test whether just bst works | 11:12 |
| alexandrufazakas | True | 11:12 |
| tlater[m] | And anyway, we've found a bug in our test fixture, you should fix it! x) | 11:13 |
| alexandrufazakas | That should be fairly easy if I understood that right | 11:13 |
| alexandrufazakas | So sure | 11:13 |
| alexandrufazakas | tlater: Should we just set project to '' in that case, or raise an exception? | 11:18 |
| gitlab-br-bot | AlexFazakas opened MR !1437 (AlexFazakas/str-none-runcli->master: runcli: Don't set project to 'None' on no input) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1437 | 11:22 |
| tlater[m] | alexandrufazakas: Neither of those things | 11:22 |
| tlater[m] | You should leave it None :p | 11:23 |
| tlater[m] | So that we get buildstream's default path | 11:23 |
| *** lachlan has quit IRC | 11:34 | |
| tpollard | 'instead of leaving this poor class confused about its very essence' | 11:36 |
| tpollard | this made me chuckle tlater[m] | 11:36 |
| benschubert | jennis , tristan do you see any danger in removing 'node_sanitize' for cache keys? if all call sites are updated to set the value and not the node | 11:37 |
| tristan | benschubert, The keys need to be ordered when dumped for sure | 11:41 |
| tristan | benschubert, can't have any random in there... otherwise originally I think it was just sorting/ordering the dict keys and removing the internal __bst_provenance members | 11:42 |
| benschubert | tristan: ujson already takes care of ordering | 11:42 |
| tristan | sorted ? | 11:43 |
| benschubert | yep | 11:43 |
| tristan | We don't require plugins to provide OrderedDicts so... yeah they need sorting | 11:43 |
| tristan | Ummm, asides from thaaaaat... is it okay to be reliant on this third party thing for the format of cache keys ? | 11:44 |
| *** lachlan has joined #buildstream | 11:44 | |
| benschubert | tristan: https://github.com/esnme/ultrajson/blob/master/python/objToJSON.c#L432 this is sorting it, and we pass sort_keys=True | 11:44 |
| tristan | benschubert, I guess it should be fine as long as it's well commented where we use ujson, and that we should be able to swap implementations if desirable without breaking cache keys ? | 11:45 |
| benschubert | well, we are still using it to dump the values (and re-sort everything again) | 11:45 |
| benschubert | and we have tests for the cache keys | 11:45 |
| benschubert | (also, to ensure we don't ever try to add a Node in a cache key, i can add a "__json__" method on Nodes, that throws a ValueError | 11:46 |
| tristan | yeah, why... are we able to remove the santize thing and *still* pass the cache key tests ? | 11:46 |
| tristan | that would probably be good :) | 11:47 |
| tristan | benschubert, also the __json__ thing would be good | 11:47 |
| benschubert | yep, I currently have the json thing that just returns the internal value jsonified, and it's passing while removing the node_sanitize, but I think we can go one step further, and speed things up :D | 11:48 |
| benschubert | ok! I'll have a shot at it | 11:48 |
| benschubert | I actually believe we should be able to get rid of node_sanitize entirely | 11:49 |
| tristan | sec | 11:49 |
| tristan | benschubert, there may be other places we still need node_sanitize | 11:49 |
| tristan | benschubert, For instance... when dumping YAML formatted things to stdout for scripting purposes | 11:49 |
| benschubert | for this we can add custom encoders that encode the internal value | 11:50 |
| benschubert | and rely on yaml to dump things sensibly | 11:50 |
| benschubert | and finally we are using it in workspaces to generate a dict and pass it to the constructor as kwargs | 11:50 |
| benschubert | and for this, I actually believe we should be checking it, because even though it is a private format, things become interesting if someone changes that file | 11:51 |
| tristan | benschubert, right - just saying that it's best we dont dump randomly sorted YAML dicts to stdout | 11:51 |
| tristan | whatever takes care of it is fine :) | 11:51 |
| benschubert | tristan: we can sort dicts in the parser itself yep | 11:52 |
| *** lachlan has quit IRC | 11:52 | |
| tristan | right, `bst workspace list` is public... and things like `bst show --format '%{vars}'` also | 11:52 |
| benschubert | won't be broken if we craft the yaml dumper correctly (we also have tests covering this) | 11:53 |
| tristan | sure, I mean it might not be technically broken if unsorted... but I think it's better if we do keep them sorted :) | 11:54 |
| tristan | it's definitely nice to have variables alphabetically sorted when debugging/viewing what vars are resolved for a given element | 11:54 |
| * tristan uses that fairly frequently | 11:54 | |
| *** lachlan has joined #buildstream | 11:57 | |
| *** lachlan has quit IRC | 12:01 | |
| jonathanmaw | tristan: when you've got time, can you look at !1409 again? I've reworked it so that all the frontend stuff get passed objects to register callbacks with | 12:12 |
| gitlab-br-bot | MR !1409: Separate frontend state handling from core state https://gitlab.com/BuildStream/buildstream/merge_requests/1409 | 12:12 |
| tristan | jonathanmaw, awesome sauce... going to dinner now :) | 12:16 |
| benschubert | Am I the only one seeing the test 'no_needless_overwrites' fails from time to time? | 12:19 |
| benschubert | from tests/frontend/track.py | 12:20 |
| *** tristan has quit IRC | 12:22 | |
| gitlab-br-bot | AlexFazakas closed MR !1437 (AlexFazakas/str-none-runcli->master: runcli: Don't set project to 'None' on no input) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1437 | 12:58 |
| gitlab-br-bot | AlexFazakas reopened MR !1437 (AlexFazakas/str-none-runcli->master: runcli: Don't set project to 'None' on no input) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1437 | 13:11 |
| benschubert | Do we have an exception to signal a bug in a plugin? | 13:22 |
| *** lachlan has joined #buildstream | 13:39 | |
| *** lachlan has quit IRC | 13:44 | |
| alexandrufazakas | tlater: hey, sorry to ask this again, but how should we avoid the thing on line 342 in runcli.py? We can't just set project to None since we need to run the command in a certain directory, right? | 13:55 |
| tlater[m] | alexandrufazakas: For your test it's enough to prove that we can create a project in that directory, so we don't need to set -C to the temporary directory. | 13:57 |
| tlater[m] | You can just do `bst init <tempdir>/foo/bar` | 13:57 |
| *** lachlan has joined #buildstream | 13:57 | |
| tlater[m] | Instead of `bst -C <tempdir> init foo/bar` | 13:57 |
| alexandrufazakas | Yeah, but for that test to work I need that if to fail | 13:57 |
| tlater[m] | Ah, for the failing test you can just go `bst -C <tempdir> init foo/bar`, and ask pytest to assert that fails | 13:58 |
| alexandrufazakas | But then other tests won't work since they do cli.run(project=project, args-[..]) and here I need to use that project since it contains th eproject's location | 13:58 |
| tlater[m] | Yes, that's why for the test where you do want a project you go project=project | 13:59 |
| tlater[m] | But for the test where you don't want a project you go project=None | 13:59 |
| alexandrufazakas | Yeah but I mean, using project=project, basically means using -C, which is what all of the tests in init.py do | 13:59 |
| tlater[m] | Yes, and those tests stay the same | 14:00 |
| tlater[m] | The only test that is slightly different is the one that uses `bst init <tempdir>/foo/bar` | 14:00 |
| tlater[m] | It specifies project=None | 14:00 |
| tlater[m] | Every other test specifies project=project | 14:00 |
| tlater[m] | Does that make sense? | 14:00 |
| alexandrufazakas | I get what you're saying, but I'm not sure how to implement it so that we abort whenever '-C' is used with init | 14:01 |
| alexandrufazakas | And still run tests using cli.run(project=project) | 14:01 |
| tlater[m] | Ok | 14:01 |
| tlater[m] | How does one affect the other? | 14:01 |
| tlater[m] | Oh! | 14:01 |
| tlater[m] | Nevermind | 14:02 |
| tlater[m] | I just caught on | 14:02 |
| tlater[m] | Right, yes, you'll need to change the other tests | 14:02 |
| tlater[m] | All tests will need to run with `project=None`, and instead specify `project` as an argument | 14:02 |
| alexandrufazakas | Yep | 14:02 |
| alexandrufazakas | That's what I was thinking | 14:02 |
| alexandrufazakas | Makes sense then | 14:03 |
| tlater[m] | So you'll change all the tests to run with `cli.run(project=None, args=["init" project])` | 14:03 |
| alexandrufazakas | Exactly | 14:03 |
| tlater[m] | Yep, that's fine | 14:03 |
| alexandrufazakas | I think it's only the tests in init.py? tlater | 14:03 |
| tlater[m] | Should be for the most part. If there are other tests you'll see them fail :) | 14:04 |
| alexandrufazakas | Right. Thanks :) | 14:04 |
| *** lachlan has quit IRC | 14:05 | |
| *** lachlan has joined #buildstream | 14:08 | |
| gitlab-br-bot | BenjaminSchubert merged MR !1425 (bschubert/node-api-noset->bschubert/new-node-api: Remove 'node_set'. Now use MappingNode[key] = value) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1425 | 14:11 |
| *** tristan has joined #buildstream | 14:13 | |
| *** lachlan has quit IRC | 14:17 | |
| *** CTtpollard has joined #buildstream | 14:23 | |
| *** tpollard has quit IRC | 14:24 | |
| alexandrufazakas | tlater: how would you feel about changing runcli.py? I'm thinking instead of adding `'--directory' + project`, we could just add `project` to the list of arguments, so everything tests the new functionality :D | 14:34 |
| benschubert | alexandrufazakas: One problem with this is that then the old behavior wouldn't be tested anymore :) | 14:41 |
| alexandrufazakas | benschubert: yeah, that's true haha | 14:41 |
| *** lachlan has joined #buildstream | 14:44 | |
| alexandrufazakas | We can always make all tests use both options ¯\_(ツ)_/¯ | 14:44 |
| benschubert | That would be a possibility, however, keep in mind that a full run of the tests is roughly 20 to 40 minutes, doubling that might not fly with developers :) It would be good however to have at least one test of each use the new option | 14:46 |
| tlater[m] | alexandrufazakas: Yeah, I'd be hestitant to change the test API | 14:46 |
| alexandrufazakas | benschubert: Yeah, I really don't think that's doable | 14:47 |
| tlater[m] | Note that you're only changing the functionality of `bst init` | 14:47 |
| tlater[m] | So *only* the tests in `init.py` need to adopt the new functionality | 14:47 |
| alexandrufazakas | or, rather, trying to haha | 14:47 |
| tlater[m] | Which is like, 10 | 14:48 |
| tlater[m] | Whereas we have like 1000 in total | 14:48 |
| benschubert | ^ then running with both for all init tests makes sense to me :) | 14:48 |
| tlater[m] | So please don't modify the API to make just one test slightly more comfortable :D | 14:48 |
| tlater[m] | (Although, by all means, write helper functions and use @pytest.mark.parametrize) | 14:49 |
| *** toscalix has quit IRC | 15:02 | |
| alexandrufazakas | tlater: Okay, I'm kind of back to step 1 with this issue rn, how should we look for -C if we pretty much always inject it in runcli.run()? | 15:10 |
| tlater[m] | alexandrufazakas: This sounds like you're really confused between what the tests are and what the actual code is | 15:12 |
| tlater[m] | Unless I'm completely misunderstanding you again | 15:12 |
| WSalmon | Is the CI broken? i have https://gitlab.com/BuildStream/buildstream/-/jobs/242078969 and https://gitlab.com/BuildStream/buildstream/-/jobs/242104423 are reruns of the exact same job but one has 2 failures and the other has 3 failures | 15:13 |
| benschubert | WSalmon: those are tests that seem super flaky | 15:21 |
| benschubert | I had to run a pipeline 5-6 times before it would go through | 15:21 |
| *** CTtpollard is now known as tpollard | 15:22 | |
| gitlab-br-bot | raoul.hidalgocharman approved MR !1431 (tar-target-renaming->master: tar.py: Make link target renaming work between base-dirs) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1431 | 15:41 |
| *** lachlan has quit IRC | 15:41 | |
| *** MarkShuttleworth has joined #buildstream | 15:41 | |
| *** adi has joined #buildstream | 15:42 | |
| *** adi has quit IRC | 15:42 | |
| *** MarkShuttleworth has quit IRC | 15:42 | |
| *** MarkShuttleworth has joined #buildstream | 15:43 | |
| *** adi has joined #buildstream | 15:43 | |
| *** MarkShuttleworth has quit IRC | 15:44 | |
| *** MarkShuttleworth has joined #buildstream | 15:45 | |
| *** lachlan has joined #buildstream | 15:46 | |
| *** MarkShuttleworth has quit IRC | 15:46 | |
| *** phildawson_ has joined #buildstream | 15:50 | |
| *** phildawson has quit IRC | 15:52 | |
| tlater[m] | Grmbl, magit-forge can't marge :| | 15:56 |
| *** tpollard has quit IRC | 15:58 | |
| *** lachlan has quit IRC | 15:59 | |
| *** tristan has quit IRC | 15:59 | |
| *** raoul has quit IRC | 15:59 | |
| *** paulsherwood has quit IRC | 15:59 | |
| *** Becky has quit IRC | 15:59 | |
| *** adds68 has quit IRC | 15:59 | |
| *** ikerperez has quit IRC | 15:59 | |
| *** samkirkham has quit IRC | 15:59 | |
| *** valentind has quit IRC | 15:59 | |
| *** Trevinho has quit IRC | 15:59 | |
| *** swick has quit IRC | 15:59 | |
| *** lantw44 has quit IRC | 15:59 | |
| *** SotK has quit IRC | 15:59 | |
| *** SotK_ has quit IRC | 15:59 | |
| *** SotK_ has joined #buildstream | 15:59 | |
| *** paulsherwood has joined #buildstream | 15:59 | |
| *** swick has joined #buildstream | 15:59 | |
| *** tristan has joined #buildstream | 15:59 | |
| *** valentind has joined #buildstream | 16:01 | |
| *** raoul has joined #buildstream | 16:01 | |
| *** lachlan has joined #buildstream | 16:01 | |
| *** samkirkham has joined #buildstream | 16:02 | |
| *** ikerperez has joined #buildstream | 16:02 | |
| *** adds68 has joined #buildstream | 16:02 | |
| *** Becky has joined #buildstream | 16:02 | |
| *** lantw44 has joined #buildstream | 16:02 | |
| *** Trevinho has joined #buildstream | 16:02 | |
| WSalmon | benschubert, looking at those mtimes being to the nearist second i wonder if something has changed with the runners so there files systems only run to the nearist second, i wonder if something has changed with the runners, we fixed this in the past by adding sleeps | 16:02 |
| *** SotK has joined #buildstream | 16:03 | |
| benschubert | WSalmon: so those tests are old? I haven't look at when they were added to the codebase | 16:03 |
| benschubert | I know that in my perf-optimisations branches, I get them more often, so that would be consistent | 16:03 |
| WSalmon | i have not had erros like that before today for my branch and i dont get them locally on my "normal" file system | 16:04 |
| WSalmon | jjardon, ironfoot valentind do any of you know if gnome have changed anything about the gnome provided runners? | 16:05 |
| WSalmon | we used to have very similar issues on the gitlab free tear digital ocean runners | 16:06 |
| WSalmon | this was how we fixed all issues for the free runners but i dont know if any one has been checking if we have kept new tests to the same standard https://gitlab.com/BuildStream/buildstream/merge_requests/595 | 16:08 |
| *** adi has joined #buildstream | 16:14 | |
| *** jonathanmaw has quit IRC | 16:14 | |
| WSalmon | just looking throught these, people have been adding m_time tests without adding wait_for_cache_granularity | 16:14 |
| WSalmon | :'( | 16:14 |
| WSalmon | (venvbuildstream) [will@localhost buildstream]$ git grep test_artifact_expires | 16:16 |
| WSalmon | tests/artifactcache/expiry.py:def test_artifact_expires(cli, datafiles): | 16:16 |
| WSalmon | tests/frontend/push.py:def test_artifact_expires(cli, datafiles, tmpdir): | 16:16 |
| WSalmon | also i thought the lint was now going to complain if we had two tests with the same name? | 16:16 |
| WSalmon | is this not a issue | 16:17 |
| benschubert | WSalmon: they are not in the same file, that's not a problem for a linter | 16:17 |
| WSalmon | oh ok, but is it ok in general? | 16:18 |
| benschubert | yep | 16:18 |
| WSalmon | ok | 16:18 |
| WSalmon | ta | 16:18 |
| WSalmon | right i will put up a mr adding wait_for_cache_granularity() were it has been missed in a miniute | 16:21 |
| gitlab-br-bot | tlater opened issue #1061 (Allow specifying element-plugin-wise default dependencies) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1061 | 16:23 |
| alexandrufazakas | tlater: I think I'm getting pretty close to wrapping this up, I've just pushed my changes. There's 2 init.py tests that fail for some reason right now and I'm not sure what the issue is. Let me know how it looks when you've some time | 16:29 |
| tlater[m] | Sure | 16:30 |
| * tlater[m] will probably do so right now, not that much energy left... ;p | 16:30 | |
| alexandrufazakas | Oh, I think I know why they fail | 16:31 |
| alexandrufazakas | tlater: let me update them real quick, sorry about that :/ | 16:31 |
| alexandrufazakas | Okay, I fixed them | 16:32 |
| alexandrufazakas | I uhh forgot to update project=None somewhere by mistake :D | 16:32 |
| WSalmon | benschubert, i have pushed up willsalmon/hotfix-cache-tests for testing have you had any other tests be really flaky on you i might be able to fix? | 16:35 |
| benschubert | right now, I don't think so :) | 16:45 |
| benschubert | thanks WSalmon | 16:45 |
| tlater[m] | alexandrufazakas: Awesome, so for the most part this is good | 16:47 |
| tlater[m] | You should rebase on top of your other branch once that lands, though. | 16:47 |
| tlater[m] | Comments are up, mostly nitpicking at this point | 16:47 |
| tlater[m] | No fundamental changes anymore :) | 16:47 |
| gitlab-br-bot | BenjaminSchubert opened MR !1438 (bschubert/node-api-nosanitize->bschubert/new-node-api: Remove the need for 'node_sanitize') on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1438 | 16:48 |
| benschubert | tristan: ^ might be of interest to you ;) | 16:49 |
| alexandrufazakas | tlater: thanks a lot for the review and all the help :) | 16:50 |
| alexandrufazakas | One more thing: https://gitlab.com/BuildStream/buildstream/merge_requests/1430#note_186580606 not sure this is right though | 16:50 |
| alexandrufazakas | In that case, main_options['directory'] is literally the string 'None' | 16:50 |
| alexandrufazakas | if i check using `if main_options['directory']`, this is evaluated to true, which is something we don't want | 16:51 |
| tlater[m] | Wait, why is it literally the string 'None'? | 16:51 |
| alexandrufazakas | Heh | 16:51 |
| tlater[m] | That'd mean you can't name your project 'None' | 16:52 |
| tlater[m] | That's stupid | 16:52 |
| alexandrufazakas | True | 16:52 |
| alexandrufazakas | I'm not sure why that's the case | 16:52 |
| tlater[m] | It's set to None here: https://gitlab.com/BuildStream/buildstream/merge_requests/1430/diffs#48a5425791d6d9d502dfdb9fad1e14e38ec05629_220_220 | 16:52 |
| alexandrufazakas | I think I do something like str() | 16:52 |
| tlater[m] | No, nevermind | 16:53 |
| tlater[m] | It's the bug in the test suite | 16:53 |
| tlater[m] | That's why I said land your other patch first | 16:53 |
| tlater[m] | (This is exactly why I told you to fix that this afternoon ;p) | 16:53 |
| alexandrufazakas | oh, alright | 16:53 |
| alexandrufazakas | I *did* open an MR on that | 16:54 |
| alexandrufazakas | But some tests failed there for some reason | 16:54 |
| tlater[m] | Yeah, CI broke | 16:54 |
| alexandrufazakas | Yep | 16:54 |
| tlater[m] | We have intermittent test failures today | 16:54 |
| tlater[m] | We occasionally have those | 16:54 |
| alexandrufazakas | It's alright | 16:54 |
| alexandrufazakas | I'll just rebase on top of that once it's in | 16:54 |
| tlater[m] | They're like "friends" who come over every couple of weeks and you just really want them to go again | 16:54 |
| alexandrufazakas | rofl | 16:55 |
| tlater[m] | Although they can be pretty fun sometimes, you don't want them there right now | 16:55 |
| tlater[m] | Hang, lemme restart your pipeline, we'll get that merged | 16:55 |
| alexandrufazakas | tlater: I manually restarted the ones that failed | 16:55 |
| alexandrufazakas | Maybe there is something wrong with that patch haha | 16:56 |
| alexandrufazakas | tlater: I'll get going, I'll fix everything on !1430 on Monday. o/ | 16:58 |
| gitlab-br-bot | MR !1430: Add bst init argument https://gitlab.com/BuildStream/buildstream/merge_requests/1430 | 16:58 |
| *** alexandrufazakas has quit IRC | 16:58 | |
| tlater[m] | o/ | 16:58 |
| *** phildawson_ has quit IRC | 17:12 | |
| *** lachlan has quit IRC | 17:33 | |
| WSalmon | benschubert, tristan juergbi tlater[m] https://gitlab.com/BuildStream/buildstream/merge_requests/1439 | 18:11 |
| *** bochecha has joined #buildstream | 18:23 | |
| *** WSalmon has quit IRC | 18:34 | |
| *** WSalmon has joined #buildstream | 18:34 | |
| *** brlogger has joined #buildstream | 22:39 | |
| *** paulsherwood has joined #buildstream | 22:41 | |
| *** benbrown has joined #buildstream | 22:41 | |
| *** rafaelff[m] has joined #buildstream | 22:47 | |
| *** bochecha has quit IRC | 23:03 | |
| *** dineshdb[m] has joined #buildstream | 23:07 | |
| *** reuben640[m] has joined #buildstream | 23:21 | |
| *** Demos[m] has joined #buildstream | 23:24 | |
| *** pro[m] has joined #buildstream | 23:32 | |
| jjardon | WSalmon: no, are you seeing any errors in the gnome runners? Do you have a link to a fail job? | 23:34 |
| *** doras[m] has joined #buildstream | 23:36 | |
| *** cgmcintyre[m] has joined #buildstream | 23:40 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!