| *** Prince781 has joined #buildstream | 00:34 | |
| *** Prince781 has quit IRC | 01:51 | |
| *** Prince781 has joined #buildstream | 01:51 | |
| *** Prince781 has quit IRC | 02:05 | |
| *** Prince781 has joined #buildstream | 02:11 | |
| gitlab-br-bot | buildstream: merge request (135-expire-artifacts-in-local-cache->master: WIP: Resolve "Expire artifacts in local cache") #347 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/347 | 02:42 |
|---|---|---|
| *** Prince781 has quit IRC | 02:47 | |
| *** Prince781 has joined #buildstream | 02:54 | |
| *** Prince781 has quit IRC | 05:30 | |
| *** Guest45103 is now known as ernestask[m] | 07:16 | |
| *** finn has joined #buildstream | 07:31 | |
| *** bochecha has joined #buildstream | 07:40 | |
| *** adds68 has joined #buildstream | 07:54 | |
| *** coldtom has joined #buildstream | 07:59 | |
| *** bethw has joined #buildstream | 08:02 | |
| *** Phil has joined #buildstream | 08:15 | |
| *** dominic has joined #buildstream | 08:30 | |
| *** benbrown has joined #buildstream | 08:30 | |
| *** jonathanmaw has joined #buildstream | 08:45 | |
| *** jmac has joined #buildstream | 08:56 | |
| *** Nexus has joined #buildstream | 09:16 | |
| *** toscalix has joined #buildstream | 11:42 | |
| *** toscalix has quit IRC | 11:47 | |
| *** noisecell has quit IRC | 12:16 | |
| *** noisecell has joined #buildstream | 12:30 | |
| *** noisecell has joined #buildstream | 12:30 | |
| *** krichter[m] has joined #buildstream | 12:35 | |
| *** noisecell has quit IRC | 12:41 | |
| *** noisecell has joined #buildstream | 12:41 | |
| *** pro[m] has joined #buildstream | 12:47 | |
| jennis | Hi guys, I'd like to discuss a potential optimisation: I've just tried to `track` an element which has a git source, I looked at the git repo and noticed that track was not updating the ref to the latest ref | 13:15 |
| jennis | From this, I then discovered the element itself did not specify a `track:` field | 13:16 |
| jennis | So adding `track: master` to the element, I was then able to `bst track <element>` and this updated the ref | 13:16 |
| jennis | However, I thought perhaps we should define a built-in that automatically uses `track: master` for git sources? | 13:17 |
| jennis | I guess for now, I only require thoughts on whether I this is worth writing an issue about... | 13:17 |
| jennis | on whether this* | 13:17 |
| tlater | jennis: I find these days it's easier to create an issue and have it closed as not relevant ;p I for one think this might be n | 13:19 |
| tlater | ice for new users | 13:19 |
| Phil | jennis, that sounds sensible. In fact, that was what I assumed it did. | 13:20 |
| Phil | until I tried it and it didn't work | 13:20 |
| jennis | :p | 13:20 |
| jennis | I also think that this is potentially a nice optimisation for a newcomer to implement | 13:21 |
| tlater | Phil, jennis: I believe the intention was to mimick git as closely as possible | 13:21 |
| tlater | Which doesn't set a default | 13:21 |
| tlater | But I also think 'track: master' makes sense :) | 13:22 |
| noisecell | jennis, as a curiosity if the ref is not in master but in any other branch how the element behaves? | 13:22 |
| noisecell | when you try to track it? | 13:22 |
| jennis | ahh that's a good thought | 13:22 |
| tlater | (Currently tracking in that case does nothing) | 13:23 |
| noisecell | then the assumption to set up master is wrong ^^ | 13:23 |
| noisecell | though | 13:23 |
| jennis | mhm yeah, that's an odd usecase | 13:23 |
| noisecell | not that odd... | 13:23 |
| noisecell | people can forget to add fields | 13:23 |
| jennis | Well for my proposal | 13:23 |
| tlater | noisecell: I don't think it's wrong, per se | 13:23 |
| tlater | You can *still* set the branch to something else | 13:24 |
| tlater | And if you don't want to track certain elements, --deps/--except is how that should be handled, not by setting an empty 'track' | 13:24 |
| noisecell | tlater, well, if you are changing the ref of that element then you are not building anymore that ref... which will be updated to master ref, if Im not mistaken? | 13:24 |
| jennis | Mmhm I'm wondering whether it's possible to determine which branch the ref belongs to and track that as default | 13:25 |
| tlater | noisecell: Yes, that is true, I don't think that's an issue, because yo u will either explicitly set it to another branch in that case, or explicitly except that element from your track | 13:25 |
| noisecell | tlater, you are missing that people can make mistakes and this will create a burden for them | 13:26 |
| jennis | ^^ maybe we also implement a track: None option or something like that | 13:26 |
| noisecell | if you have loads of elements | 13:26 |
| tlater | Hm, I suppose that if you forget to set 'track: master' that's less of a burden | 13:26 |
| tlater | Yes, nevermind, I agree | 13:26 |
| dominic | it seems like it's already caused people issues | 13:26 |
| tlater | dominic: Sort of, but currently what happens is "wait, why didn't that update?" | 13:27 |
| tlater | If we changed it, you'd lose the ref, and have to painstakingly backtrack what it was | 13:27 |
| dominic | hmm | 13:27 |
| Phil | I guess at least as it is it's obvious something isn't right | 13:27 |
| tlater | I think the current inconvenience is better than what the suggested change would cause | 13:27 |
| noisecell | maybe add a warning, saying you haven't set a track, would be my suggestion | 13:27 |
| tlater | A warning could help :) | 13:27 |
| noisecell | snap :) | 13:27 |
| tlater | jennis: track: None is not an option, because you may call a branch None | 13:28 |
| jennis | ahh good point | 13:29 |
| jennis | I like the warning idea then | 13:29 |
| * tlater has missed buildstream warnings before, but I suppose it's not too bad in this case | 13:29 | |
| jennis | but just to clarify, would this be a warning for "your element doesn't specify a track" or is it a warning for "we've tracked master by default"? | 13:30 |
| tlater | jennis: Given that the latter is potentially destructive, we should stick with the former | 13:31 |
| noisecell | jennis, I would go for "your element doesn't specify a track, so no track is possible" | 13:31 |
| jennis | Ok, that makes sense to me, thanks all | 13:31 |
| jennis | I'll write up an issue lter | 13:31 |
| * tlater takes note that this time looks like a good time to discuss annoyances ;) | 13:32 | |
| noisecell | heh | 13:32 |
| noisecell | tlater, did you have time to look at the proposal of adding or moving functionality to "bst artifact" ? | 13:33 |
| jennis | git diff | 13:33 |
| tlater | noisecell: I had a quick look at it. No strong opinions other than it looks nice. | 13:33 |
| jennis | whoops lol | 13:33 |
| * tlater has been taking most of his free time to work on the artifact cache, so hasn't really had time to spend on anything else | 13:34 | |
| noisecell | tlater, thanks :) | 13:34 |
| gitlab-br-bot | buildstream: merge request (relative_workspaces->master: WIP: Patch for issue #191 support relative workspaces) #504 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/504 | 14:01 |
| *** toscalix has joined #buildstream | 14:24 | |
| gitlab-br-bot | buildstream: merge request (relative_workspaces->master: WIP: Patch for issue #191 support relative workspaces) #504 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/504 | 14:38 |
| *** toscalix has quit IRC | 14:42 | |
| *** toscalix has joined #buildstream | 14:46 | |
| *** tristan has joined #buildstream | 15:16 | |
| tiago | How do folks use print statements in buildstream? I fear mine are being swallowed | 15:30 |
| jmac | you're adding print statements to the source code? | 15:31 |
| tiago | for debugging purposes only | 15:32 |
| jmac | You can do it if you pipe output to something else, e.g. 'bst build bootstrap.bst | tee bstlog' | 15:32 |
| jmac | That stops it using fancy output | 15:33 |
| jmac | Usually it's better to use 'self.warn' if you are in an element/plugin or have access to one | 15:33 |
| tiago | Thanks. I saw that, but I am not on a plugin | 15:34 |
| jonathanmaw | jmac: I think you can accomplish the same by using "--no-interactive" | 15:34 |
| jonathanmaw | (as an alternative to piping output) | 15:34 |
| tristan | tiago, I sometimes do sys.stderr.write(), where are the prints coming from ? | 15:35 |
| tiago | That's one import away :) From sandbox.py | 15:36 |
| tristan | yeah, usually the import sys is directly above it when I do that heh | 15:37 |
| tristan | I think writing to stderr will do it from sandbox.py | 15:37 |
| tristan | there is a way to do that with print() also, but I think it anyway means importing sys for sys.stderr | 15:38 |
| jonathanmaw | tristan, I'm having some difficulty understanding the right way to ensure backward compatibility when changing the function signature for Source.fetch() and Source.track() (i.e. I've added an alias_override arg). Should I be using BST_FORMAT_VERSION to check whether the sources support the old API or a newer version? | 15:51 |
| jonathanmaw | Going by the description, I don't think it's quite right, because the versioning is meant to be per version of the plugin (but internal plugins are kept in sync with buildstream as a convenience), rather than something I can use so that buildstream is aware of what version the plugins are. | 15:52 |
| *** bochecha has quit IRC | 16:01 | |
| *** bochecha has joined #buildstream | 16:01 | |
| *** Phil has quit IRC | 16:03 | |
| *** bochecha has quit IRC | 16:06 | |
| tristan | jonathanmaw, nope, that's not how it works | 16:11 |
| tristan | jonathanmaw, or... unless maybe there is a python feature I dont know about | 16:12 |
| tristan | jonathanmaw, What I'd shoot for though is not do have to do that kind of conditional, and carefully grow the signature such that people calling the old way still get their expected results | 16:13 |
| tristan | we do have some flexibility with mandatory args, optional args and keyword only args | 16:13 |
| tristan | alias_override huh | 16:13 |
| tristan | jonathanmaw, that sounds wrong, like as if we're passing a new parameter and expecting plugins to know what it means | 16:14 |
| jonathanmaw | tristan, hrm, maybe I'm missing a python feature, then. I was getting TypeErrors because I was passing args into functions that weren't expecting them. | 16:14 |
| tristan | jonathanmaw, right, that might require a looser definition of the method | 16:15 |
| tristan | jonathanmaw, adding an optional or keyword_only argument should "work" | 16:16 |
| tristan | at that level | 16:16 |
| tristan | but the expectation that plugins will treat the new parameter would be incorrect | 16:16 |
| tristan | did we eliminate the possibility of getting this to work seamlessly with the existing public API, via Source.resolve_url() ? | 16:17 |
| jonathanmaw | tristan: I could get it working without changing the existing public API, but it involved some unpleasant hacks | 16:19 |
| jonathanmaw | So I've been trying to implement it in a nicer way based off a suggestion from valentind | 16:19 |
| tristan | jonathanmaw, unpleasant hacks sound alright to be honest | 16:19 |
| tristan | The reinstantiation could be done fairly cleanly, maybe there are some ugly corner cases to that | 16:20 |
| tristan | but... it doesnt break API | 16:20 |
| tristan | jonathanmaw, basically, we can teach plugins to do new things, and then the core has to accept that some plugins dont do those things | 16:21 |
| tristan | or, we can use the things that the plugin already does | 16:21 |
| tiago | I was thinking to tackle issue 289. What's desirable to do here, check if there's a shell in the sandbox and abort with an appropriate error if not? Or just avoiding running the strip-commands if no install-commands were defined | 16:22 |
| jonathanmaw | tristan: okay, I can have the branch that doesn't make API changes ready for review in a few minutes | 16:22 |
| *** coldtom has quit IRC | 16:22 | |
| tristan | tiago, I think juergbi and I discussed it and came fairly close to a conclusion that we could have Sandbox raise an error when it's asked to run a command which it knows does not exist in the runtime | 16:23 |
| tristan | tiago, there may be some gray area as to, whether we only do it for the shell, or if we expect a full path... probably better to use the sandbox PATH, only check the first thing in the sandbox argv | 16:24 |
| tiago | Hmm, if the command doesn't exist, the shell will already tell you about. In this case, wouldn't be better to just check for the existence of the shell? | 16:25 |
| tristan | jonathanmaw, what was the API changes you introduced and was there a plan for how existing plugins would behave while ignoring the new param ? | 16:25 |
| tristan | or, maybe you cannot add the param because the source implementations themselves override the signature with a fixed signature | 16:25 |
| tristan | tiago, yes and no | 16:26 |
| tristan | tiago, so under *no* circumstance would we be parsing the scripts and checking for more than one program | 16:27 |
| tristan | tiago, i.e. if we ran the shell, then as you say, the shell will notify of anything else | 16:27 |
| tristan | but if we don't run the shell, like if we run /bin/find directly with no shell... then there is no shell to tell us what's up | 16:28 |
| tristan | tiago, the BuildElement runs a shell for it's "commands", not everything does | 16:28 |
| tristan | (or, there are ways to invoke things directly) | 16:28 |
| tiago | Ok :) | 16:28 |
| jonathanmaw | tristan: The API changes in the API-changing branch are: | 16:33 |
| jonathanmaw | * A class called SourceDownloader now exists, and Source derives from it. | 16:33 |
| jonathanmaw | - SourceDownloader now holds the fetch and track methods | 16:33 |
| jonathanmaw | - SourceDownloader also has a get_alias method, so we don't need to intercept translate_url calls to figure out which aliases are being used. | 16:33 |
| jonathanmaw | - By default it checks whether an original_url field exists, and extracts the alias from there. | 16:33 |
| jonathanmaw | * Source now has the get_source_downloaders method, which is expected to return a list of SourceDownloaders. For most sources (i.e. ones that only ever download from one URL, e.g. everything but git sources) this defaults to returning a list containing itself. | 16:33 |
| jonathanmaw | - get_source_downloaders takes the alias_override as an arg, as the git source proves we need to fetch the repo before we can know what the submodules are. | 16:34 |
| jonathanmaw | * The fetch method has been extended from fetch(self) to fetch(self, alias_override=None) | 16:34 |
| jonathanmaw | * The track method has been extended from track(self) to track(self, alias_override=None) | 16:34 |
| jonathanmaw | (big paste ends here) | 16:35 |
| tristan | jonathanmaw, errrm, ok is sounds generally sane but, was it planned how existing, unmodified source implementations were going to work under this design ? | 16:41 |
| tristan | jonathanmaw, there may be a better way than reinstantiation (although I think the reinstantiation is a very nice way to avoid the API break and leverage something existing) | 16:42 |
| jonathanmaw | tristan: I was labouring under the misapprehension that the unmodified source implementations would ignore the additional args and behave as normal | 16:43 |
| tristan | Ahhhh | 16:44 |
| tristan | jonathanmaw, ok so in this view basically... unmodified plugins simply never make use of mirrors ? | 16:44 |
| jonathanmaw | yep | 16:44 |
| tristan | but still we put some of the business logic into the source implementations | 16:45 |
| tristan | which is... meh has it's ups and downs | 16:45 |
| tristan | jonathanmaw, your alternative approach is still possible, you just can't use a function argument to communicate it | 16:45 |
| tristan | there are other ways for a function to obtain a named value | 16:46 |
| tristan | still, there is the other side to look at... A.) The core has this nasty big loop which treats all plugins pretty equally and does some iterations over aliases and source reinstantiation | 16:47 |
| tristan | B.) The plugins each have to do something on their own | 16:47 |
| tristan | With A, plugins remain more dumb, and the core can assert that the mirrors have been tried (it's executed a plugin code path for each mirror) | 16:48 |
| jonathanmaw | tristan: ok, I'll tidy up my approach A branch and make sure it's in a MR | 16:51 |
| tristan | jonathanmaw, alright, I think A has got some clear advantages over B yes | 16:52 |
| tristan | Also if there is policy and control about what order aliases get traversed | 16:53 |
| tristan | then the core can also ensure it, without saying that "plugins might honor the order" | 16:53 |
| gitlab-br-bot | buildstream: merge request (328-support-for-downloading-sources-from-mirrors->master: Resolve "Support for downloading sources from mirrors") #404 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/404 | 16:59 |
| jonathanmaw | tristan: ^ should be the MR in a state ready to review. the only changes I made after valentind looked it over was to add documentation and NEWS entries | 17:04 |
| tristan | jonathanmaw, nice I'll take a look at this today :) | 17:05 |
| jonathanmaw | tristan: thanks | 17:05 |
| tristan | let's see how horrible the hacks are and if we can improve them hehe :) | 17:06 |
| *** Prince781 has joined #buildstream | 17:06 | |
| jonathanmaw | tristan: yep, part of it involves generating unique combinations of alias overrides for every alias that the source suspects, which had an extremely complicated algorithm until I actually understood what it did. | 17:07 |
| jonathanmaw | s/suspects/expects/ | 17:09 |
| jonathanmaw | the algorithm's now only slightly magical | 17:09 |
| *** bochecha has joined #buildstream | 17:10 | |
| *** tristan has quit IRC | 17:13 | |
| *** jonathanmaw has quit IRC | 17:13 | |
| *** dominic has quit IRC | 17:15 | |
| *** bethw has quit IRC | 17:22 | |
| *** Prince781 has quit IRC | 18:45 | |
| jjardon | maybe of intesrest here: https://www.collabora.com/news-and-blog/blog/2018/06/27/introducing-debos/ | 19:09 |
| *** Prince781 has joined #buildstream | 19:23 | |
| *** bethw has joined #buildstream | 19:48 | |
| *** Prince781 has quit IRC | 19:59 | |
| *** Prince781 has joined #buildstream | 20:16 | |
| *** Prince781 has quit IRC | 21:12 | |
| *** bethw has quit IRC | 21:26 | |
| gitlab-br-bot | buildstream: merge request (valentindavid/331_include->master: WIP: Add support for include in project.conf) #471 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/471 | 21:46 |
| *** bochecha has quit IRC | 21:52 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!