| *** narispo has quit IRC | 00:06 | |
| *** narispo has joined #buildstream | 00:08 | |
| *** narispo has quit IRC | 02:25 | |
| *** narispo has joined #buildstream | 02:25 | |
| *** narispo has quit IRC | 05:00 | |
| *** narispo has joined #buildstream | 05:03 | |
| *** narispo has quit IRC | 06:18 | |
| *** narispo has joined #buildstream | 06:19 | |
| *** narispo has quit IRC | 06:23 | |
| *** narispo has joined #buildstream | 06:28 | |
| *** narispo has quit IRC | 06:39 | |
| *** narispo has joined #buildstream | 06:40 | |
| *** narispo has quit IRC | 07:36 | |
| *** traveltissues has joined #buildstream | 07:42 | |
| *** narispo has joined #buildstream | 08:00 | |
| *** ikerperez has joined #buildstream | 08:32 | |
| *** paulsherwood has joined #buildstream | 08:51 | |
| *** paulsherwood has joined #buildstream | 08:51 | |
| *** paulsher1ood has joined #buildstream | 09:36 | |
| *** tiagogomes has joined #buildstream | 09:48 | |
| coldtom | looks like buildstream master can't be installed using the command in the docs? `pip3 install --user -e .` fails, i think due to https://github.com/pypa/pip/issues/6434 | 09:52 |
|---|---|---|
| benschubert | coldtom: ah, --user and -e are incompatible if I recall correctly? Or is is --target and -e? | 09:56 |
| coldtom | i think it's pep517 with build-backend and -e which is broken | 09:59 |
| coldtom | at least, --user -e works for 1.4 | 10:01 |
| *** tme5 has joined #buildstream | 10:03 | |
| *** phildawson has joined #buildstream | 10:10 | |
| benschubert | I always run pip install -e buildstream, not sure what version of pip I have though | 10:22 |
| juergbi | it might no longer work since cython, don't remember exactly | 10:25 |
| juergbi | pip install -e with venv works fine, though | 10:25 |
| * coldtom does a bit more digging | 10:25 | |
| tpollard | -e doesn't work for me | 10:28 |
| tpollard | pip 19.3.1, python 3.7 | 10:28 |
| *** douglaswinship has joined #buildstream | 10:29 | |
| *** jonathanmaw has joined #buildstream | 10:29 | |
| coldtom | yeah, seems to be only --user which causes it to break, looks like the --user gets passed to a python invocation somewhere | 10:32 |
| tpollard | setup.py 'error: option --user not recognized', works fine without -e though | 10:33 |
| juergbi | yes, --user and -e are incompatible with our current setup. don't remember what broke it | 10:34 |
| juergbi | I think there was a bit of discussion about this on IRC when it broke and the conclusion was that our recommendation should anyway be to use venv, where --user is not needed and -e works | 10:34 |
| tpollard | sounds reasonable for development | 10:35 |
| tme5 | fielding suggestions: if I'm merging tag tracking, it will need a different config name to 'track-tags' as that's already in use | 11:06 |
| tme5 | 'track-latest-tag' ? | 11:07 |
| coldtom | 'track-latest-tag' sounds good to me, or perhaps 'auto-update-tags' or something similar | 11:14 |
| tme5 | the latter sounds more like an alternative name for the current track-tags behaviour | 11:18 |
| tme5 | i think 'track-latest-tag' is ok too, just kind of sounds like it's an extra option to track-tags | 11:19 |
| tlater[m] | Grmbl grmbl | 11:28 |
| tlater[m] | Stupid automake | 11:28 |
| tlater[m] | Why does your mirror never work :( | 11:28 |
| tme5 | is there a reason why hard-coded Git committer and author dates are part of the test env? | 11:35 |
| tlater[m] | tme5: Where exactly? | 11:36 |
| tme5 | testing/_utils/site.py | 11:36 |
| tpollard | I presume so the git tests work correctly on systems where a user hasn't been configured? | 11:37 |
| tme5 | but why the dates? | 11:37 |
| tlater[m] | tme5: Reproducibility, so the commits always end up being the same | 11:37 |
| tlater[m] | We test making commits | 11:38 |
| tlater[m] | And we assert that the commits end up right | 11:38 |
| tme5 | commits don't need to be hard-coded though, there is repo.latest_commit | 11:38 |
| tlater[m] | tme5: Which uses that | 11:39 |
| tlater[m] | It looks like it's used so that junction shas end up the same every time | 11:40 |
| tme5 | ah, which test suite? | 11:41 |
| tlater[m] | `testing/_utils/junction.py` configures junctions with git | 11:41 |
| tlater[m] | So anything that uses a junction will be affected by that | 11:41 |
| tme5 | there is no hard-coding of refs here though right? | 11:42 |
| tlater[m] | tme5: It will always generate the same ref | 11:43 |
| tme5 | _SimpleGit.create returns the commit by latest_commit | 11:43 |
| tme5 | but why is that required? | 11:43 |
| tlater[m] | Presumably element definitions will reference specific refs | 11:43 |
| tlater[m] | It makes it easier to write tests that track in specific ways. | 11:43 |
| tlater[m] | Sure, you can work around that | 11:44 |
| tlater[m] | But I don't see how reproducibility in our test suite hurts | 11:44 |
| tme5 | the reason I ran into this is because i'm testing a feature which compares several branches by their latest commit date and it was messing that up | 11:45 |
| tlater[m] | tme5: Ah, you'll need that if you scrub tags and stuff, I suppose | 11:46 |
| tlater[m] | Well, I think we could use API for specifying dates on commits. | 11:47 |
| tlater[m] | A little hard to implement from a plugin testing point... | 11:47 |
| tme5 | i've added that already | 11:48 |
| tme5 | but it requires more mucking around with instance variables | 11:48 |
| tlater[m] | Why would it? Could it not just be a kwarg on one or two methods? | 11:49 |
| tme5 | (repo.Git.env) | 11:49 |
| tme5 | yeah it's not difficult, it's just not as simple as --date to git commit because that only changes the author date and I am wanting to change the committer date | 11:49 |
| tlater[m] | Right | 11:50 |
| tlater[m] | Any reason you can't use the author date for your tests? | 11:50 |
| tme5 | because the relevant functionality ought (at least i think) to compare committer dates | 11:51 |
| tlater[m] | Well, fair enough | 11:51 |
| tme5 | thanks for the help with the rationale | 11:53 |
| gitlab-br-bot | tpollard closed issue #1205 (Improve messaging around state of junction elements) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1205 | 12:18 |
| gitlab-br-bot | tpollard merged MR !1764 (tpollard/showjunction->master: _frontend/widget.py: show_pipeline() Don't show buildable for junction) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1764 | 12:18 |
| benschubert | tpollard: ^ nice one :) | 12:24 |
| tpollard | do you have any opinion on !1788 juergbi? | 12:40 |
| gitlab-br-bot | MR !1788: scheduler.py: Handle exceptions that are caught under the event loop https://gitlab.com/BuildStream/buildstream/merge_requests/1788 | 12:40 |
| juergbi | tpollard: let me take a look | 12:41 |
| juergbi | tpollard: in what scenario is context.get("exception") set/unset? | 12:45 |
| juergbi | I would have expected us to trigger a BUG message in any exception case | 12:46 |
| tpollard | the docs don't really specify | 12:46 |
| tpollard | which is annoying | 12:47 |
| tpollard | 'context is a dict object containing the following keys (new keys may be introduced in future Python versions)' with exception being optional | 12:47 |
| tpollard | BUG should be raised regardless though | 12:48 |
| juergbi | ah, by the app handler | 12:48 |
| tpollard | yep | 12:48 |
| tpollard | but if there's no exception object it won't halt, just message | 12:49 |
| juergbi | tpollard: wouldn't it make sense to create an exception object if there isn't one? | 12:50 |
| juergbi | and then follow the first path | 12:50 |
| juergbi | that way it will halt etc. | 12:50 |
| tpollard | I don't see an issue with doing that, but would be more confident if I knew the condition in which the exception wasn't given | 12:55 |
| juergbi | sure, that would definitely be good to know. however, if we don't know more, reducing the number of exception code paths makes sense to me | 12:58 |
| juergbi | overall it seems odd that we have to call excepthook() directly but I don't know of a better alternative | 12:59 |
| juergbi | and I expect it to work. I assume you've tested it with that revert | 12:59 |
| tpollard | one one hand I'd like async to not do any handling once it's passed the exception to a custom handler | 12:59 |
| juergbi | testing in the test suite is probably not easily possible | 12:59 |
| tpollard | but I do get that it might be a legitimate error if a custom handler ends up raising an exception itself | 13:00 |
| juergbi | I guess the expectation from asyncio is that the loop is owned by something like our App object | 13:01 |
| juergbi | and thus the App would directly set the exception handler | 13:02 |
| tpollard | could we monkey patch the buildstream instance (say, with the example I gave in the MR) for the test case? | 13:02 |
| juergbi | could be possible but not sure whether we can easily do it in a robust way | 13:03 |
| juergbi | also need to make sure other tests are not affected | 13:03 |
| tpollard | I'll update it to generate an exception in the optional case | 13:08 |
| tpollard | juergbi: although that leaves us with having to infer what type of error it should be | 13:23 |
| tpollard | in consistencybug.py we just raise a generic Exception() | 13:30 |
| tpollard | juergbi: I'd go with creating a new scheduler error type or a generic exception, what do you think? | 13:42 |
| *** narispo has quit IRC | 13:57 | |
| *** narispo has joined #buildstream | 13:58 | |
| *** traveltissues has quit IRC | 14:37 | |
| tme5 | Just pushed to !1774. Any comments welcome :) It is incomplete but it can be considered finished for the functionality implemented so far. | 14:55 |
| gitlab-br-bot | MR !1774: WIP: Merge 'git_tag' functionality into 'git' source https://gitlab.com/BuildStream/buildstream/merge_requests/1774 | 14:55 |
| gitlab-br-bot | tpollard closed issue #1084 (stack trace with python 3.7 when exiting client with `^C`) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1084 | 15:09 |
| juergbi | tpollard: as we don't even know when this can happen and we essentially need the exception just as a container for the message, a generic exception is ok, imo | 15:19 |
| *** narispo has quit IRC | 15:26 | |
| *** narispo has joined #buildstream | 15:26 | |
| *** narispo has quit IRC | 15:29 | |
| tme5 | fielding more suggestions: the 'match' and 'exclude' options ought to be copied over from git_tag too, but they need to be renamed | 15:42 |
| coldtom | tag-match, tag-exclude? | 15:42 |
| tme5 | seeing as they are only relevant when 'track-latest-tag' is set, I considered making them sub-options of that | 15:43 |
| tme5 | so you can write `track-latest-tag: True` or `track-latest-tag: match: - ... exclude: - ...` (newlines left to imagination) | 15:43 |
| tme5 | tlater[m], the other day you discussed with me loading a config key as either one or two types. Do you know how to do that without catching the private API LoadError exception? | 15:51 |
| tme5 | *either one of two | 15:52 |
| tlater[m] | I did not know LoadError was private API | 15:52 |
| tlater[m] | But I believe it to be a generic BstError | 15:52 |
| tlater[m] | So you can definitelly catch it as a BstError, in the worst case | 15:52 |
| tlater[m] | If it is private API we should make it public though. If it can be thrown from public API methods, it should not be private | 15:53 |
| tme5 | it is buildstream._exceptions.LoadError, so i assumed it's private? is that what _ means? | 15:53 |
| tlater[m] | Yep | 15:54 |
| tme5 | BstError is also private | 15:54 |
| tlater[m] | I've not written many plugins myself, so I rarely face that problem; don't know the API surface that well. | 15:54 |
| tlater[m] | Yep | 15:54 |
| tlater[m] | I'd say, mailing list post complaining about LoadError being private | 15:54 |
| coldtom | I believe that they are public at the `buildstream` level though, aren't they? | 15:54 |
| tlater[m] | Is the way to handle this | 15:55 |
| tlater[m] | coldtom: Any module that starts with _ is private | 15:55 |
| *** narispo has joined #buildstream | 15:55 | |
| tlater[m] | "private" in BuildStream terminology means "only use this within buildstream" | 15:55 |
| tlater[m] | If you're writing an external plugin, we don't guarantee that anything you access through a _ will stay there | 15:55 |
| coldtom | ah, my brain broke and thought it'd seen BstError used in plugins before, but ofc it was ElementError or SourceError | 15:55 |
| tpollard | plugin.py has public api that can raise LoadError | 15:57 |
| tlater[m] | Yeah, that's definitely an oversight | 15:57 |
| tlater[m] | We should make LoadError public, or wrapper that | 15:57 |
| *** narispo has quit IRC | 15:57 | |
| gitlab-br-bot | tmewett opened issue #1248 (Some public API functions can raise private LoadError exception) on buildstream https://gitlab.com/BuildStream/buildstream/issues/1248 | 16:09 |
| *** phildawson_ has joined #buildstream | 16:31 | |
| *** phildawson has quit IRC | 16:33 | |
| tme5 | i suppose I ought to have added my recurse submodule change to the changelog? | 16:58 |
| tpollard | in NEWS yep | 17:00 |
| tpollard | easily missed though | 17:00 |
| tme5 | alright, i'll add that in a separate MR some time | 17:21 |
| benschubert | tme5: ^ any reason why you would need catching LoadError? | 17:23 |
| tme5 | i'm trying to allow a config option to be one of two types | 17:24 |
| tpollard | juergbi: updated it with a generic exception | 17:24 |
| benschubert | tme5: https://buildstream.gitlab.io/buildstream/buildstream.node.html#buildstream.node.MappingNode.get_node is that not enough? Or do you have other requirements? | 17:25 |
| benschubert | I'm happy improving the node api to extend it if needed | 17:25 |
| tme5 | do I then just check the class type of the returned value? | 17:26 |
| benschubert | yup | 17:26 |
| tme5 | what if I have further requirements, like that the scalar is a bool, or a sequence that is a str list? | 17:27 |
| benschubert | (unless you have a better idea for this, but since it can be two different types, not sure what else can be done?) | 17:27 |
| benschubert | you can do: | 17:28 |
| benschubert | node = mapping.get_node("key", allowed_types=[ScalarNode,SequenceNode]) | 17:28 |
| benschubert | if type(node) is ScalarNode: | 17:28 |
| benschubert | value = node.as_bool() | 17:28 |
| benschubert | ellse: | 17:28 |
| benschubert | value = node.as_str_list() | 17:28 |
| benschubert | And it would throw if it is misformed | 17:28 |
| tme5 | ah, yeah that would work i think, thanks | 17:30 |
| tme5 | though i still think exceptions thrown by public API functions should be public | 17:30 |
| benschubert | The reason for this one not to be public, is that we don't want plugins to start catching and throwing their custom errors, that way we can ensure that all errors of bst file formats are consistent :) | 17:35 |
| tme5 | that seems sensible | 17:43 |
| tme5 | although, it won't stop people in the wild from just putting the methods in try-except blocks anyway | 17:44 |
| benschubert | That's correct | 17:45 |
| tlater[m] | benschubert: Are we trying to prevent others from implementing BstError? | 17:45 |
| benschubert | As it won't prevent them from changing any parts of BuildStream, it's python so we have to live with that. I just believe that the 'official' stance should be restrictive and guiding enough so that most users get a good experience :) | 17:46 |
| tlater[m] | Or is this about custom LoadErrors? | 17:46 |
| benschubert | tlater[m]: correct, BstError is a BuildStream exception, not something for plugins, they have ElementError and SourceError | 17:46 |
| benschubert | tlater[m]: correct | 17:46 |
| tlater[m] | Ah, yes, ok. | 17:46 |
| tlater[m] | I'd like to see those exceptions wrappered into Element/SourceErrors | 17:46 |
| tlater[m] | Because then plugins can use them properly | 17:46 |
| tlater[m] | Because even if they aren't part of public API, they are still being thrown from public API | 17:47 |
| benschubert | Element and Source errors are for plugins to extend and implement, not really to catch :) | 17:47 |
| tlater[m] | And it's hard to work with that if you can't catch those exceptions | 17:47 |
| benschubert | Why? All the methods should be there to be able to work with them consistenly _unless_ they have a format error | 17:48 |
| benschubert | and that's not for plugins to handle, that's for BuildStream | 17:48 |
| tlater[m] | Plugins may still want to handle format errors | 17:49 |
| tlater[m] | For reasons like the above, or version deprecation or such | 17:49 |
| benschubert | Why? In which case? | 17:49 |
| tlater[m] | Even if it's just modifying the message BuildStream sends | 17:49 |
| tlater[m] | I imagine for example that a plugin may have a "git_tag" option in one version | 17:49 |
| benschubert | For reasons like above they have apis to get without exception for this | 17:49 |
| tlater[m] | But then two versions down that is deprecated | 17:49 |
| benschubert | and for the other, you can always Node.get_(..., default=...) and act on the default to say it's deprecated | 17:50 |
| tlater[m] | Hmm | 17:50 |
| tlater[m] | Well, I still don't like that we're essentially leaving plugin authors with exceptions they can't handle. | 17:50 |
| benschubert | Well, there are many that could go wrong, techinically even segfaults if the cython code is not correct | 17:50 |
| benschubert | plugin authors are not meant to handle most exceptions | 17:51 |
| tlater[m] | Is there any way we can make more obvious that this is the goal? | 17:51 |
| tlater[m] | It definitely goes against normal python coding flow | 17:51 |
| benschubert | not necessarily, many libraries do the same | 17:51 |
| benschubert | https://buildstream.gitlab.io/buildstream/buildstream.node.html#node-parsed-yaml-configuration last paragraph tries to convey this | 17:52 |
| benschubert | not sure how to make it clearer | 17:52 |
| tme5 | say "do not catch errors thrown by these functions!" :P | 17:52 |
| tlater[m] | Yep | 17:52 |
| tme5 | or "you should not" | 17:52 |
| benschubert | fair , I'll happily review a PR on this or update it when I've got time :) | 17:53 |
| tlater[m] | "Exceptions thrown by these methods are not expected to be handled by plugins; the relevant exceptions are private API" | 17:53 |
| tlater[m] | Might also be handy to note in a plugin writing doc | 17:53 |
| tlater[m] | Not sure we have one | 17:54 |
| *** tme5 has quit IRC | 17:56 | |
| gitlab-br-bot | tlater opened MR !1791 (tlater/node-exceptions->master: node.pyx: Add note on node exceptions in plugins) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/1791 | 18:02 |
| *** slaf_ has joined #buildstream | 18:21 | |
| *** slaf_ has joined #buildstream | 18:21 | |
| *** slaf_ has joined #buildstream | 18:22 | |
| *** slaf_ has joined #buildstream | 18:22 | |
| *** slaf has quit IRC | 18:23 | |
| *** slaf_ is now known as slaf | 18:23 | |
| *** phildawson_ has quit IRC | 18:36 | |
| *** phildawson_ has joined #buildstream | 18:37 | |
| *** tiagogomes has quit IRC | 18:38 | |
| *** jonathanmaw has quit IRC | 18:47 | |
| *** phildawson_ has quit IRC | 19:34 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!