| *** mcatanzaro has quit IRC | 05:13 | |
| *** Prince781 has joined #buildstream | 05:37 | |
| *** tristan has joined #buildstream | 06:40 | |
| *** Prince781 has quit IRC | 07:12 | |
| *** Prince781 has joined #buildstream | 07:25 | |
| *** noisecell has joined #buildstream | 08:05 | |
| gitlab-br-bot | buildstream: issue #283 ("New sequence ID in messages cannot work") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/283 | 08:06 |
|---|---|---|
| *** Prince781 has quit IRC | 08:32 | |
| gitlab-br-bot | buildstream: merge request (sam/document-profiling-and-benchmarks->master: HACKING.rst: Mention benchmarking and profiling tools) #267 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/267 | 09:08 |
| gitlab-br-bot | buildstream: merge request (sam/document-profiling-and-benchmarks->master: HACKING.rst: Mention benchmarking and profiling tools) #267 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/267 | 09:15 |
| *** sstriker has quit IRC | 09:16 | |
| gitlab-br-bot | buildstream: issue #206 ("HACKING.rst: Document recommended strategies for profiling") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/206 | 09:16 |
| *** valentind has joined #buildstream | 09:23 | |
| tristan | juergbi, well, HACKING.rst is already merged; but it's pretty annoying that it documents useful external profiling stuff and completely ignores the profiling helpers we have :-/ | 09:30 |
| juergbi | tristan: didn't you mention once that you don't want to document bst env. variables at all, not even in HACKING.rst? | 09:31 |
| tristan | I mentioned that env vars are internal, I dont recall saying that I dont want it in HACKING.rst, that sounds like the right internal place | 09:32 |
| tristan | I may have, I really dont recall | 09:32 |
| juergbi | maybe I misremember | 09:32 |
| tristan | Either we should make it a thing, or we should remove _profile.py entirely | 09:32 |
| tristan | but a unilateral decision should be made and the codebase should be in sync with itself | 09:33 |
| juergbi | i haven't been involved in bst profiling yet, so i don't have an opinion on it | 09:33 |
| juergbi | ssssam[m] wrote it and jmac reviewed it, so i assume it matches what they actually used | 09:34 |
| tristan | It is *much* more important that we dont have double standards and multiple stories for a "thing", than the actual decision of what the "thing" is. | 09:34 |
| tristan | that goes for pretty much any "thing" | 09:34 |
| juergbi | certainly less critical for internal things, though | 09:34 |
| tristan | One area where we are dirty now, is how we denote privateness, partly it's my fault for leaving a mess behind after privatizing some things pre 1.0 release (Context and Project specifically) | 09:35 |
| juergbi | e.g., if both approaches have their use cases but we want to recommend only one way (as that's what should cover most use cases), then it could make sense to only/mainly document that one way but keep support for both | 09:36 |
| tristan | I tried cProfile by itself without internal hooks, but that makes it impossible to only observe, say; the time it takes to sniff out cyclic dependencies in the loaded project | 09:37 |
| *** noisecell has quit IRC | 09:38 | |
| tristan | juergbi, you have a point, *if* they are both useful | 09:38 |
| juergbi | yes, as i said, i can't speak from experience with regards to bst profiling, so i don't have anything to add here | 09:39 |
| tristan | But I strongly disagree about importance of internalness being much lower when it comes to consistency and dual standard | 09:39 |
| juergbi | it's better to have something documented for profiling than nothing at all | 09:40 |
| tristan | I.e., I would like to use some of the features in python to make stronger API surfaceses, eliminate possibility of monkey patching etc | 09:40 |
| juergbi | if this is not the recommended approach after all, we'll update/amend it | 09:40 |
| tristan | But to be clear about policy: I would *block* a patch which adds this stuff without being applied unilaterally across the codebase | 09:40 |
| juergbi | sure, that makes sense. that's a different case | 09:41 |
| tristan | juergbi, just want to make sure we're on the same page for that part :) | 09:41 |
| juergbi | yes, i definitely want internal consistency in such aspects as well | 09:42 |
| juergbi | but tooling recommendation is a bit different, imo | 09:42 |
| juergbi | before the MR we didn't have anything documented | 09:42 |
| juergbi | and i think cProfile was very useful for ssssam[m] for the realpath issue, e.g. | 09:43 |
| juergbi | so i think it's good to have that in HACKING.rst | 09:43 |
| tristan | Frankly, we use cProfile in _profile.py | 09:43 |
| tristan | And, I dont see other C projects carrying documentation baggage informing developers that they can run valgrind | 09:44 |
| tristan | I sort of do feel like, this is something about python, not BuildStream | 09:44 |
| tristan | In this case I dont care so much though; it's more of a nitpick I guess; it's not costing much other than making the HACKING.rst bigger | 09:45 |
| juergbi | we also have instruction to run ./setup.py test even though that's a normal setuptools test setup | 09:46 |
| tristan | It would be though, IMO, more appropriate to document BST_PROFILE and recommend improving on that, with a very short mention or link to external cProfile documentation | 09:46 |
| juergbi | i certainly agree that BST_PROFILE should be documented there | 09:46 |
| juergbi | and if external cProfile documentation covers all aspects of what we have in HACKING.rst, sure, just link to it | 09:47 |
| juergbi | however, if there is no suitable link that really covers the topic in a comparable way, better to keep it | 09:47 |
| juergbi | btw: it does link to the python.org documentation | 09:48 |
| juergbi | the link to the buildstream benchmark repo is useful | 09:48 |
| juergbi | and python.org doesn't mention pyflame or flamegraph.pl | 09:49 |
| tristan | Yeah I dont object to keeping it there if it's useful; myself conversely: I was annoyed with using an external tool *only*, without internal code support and MARK() benchmarking style; it felt useless | 09:49 |
| tristan | Sam must have had the opposite perspective | 09:49 |
| juergbi | tristan: btw: jmac added BST_PROFILE documentation to HACKING.rst in https://gitlab.com/BuildStream/buildstream/commit/7babcb0c76ed99e26a3a9b87056f222906c69e18 | 09:58 |
| juergbi | and you told him to remove it | 09:58 |
| juergbi | you can't expect us to guess that you've changed your mind ;) | 09:59 |
| tristan | juergbi, I recall making a very strong statement that ms timing should not be exposed as an env var, as that is API | 09:59 |
| tristan | where is my comment ? | 10:00 |
| juergbi | https://gitlab.com/BuildStream/buildstream/merge_requests/275#note_59259972 | 10:00 |
| juergbi | this was in response to the above commit | 10:00 |
| tristan | juergbi, It can be that I was distracted and mixing topics, though... I gladly take blame for that... | 10:00 |
| * tristan reads | 10:00 | |
| tristan | "Can we turn this into a CLI option, and avoid documenting BST_PROFILE internal hidden debuging env var that is not guaranteed to be API stable in any way ?" | 10:01 |
| tristan | juergbi, Ok well, misunderstanding and mix of topics happened there | 10:01 |
| tristan | HACKING.rst is not API, it's core-developer docs | 10:01 |
| juergbi | ok, that makes sense | 10:02 |
| tristan | microsecond timing needs to be configurable by the user, and external benchmarking harnesses | 10:02 |
| juergbi | maybe you didn't check which file jmac added the documentation to | 10:02 |
| juergbi | so we should probably resurrect that documentation bit | 10:02 |
| tristan | Yes, hey... I'm not angry man ! | 10:02 |
| tristan | haha | 10:02 |
| tristan | But yes I will be happy to A.) Take blame, I hoard it... B.) apply the BST_PROFILE part of his patch | 10:03 |
| tristan | the MS timing since then has been removed and is now proper API | 10:03 |
| juergbi | (i don't care about blame, just wanted to clarify what we should and should not document where) | 10:03 |
| tristan | :) | 10:03 |
| tristan | Yes, so core-internal things in HACKING.rst is perfect; it is my mistake to have made that comment in that way, though | 10:05 |
| tristan | I frankly did not read the patch *at all*, I saw the title; and I knew a correction had to be made | 10:05 |
| juergbi | no problem. that makes sense | 10:06 |
| * tristan is having a hard time reproducing the source interrogation messages which are appearing in, say: https://gitlab.gnome.org/GNOME/gnome-build-meta/-/jobs/12216/raw | 10:07 | |
| tristan | (if one searches, cat-file, for instance) | 10:07 |
| tristan | I have a clean patch which A.) Adds Context._silent_messages() ... this does the same as a Context._timed_activity(... silent_nested=True ...) only without printing a message | 10:08 |
| tristan | And B.) Uses `with context._silent_messages()` to call `Source.get_consistency()` in source.py:_update_state() | 10:09 |
| tristan | juergbi, I think I'm just going to push it; it's not worth spending time trying to reproduce, and only started doing it because it's damn annoyingly noisy in the logs and thought hey "this'll only take me 10 minutes" | 10:10 |
| * tristan knows it's there in master too, just dont know what the special sauce is to reproduce | 10:11 | |
| juergbi | track during build? | 10:11 |
| tristan | yes | 10:11 |
| juergbi | or even just fetch during build | 10:11 |
| tristan | I've been doing that | 10:11 |
| tristan | I've been deleting local repos, `build --track-all`, deleting local artifacts selectively | 10:12 |
| juergbi | hm, odd | 10:12 |
| juergbi | i'm fine with just pushing it | 10:12 |
| tristan | while playing in _context.py, I spotted https://gitlab.com/BuildStream/buildstream/blob/master/buildstream/_context.py#L33 | 10:13 |
| tristan | heh | 10:13 |
| juergbi | yes, i should have caught the sequence id issue | 10:15 |
| jmac | Oops! | 10:15 |
| juergbi | i did notice the lock, however, i was expecting it to be merely unnecessary. i didn't realize that it relies on shared memory children | 10:16 |
| juergbi | i.e., i assumed sequence_ids are only created in the main process | 10:16 |
| tristan | nod, yeah timed activities actually *mostly* only happen in the child process, outer activities starting directly in job.py | 10:19 |
| tristan | Some happen in the main process (total session time) | 10:19 |
| juergbi | btw: the fork() model is again causing some issues with grpc | 10:20 |
| juergbi | it's not ideal that we use it | 10:20 |
| juergbi | complicates some things | 10:20 |
| juergbi | breaks libraries | 10:20 |
| juergbi | however, it's obviously more convenient in other aspects | 10:20 |
| tristan | It's a difficult tradeoff | 10:21 |
| tristan | Right now, the worst a third party plugin can do is crash | 10:21 |
| tristan | and buildstream can report an error nicely for that | 10:21 |
| tristan | (maybe worse since we still run some plugin things in the main process) | 10:21 |
| juergbi | with python i'd hope the issue of in-process plugins wouldn't be that big | 10:22 |
| juergbi | (using native libraries anything can happen, of course) | 10:22 |
| tristan | Well, not only that; but with a threading model; the worst a badly written plugin can do, is lockup the whole build | 10:23 |
| juergbi | i'd probably favor putting all main logic in the main process (event loop), only offloading computationally expensive operations to threads or (fork+exec) subprocesses | 10:23 |
| jmac | I can fix sequence_ids, that should be easy | 10:23 |
| *** dominic has joined #buildstream | 10:23 | |
| jmac | But if _timed_activity is being created by different processes won't _pop/_push_message_depth break? Or is there a context per process? | 10:24 |
| *** noisecell has joined #buildstream | 10:24 | |
| tristan | jmac, copy-on-write; so the main Context is copied, and at child task time the message depth never contains a silence marker | 10:24 |
| tristan | jmac, it's basically process local state | 10:25 |
| jmac | right, in that case sequence_id just needs to be an object member rather than a class variable | 10:26 |
| tristan | juergbi, I feel that we may want to schedule everything a plugin does, we can significantly improve startup time with parallelization of `Source.get_consistency()` | 10:27 |
| *** zalupik has quit IRC | 10:27 | |
| tristan | so that... might happen | 10:27 |
| juergbi | it's a big change, we have to be very careful | 10:27 |
| *** zalupik has joined #buildstream | 10:27 | |
| juergbi | but it would be nice to move towards that | 10:27 |
| tristan | juergbi, frankly, I would not be opposed to dropping fork()/exec() in favor of threading, it would also be a big change but I think it's an API compatible one | 10:28 |
| juergbi | we have to think about thread-safety of all API-exposed structures | 10:29 |
| tristan | Still, not sure it's worth it; there are advantages and disadvantages both ways | 10:29 |
| juergbi | and internal ones for our code | 10:29 |
| tristan | Right now, buildstream core devs have to know about this stuff, and we have to care about not using third party stuff which initializes threads in the main processes | 10:29 |
| tristan | so it's nice that the onus is on us and less on plugin authors | 10:29 |
| juergbi | it's actually not so simple | 10:29 |
| juergbi | import ostree alone creates a thread... | 10:30 |
| juergbi | which happens in the main process | 10:30 |
| tristan | holy shit | 10:30 |
| juergbi | it's actually not ostree's fault. it's glib/gi | 10:30 |
| tristan | sigh | 10:31 |
| juergbi | or maybe it was triggered by ostree source plugin consistency check, have to recheck | 10:32 |
| juergbi | however, it was definitely happening in the main process | 10:33 |
| *** aday has joined #buildstream | 10:39 | |
| *** valentind has quit IRC | 10:56 | |
| gitlab-br-bot | buildstream: merge request (issue-243_dpkg_import_source_plugin->master: WIP: Created DPKG Source plugin for Issue #10) #305 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/305 | 11:01 |
| *** milloni has joined #buildstream | 11:16 | |
| nexus | Hi guys, i'm looking into https://gitlab.com/BuildStream/buildstream/issues/89 and after discussing this issue with tlater, the question was raised about whether or not an element specific subdir should be created if `build-root` is explicitly defined. Any thoughts on this matter? | 11:16 |
| tristan | nexus, Nobody sets that. I would first change the default behavior and document the concerns as a separate issue | 11:21 |
| tristan | the question arises as to whether we need strict policy around these variables, that is true | 11:22 |
| tristan | but I dont think that question has to block changing the defaults; probably this will never pose any problem anyway. | 11:22 |
| juergbi | i actually do set it in an element here but it's a very special case | 11:24 |
| tristan | Heh | 11:24 |
| juergbi | i set it to a subdir of %{install-root} as i want to convert a source in an artifact | 11:24 |
| juergbi | gcc: so that i can use it in multiple elements | 11:25 |
| tristan | interesting | 11:25 |
| juergbi | should likely be handled differently. maybe includes | 11:25 |
| tristan | juergbi, maybe we could look at that as... possibly a workaround for not having cached build trees ? | 11:26 |
| juergbi | (i also noticed that it's actually faster to stage an artifact than to extract the tarball every time but that's a separate issue) | 11:26 |
| tristan | Oh ! | 11:26 |
| tristan | juergbi, you are bad. | 11:26 |
| juergbi | hm, i don't see how cached build trees will help sharing sources across elements / element variants | 11:26 |
| tristan | haha | 11:26 |
| tristan | I'm thinking, you want to reuse an already built gcc build tree; I'm not completely familiar with why you are doing though | 11:27 |
| tristan | so you know, assumptions... | 11:27 |
| juergbi | ah, no, i have a gcc-source.bst | 11:27 |
| juergbi | that doesn't have any commands | 11:27 |
| juergbi | variables: | 11:28 |
| juergbi | build-root: '%{install-root}/src/gcc' | 11:28 |
| juergbi | and then i use that as dependency in the real gcc elements | 11:28 |
| tristan | Ah | 11:28 |
| tristan | it's entirely read-only, this is a very special case | 11:28 |
| juergbi | this avoids duplicating url/ref and patches | 11:28 |
| tristan | you are not as bad as I suspected, juergbi :D | 11:28 |
| juergbi | yes, should definitely not worry upstream about it | 11:28 |
| juergbi | hehe | 11:28 |
| tristan | staging an artifact will always be faster than staging a source | 11:29 |
| juergbi | i need to work around missing features until i can solve it properly | 11:29 |
| tristan | even if tar sources do a pre-extraction to speed up staging | 11:29 |
| juergbi | as gcc is built 5 times or so for full bootstrapping, it actually helps a lot | 11:29 |
| tristan | at the cost of disk space | 11:29 |
| tristan | You cannot hard-link sources into the read-write build tree :) | 11:29 |
| juergbi | indeed but i assume extracting is the main bottleneck, not copying | 11:30 |
| juergbi | single-threaded xz. other cores twiddling thumbs | 11:30 |
| tristan | That could be an optimization; if you want to prefer CPU over disk space | 11:30 |
| juergbi | if we start caching build trees, we should actually also cache sources in the artifact cache | 11:30 |
| juergbi | as build trees are normally anyway superset of those | 11:31 |
| juergbi | so it would be free | 11:31 |
| tristan | yeah that is a more complex endeavor | 11:31 |
| juergbi | yes, just a point on a long list of possible optimizations ;) | 11:31 |
| tristan | it's something that is going to happen though :) | 11:32 |
| tristan | few things depend on it, so... for the crazy incremental build proposals, we'll have to take care to break hardlinks | 11:32 |
| juergbi | yes, for mtime | 11:32 |
| juergbi | (btw: i did a minimal resurrection of include support last night and i like it) | 11:33 |
| tristan | also to avoid corrupting artifacts | 11:33 |
| tristan | juergbi, nice :) | 11:33 |
| tristan | I threw it away mostly because it wasnt being used | 11:33 |
| juergbi | i can prepare a MR at some point but not sure whether we should have a discussion about this beforehand | 11:34 |
| juergbi | i can also first gain some experience with it and open a MR later | 11:34 |
| *** dominic has quit IRC | 11:35 | |
| *** dominic has joined #buildstream | 11:35 | |
| juergbi | i noticed that the simple code-less build element plugins are almost redundant with include support | 11:35 |
| *** ernestask has joined #buildstream | 11:35 | |
| *** xjuan has joined #buildstream | 11:39 | |
| tristan | meh, I dont really mind the boilerplate | 11:40 |
| gitlab-br-bot | buildstream: merge request (jmac/restore-brackets-282->master: status.py: Restore brackets to time in job display area) #308 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/308 | 11:52 |
| gitlab-br-bot | buildstream: merge request (jmac/restore-brackets-282->master: WIP: status.py: Restore brackets to time in job display area) #308 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/308 | 11:57 |
| gitlab-br-bot | buildstream: issue #280 ("Messages not silenced when interrogating source caches") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/280 | 11:58 |
| gitlab-br-bot | buildstream: merge request (jmac/restore-brackets-282->master: WIP: status.py: Restore brackets to time in job display area) #308 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/308 | 12:02 |
| tlater | juergbi: About the pylint defaults, it turns out that pylint follows entirely different defaults than the ones it recommends when you use `--generate-rcfile` | 12:09 |
| tlater | The usual defaults are also not always compatible with pep8 | 12:09 |
| tlater | Which is a major pain :| | 12:09 |
| * tlater thinks there is no real way around having default options set in the rcfile | 12:10 | |
| * tristan just lost like 30 minutes because he had defined a new function with the same name at same (class) scope | 12:11 | |
| tristan | would have been great if a linter told me about it :) | 12:11 |
| * tlater did some js coding last weekend, without a linter for once | 12:12 | |
| juergbi | tlater: too bad. might it still make sense to clearly mark what we changed? | 12:12 |
| juergbi | tlater: also, i assume running --generate-rcfile as part of the linting process is not sensible? | 12:13 |
| tlater | juergbi: Yeah, that would make sense, just in case we want to update at some point | 12:13 |
| tlater | We could try that, it would be quite hacky | 12:13 |
| tlater | Hmm | 12:13 |
| juergbi | (and appending our changes) | 12:13 |
| tlater | jennis: Think you'd like to try ^^^ | 12:13 |
| tlater | It would make sense | 12:13 |
| jennis | hang on | 12:13 |
| * tlater just hopes appending overwrites | 12:13 | |
| juergbi | indeed | 12:14 |
| jennis | you want to see what we've changed in our configuration file compared to the one generated by `--generate-rcfile`? | 12:14 |
| tlater | juergbi: The only problem with this approach is that this only really works in CI | 12:14 |
| tlater | What would really annoy me is that this appending approach certainly wouldn't work with flycheck, so emacs wouldn't lint for me anymore :| | 12:15 |
| juergbi | we could have a script also for non-CI usage | 12:15 |
| juergbi | maybe setup.py build could generate it? | 12:15 |
| tlater | The problem is IDEs that integrate with pylint, not CLI usage | 12:15 |
| juergbi | in which case IDEs would also pick it up | 12:15 |
| juergbi | i don't want to complicate things too much, of course. i just don't like adding so many lines to the repo where most are not buildstream-specific | 12:15 |
| gitlab-br-bot | buildstream: merge request (jmac/restore-brackets-282->master: status.py: Restore brackets to time in job display area) #308 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/308 | 12:15 |
| tlater | Yeah, I do understand. It's a bit unfortunate that pylint does things this way | 12:16 |
| juergbi | or would it be possible to split it into two files? | 12:16 |
| juergbi | one completely pristine --generate-rcfile | 12:16 |
| tlater | As long as we document this in Hacking.rst I think generating with setup.py is fine | 12:16 |
| juergbi | and the other one are our overrides? | 12:16 |
| juergbi | completely pristine import is much less problematic | 12:17 |
| juergbi | (and some kind of include, no idea whether that's supported) | 12:17 |
| gitlab-br-bot | buildstream: merge request (239-use-pylint-for-linting->master: Resolve "Use pylint for linting") #283 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/283 | 12:17 |
| tlater | Ah, that might be a point, I'll check the doc | 12:17 |
| tlater | Never tried that | 12:17 |
| juergbi | generation with setup.py would certainly be fine with me if that works | 12:17 |
| tlater | It only uses a specific file unfortunately, no mention of include directives | 12:19 |
| tlater | So generation seems like the only option | 12:19 |
| tristan | jmac, is there a reason you dont develop directly in the upstream repo ? | 12:21 |
| tristan | jmac, just in case you didnt know, you are allowed to push branches directly, it's much less annoying to keep your branches in sync (for you) | 12:22 |
| jmac | No good reason, I just started developing before I got permissions on the main repo | 12:23 |
| tristan | well, up to you :) | 12:24 |
| jmac | I'll push future branches straight to BuildStream | 12:24 |
| jmac | Looks like I could replace that lock with multiprocessing.lock but it's not clear from them whether that lock works with processes created outside multiprocessing | 12:25 |
| tristan | eek | 12:26 |
| tlater | That sounds pretty ugly | 12:26 |
| * tristan not very trustful of python code trying to magically do stuff like that | 12:26 | |
| jmac | Me neither really | 12:27 |
| tlater | Oof | 12:27 |
| tlater | juergbi: Another issue: pylint apparently takes system defaults | 12:27 |
| tristan | jmac, you might go with a dual token | 12:27 |
| tlater | So if we use --generate-rcfile, we cannot guarantee that someone using our setup.py will get the same config file | 12:27 |
| tlater | Because they might have a config in ~/.pylintrc | 12:28 |
| tristan | jmac, i.e. pass a Job sequence to the Job, and inside the Job there are activity sequences | 12:28 |
| * tlater apparently had one when generating | 12:28 | |
| juergbi | ok, that actually explains why --generate-rcfile produces something different than the built-in defaults | 12:28 |
| jmac | I'm thinking of replacing it with a random number or something based on pid+sequence, would be a lot simpler | 12:28 |
| juergbi | tlater: as the former uses the system (or user) defaults | 12:28 |
| tristan | Could we trust pid ? | 12:29 |
| juergbi | pid could be reused | 12:29 |
| tlater | Unfortunately pylint *also* uses system/user defaults by default | 12:29 |
| jmac | Yes it can | 12:29 |
| tristan | jmac, explicit Job number is better than pid indeed | 12:29 |
| jmac | Same problem with random numbers | 12:29 |
| tristan | <tristan> jmac, i.e. pass a Job sequence to the Job, and inside the Job there are activity sequences | 12:30 |
| juergbi | tlater: in that case i think you should first check where exactly the defaults came from. are these really upstream defaults or some distro-specific thing | 12:31 |
| tlater | Yeah, I should definitely take a closer look | 12:31 |
| juergbi | and then we could do a straight import of these defaults directly from the source (with appropriate comment) and use setup.py to combine it with our overrides | 12:32 |
| juergbi | if that makes sense to you | 12:32 |
| juergbi | i was hoping it wouldn't get too complicated but we need to avoid including random stuff | 12:32 |
| tlater | Yep, definitely makes sense | 12:32 |
| tlater | Should probably not be too much of a hassle to use either | 12:33 |
| gitlab-br-bot | buildstream: merge request (jmac/restore-brackets-282->master: status.py: Restore brackets to time in job display area) #308 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/308 | 12:36 |
| tristan | jmac, another thought | 12:41 |
| tristan | eh, not sure it's entirely relevant to what you're doing | 12:42 |
| tristan | jmac, just a thought; is that there is a desire to collate build logs stored in artifacts with the build session logs which caused those builds | 12:43 |
| tristan | although there is currently nothing about a "session" that is stored in an artifact | 12:43 |
| tristan | or rather, there is nothing persisted and controlled by buildstream for this connecting of the dots after the fact | 12:44 |
| tristan | may be related to these job sequences (it may be interesting to know that "This was job number 42 in session 582953" | 12:44 |
| tristan | ) | 12:44 |
| *** jennis has quit IRC | 12:52 | |
| tlater | So, if I understand our current incremental workspace builds correctly, then we bind-mount any workspaces into the build environment | 12:53 |
| tlater | Oh, this only relates to dependencies of workspaced elements, of course | 12:54 |
| tristan | you mean the naming of %{build-root} ? | 12:56 |
| tlater | tristan: I was starting to ask something about our incremental builds issue | 12:57 |
| tlater | But I realized that I just didn't understand the problem properly | 12:58 |
| * tlater reads the issue again | 12:58 | |
| tristan | I dont think naming of %{build-root} is important for workspace incremental builds | 12:58 |
| tlater | No, it's completely separate from that :) It's about this issue: https://gitlab.com/BuildStream/buildstream/issues/215 | 12:59 |
| tlater | Somehow I distilled that into "we need to keep track of changes in the workspace" | 12:59 |
| tlater | But that's of course entirely wrong | 13:00 |
| tristan | ah right | 13:01 |
| *** jennis has joined #buildstream | 13:06 | |
| *** sstriker has joined #buildstream | 13:08 | |
| *** tristan has quit IRC | 13:09 | |
| *** tristan has joined #buildstream | 13:15 | |
| *** nexus is now known as Nexus | 13:41 | |
| jmac | tristan: The idea was simply to tie together log lines relating to a particular job, since there didn't appear to be any other unique identifier that would tie the start/finish times with other log messages in the same jbo | 13:42 |
| jmac | job | 13:42 |
| jmac | It may be possible to do this by always printing the log file name | 13:43 |
| *** Nexus has quit IRC | 13:55 | |
| *** nexus has joined #buildstream | 13:56 | |
| adds68 | juergbi, we have options defined in the project.conf, but i'm not sure how to define how an element should only be build if the architecture is only x86? | 14:04 |
| adds68 | jmac, specifically this element here: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/blob/adamjones/vaapi-intel/sdk/elements/all.bst#L18 | 14:04 |
| juergbi | adds68: have you seen the target_arch (?) conditional in project.conf? | 14:05 |
| jmac | Was that meant to be for juergbi? | 14:05 |
| juergbi | i assume so | 14:05 |
| adds68 | sorry jmac! :O | 14:06 |
| jmac | Just checking :) | 14:06 |
| juergbi | it may be a bit inconvenient with lists, though, let me check | 14:06 |
| adds68 | juergbi, so do i simply define that under variables and buildstream will only build if the arch is correct? | 14:07 |
| adds68 | juergbi, could i put that inside the element itself? | 14:07 |
| juergbi | you'll have to put it into the all.bst stack | 14:07 |
| juergbi | as you want to influence `depends` | 14:07 |
| juergbi | the issue is that the conditional only directly work with dicts, iirc | 14:09 |
| juergbi | while depends: is a list | 14:09 |
| *** nexus has quit IRC | 14:09 | |
| *** nexus has joined #buildstream | 14:10 | |
| *** jennis has quit IRC | 14:11 | |
| *** jennis has joined #buildstream | 14:11 | |
| *** nexus has quit IRC | 14:11 | |
| *** nexus has joined #buildstream | 14:11 | |
| juergbi | adds68: something like this should work: https://pastebin.com/dMKZ0u9S | 14:11 |
| juergbi | however, it duplicates the common elements | 14:11 |
| juergbi | not sure whether we can use list composition in a single element file | 14:12 |
| adds68 | juergbi, ah ok, i understand now | 14:12 |
| adds68 | Hm that isn't pretty is it | 14:12 |
| juergbi | adds68: you could give this a try: https://pastebin.com/39hMzZtt | 14:12 |
| juergbi | not sure whether this works in a single file, though | 14:13 |
| juergbi | i think there is an issue in yaml with duplicate dict entries | 14:13 |
| adds68 | juergbi, it's a shame you can't define an elements architecture | 14:14 |
| juergbi | well, it would be odd to have an unconditional dependency on an element that may not exist, imo | 14:14 |
| juergbi | you could put a conditional in the element itself and just have 'kind: manual' without sources or commands for the non-x86 case | 14:15 |
| *** nexus has quit IRC | 14:16 | |
| *** Nexus has joined #buildstream | 14:16 | |
| juergbi | which would result in an empty artifact. but i think this would be a bit odd | 14:16 |
| *** Nexus has quit IRC | 14:16 | |
| *** Nexus has joined #buildstream | 14:16 | |
| tlater | juergbi: So, I misunderstood this issue a bit originally, I think: https://gitlab.com/BuildStream/buildstream/issues/215 | 14:17 |
| tlater | Are you suggesting we check the differences between old and new dependency artifacts, based on whether they were built before or after the latest successful build of a workspaced element? | 14:19 |
| tlater | Ew, that's a mouthful | 14:19 |
| juergbi | i'm not suggesting anything time-based (if you mean time-based with before/after) | 14:21 |
| tlater | I do mean that. So, if I understand the problem correctly... | 14:22 |
| tlater | If I have an A that depends on B | 14:22 |
| juergbi | it's also possible to roll back dependencies to older versions, which may already be in the cache | 14:22 |
| juergbi | that wouldn't matter | 14:22 |
| tlater | Right, I think I'm misunderstanding the problem. | 14:23 |
| juergbi | let's first talk about the simple case, no failed builds | 14:23 |
| juergbi | is that already clear or not? | 14:23 |
| tristan | jmac, you will want finer grained grouping then just "this job", for benchmarking at least | 14:23 |
| tlater | I thought it was, starting to doubt it x) | 14:24 |
| tlater | Mind trying to repeat that case? | 14:24 |
| tristan | jmac, still, handing out job ids at dispatch time is not a bad idea; my previous comments was just that; this might be useful for other things as well | 14:25 |
| juergbi | ok, for an incremental build after a successful build, you need to calculate the difference between all staged artifacts of the previous successful build and the to-be-staged artifacts of the planned incremental builds | 14:25 |
| juergbi | all files that differ need to get updated mtime (and thus also can't be hardlinks) | 14:25 |
| jmac | tristan: Yep. I'm having a think about it, but will probably end up doing that | 14:26 |
| tlater | Ah, this was where I got confused with time-based things | 14:26 |
| juergbi | yes, so the updated mtime should be the current time (i.e., start time of incremental build) | 14:26 |
| juergbi | we don't care about when the dependencies were built | 14:26 |
| tlater | Well, we need to know which artifacts we staged during the previous build | 14:27 |
| juergbi | we need this because make and other traditional build systems unfortunately use time-based dependency checks, not checksum-based | 14:27 |
| juergbi | yes, that we can read out from the previous build artifact metadata | 14:27 |
| tlater | Oh, that's already in there? | 14:27 |
| * tlater wasn't aware | 14:27 | |
| juergbi | i think so | 14:28 |
| juergbi | there is a dependencies list in the metadata | 14:28 |
| tlater | Well, that makes a whole lot more sense now. | 14:28 |
| tlater | Now, assuming we have a few failed builds in-between, what changes? | 14:28 |
| juergbi | ok, so that's where it gets tricky | 14:29 |
| juergbi | we still need to update the mtime of all modified files since the last successful build as we don't know which parts got rebuilt and which parts didn't | 14:29 |
| juergbi | (modified files in dependencies) | 14:29 |
| juergbi | however, we also need to update the mtime of all files that were different in the dependencies of any of the failed build attempts since the last successful build | 14:30 |
| juergbi | as those parts that got rebuilt during the failed build have to be rebuilt again | 14:30 |
| tlater | Ah, ok, I think I understand | 14:31 |
| juergbi | as we don't have artifacts for failed builds (at least not right now), we don't have corresponding metadata either | 14:31 |
| juergbi | so we have to carry a list ourselves | 14:31 |
| juergbi | (which i'm still not really happy) | 14:31 |
| tlater | Well, at least I think I understand your solution now | 14:32 |
| juergbi | a possible alternative would be to start creating some form of artifact for failed builds as well (we may want that anyway to save and inspect build trees) | 14:32 |
| juergbi | in which case we'd only have to list the refs for the failed builds instead of actual files | 14:32 |
| juergbi | but that would be a broader change | 14:33 |
| tlater | I think for now the quick-and-dirty file lists should be alright | 14:33 |
| tlater | This can be cleaned up when we get to saving failed builds | 14:33 |
| juergbi | yes, i think this is acceptable | 14:34 |
| juergbi | i don't think tristan has expressed an opinion about it yet, though | 14:34 |
| tlater | Well, file lists don't seem like so much work that potentially having to throw them away would matter much... I can wait on that opinion for a bit | 14:37 |
| tlater | tyvm in either case juergbi, sorry if I was a bit slow, been going in and out of this issue over the past weeks... | 14:38 |
| juergbi | no problem. it's not a trivial issue, especially when you consider failed builds | 14:39 |
| *** Nexus has quit IRC | 14:39 | |
| juergbi | i just hope the suggested approach really works ;) | 14:39 |
| *** Nexus has joined #buildstream | 14:39 | |
| *** noisecell has quit IRC | 14:41 | |
| jmac | tristan: Yes, we want the timings for individual actitivites in jobs, but I think we can figure these out without the sequence number, if a couple of assumptions hold true | 14:42 |
| jmac | Depends whether there's any multiprocess/threading inside a job or if they all run in one thread | 14:43 |
| tristan | jmac, sure, in any case we can add features to the logs for benchmark support; making it configurable makes that non-controversial | 14:43 |
| *** jennis has quit IRC | 14:44 | |
| tristan | jmac, we dont do any deep nested processing that *we* can control (i.e. launching make doesnt count, and benchmarking into that also doesnt make sense) | 14:44 |
| tristan | but anyway; pid is not something to use... | 14:45 |
| jmac | Oh, that's fine, an individual activity can run make -j8, that's fine | 14:45 |
| jmac | So I'll just raise an MR to remove sequence ID for now, if we need anything else I'll do that separately | 14:46 |
| tristan | Sure, I'm quite sure that adding a sequence id for jobs, and eventually another for tasks inside jobs (possibly printed as [$job.$task]) makes a while lot of sense for collating times in a benchmarking log interpretation script | 14:47 |
| tristan | s/while/whole | 14:48 |
| tristan | but I'm fine with removing the broken thing first, so as not to let it live there too long | 14:48 |
| tristan | jmac, in *fact*, the approach of adding sequences for jobs, and maybe machine readable names for "kinds of activities" is preferable | 14:50 |
| tristan | In a similar way that we have machine readable strings in our exceptions | 14:51 |
| tristan | Because really, the text that says "Starting to do foo" | 14:51 |
| tristan | Is UI | 14:51 |
| tristan | jmac, it would suck if we had to never change UI... just cause benchmarks | 14:51 |
| *** mcatanzaro has joined #buildstream | 14:52 | |
| *** jennis has joined #buildstream | 14:56 | |
| *** noisecell has joined #buildstream | 14:57 | |
| jmac | I get your point. I only really need to parse the MessageType, though - the message text will be recorded, but no meaning is extracted from it | 14:58 |
| jmac | I'm a bit concerned that having to assign an identifier for all activity messages will restrict other developers | 14:59 |
| jmac | It would be good if we ever want to internationalise, though | 14:59 |
| gitlab-br-bot | buildstream: merge request (jmac/remove-sequence-id->master: WIP: Remove sequence ID.) #309 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/309 | 15:02 |
| gitlab-br-bot | buildstream: merge request (issue-89_unique_build_dirs->master: WIP: Made modification to generate unique subdirs for built elements) #310 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/310 | 15:08 |
| *** noisecell has quit IRC | 15:15 | |
| gitlab-br-bot | buildstream: merge request (issue-89_unique_build_dirs->master: WIP: Made modification to generate unique subdirs for built elements) #310 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/310 | 15:23 |
| gitlab-br-bot | buildstream: merge request (jmac/remove-sequence-id->master: WIP: Remove sequence ID.) #309 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/309 | 15:34 |
| Nexus | juergbi: as i was saying, regarding buildstream | 15:35 |
| Nexus | https://gitlab.com/BuildStream/buildstream/tree/issue-89_unique_build_dirs | 15:35 |
| Nexus | yay pasting ftw | 15:35 |
| Nexus | in what way do you think this would break functionality? | 15:35 |
| juergbi | e.g., plugins using node_subst* would still get the original %{build-root} value | 15:36 |
| juergbi | so it wouldn't match the real %{build-root} | 15:37 |
| Nexus | isn't that just an issue of things using multiple methods for getting the same value? Why are they doing it in different ways? | 15:37 |
| juergbi | node_subst* is used for plugin-specific configuration | 15:38 |
| juergbi | i.e., the values in `config:` | 15:38 |
| Nexus | can't it use get_variable? | 15:38 |
| juergbi | no, get_variable is to get the value of a specific variable | 15:38 |
| juergbi | get_variable() doesn't return config: nodes | 15:39 |
| Nexus | then the only way to change that value would be to change it in every instance | 15:39 |
| juergbi | one option could be to modify __variables | 15:39 |
| juergbi | instead of intercepting the queries | 15:40 |
| *** noisecell has joined #buildstream | 15:41 | |
| Nexus | juergbi: ok so if i intercepted it at the point where __variables is defined I.E. the __init__ of Variables, that would change all instances? | 15:45 |
| juergbi | there is only one instance of Variables per element, so yes, all method that would use it would get the modified value | 15:46 |
| juergbi | it should not be done in the Variables class definition, though. keep it in Element | 15:46 |
| juergbi | as Variables is too low level | 15:46 |
| Nexus | ok | 15:46 |
| Nexus | Although that seems a lot more hacky | 15:47 |
| Nexus | as i'd be having to change a value in a list | 15:48 |
| juergbi | feel free to suggest better options | 15:49 |
| juergbi | one possibility could be to pre-define a variable that would always contain the element name | 15:50 |
| juergbi | e.g. %{element} | 15:50 |
| juergbi | and then we could define build-root as /buildstream/build/%{element} or similar | 15:50 |
| Nexus | well it seems like the "nicest" way would be to change _variables.py, but i don't know what that would effect | 15:51 |
| juergbi | _variables.py doesn't know anything about elements, builds etc. right now. so i don't think that's a good place to change | 15:51 |
| Nexus | i see, so we'd need to introduce elements to it in order to do it there | 15:52 |
| juergbi | i think that approach would only make sense if we anyway want such a variable | 15:53 |
| *** noisecell has quit IRC | 15:53 | |
| Nexus | self.__variables.variables['build-root'] = os.path.join(self.__variables.variables['build-root'], self.normal_name) | 15:53 |
| juergbi | btw: i think you need to modify build-root between extract_variables and the creation of Variables() | 15:53 |
| juergbi | otherwise other variables that use %{build-root} will still get resolved with the old value | 15:54 |
| Nexus | juergbi: https://gitlab.com/BuildStream/buildstream/merge_requests/310/diffs | 16:00 |
| Nexus | does this seem viable to you? | 16:01 |
| juergbi | no, as i mentioned, it needs to happen before the construction of Variables() as that's where variables are resolved | 16:02 |
| Nexus | sorry, that doesn't make sense to me | 16:02 |
| juergbi | why not? | 16:03 |
| juergbi | the Variables() initializer resolves variables in other variables | 16:03 |
| juergbi | build-root already needs to be correct at that point | 16:03 |
| Nexus | you're saying i need to modify the args? | 16:04 |
| juergbi | modify the local 'variables' variable | 16:05 |
| juergbi | before it's passed to Variables() | 16:05 |
| juergbi | alternatively, put the code in __extract_variables() | 16:06 |
| Nexus | but it's a node, how do i modify that? | 16:07 |
| juergbi | you can use it as a dict | 16:09 |
| tlater | juergbi: Regarding the pylintrc, it turns out simply appending our config won't work. We'd need some solution using patch, I think, but I'm not sure including a diff file in the main repo is very useful | 16:11 |
| tlater | Especially considering this wouldn't help us if the defaults change | 16:12 |
| juergbi | tlater: we could theoretically override the options on the pylint command line but i suspect that wouldn't be nice either | 16:13 |
| tlater | Yeah, that would be an absolutely massive invocation | 16:13 |
| *** Prince781 has joined #buildstream | 16:14 | |
| juergbi | i guess all that is left is to clearly document which part of the config file are copied defaults (and mention where the defaults are from) and which part is buildstream-specific | 16:14 |
| tlater | juergbi: Do you think a nice git commit log would be enough to document that? | 16:15 |
| juergbi | i think we should put it in the file | 16:15 |
| juergbi | easier to find and i don't see a reason why not | 16:15 |
| tlater | I'm a bit afraid people would overlook the comments/not keep them up-to-date | 16:15 |
| *** noisecell has joined #buildstream | 16:15 | |
| juergbi | and putting this only in the commit message would improve the situation? | 16:16 |
| tlater | Well, it would show the path from pristine - today | 16:16 |
| juergbi | ah, you mean, first import pristine, then adapt in separate commits? | 16:16 |
| tlater | Yep | 16:16 |
| juergbi | that could indeed be an option | 16:16 |
| juergbi | yes, i think that should be fine. just make sure to mention the source of the pristine import | 16:17 |
| tlater | Yeah, that seems best considering the limitations | 16:18 |
| * tlater finds pylint's configuration interface very lacking, unfortunately | 16:18 | |
| juergbi | pylint obviously doesn't check the quality of configuration interfaces ;) | 16:23 |
| juergbi | assuming pylint devs are using pylint as well | 16:24 |
| gitlab-br-bot | buildstream: merge request (issue-89_unique_build_dirs->master: WIP: Made modification to generate unique subdirs for built elements) #310 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/310 | 16:25 |
| Nexus | juergbi: do you know of a nicer way of doing that? ^ | 16:25 |
| Nexus | i'll remove the whitespace | 16:26 |
| juergbi | can't you just use variables['build-root'] = os.path.join...? | 16:27 |
| Nexus | apparently so, the docs i was using for chainmaps was terrible, it didn't say youcould do that... | 16:29 |
| Nexus | there we go | 16:32 |
| gitlab-br-bot | buildstream: merge request (issue-89_unique_build_dirs->master: WIP: Made modification to generate unique subdirs for built elements) #310 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/310 | 16:32 |
| Nexus | my 3 line commit | 16:32 |
| juergbi | ok, this looks fine to me, assuming it works as expected with variable resolution. however, besides testing, i probably want to run this by tristan | 16:35 |
| Nexus | well it works for generating the directory, i'm not sure about anything else | 16:37 |
| juergbi | CI seems to disagree | 16:40 |
| gitlab-br-bot | buildstream: merge request (239-use-pylint-for-linting->master: Resolve "Use pylint for linting") #283 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/283 | 17:09 |
| *** Prince781 has quit IRC | 17:10 | |
| *** dominic has quit IRC | 18:21 | |
| *** toscalix has joined #buildstream | 18:29 | |
| *** valentind has joined #buildstream | 18:37 | |
| gitlab-br-bot | buildstream: issue #284 ("Source distribution cannot include symlinks") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/284 | 18:43 |
| *** tiagogomes has quit IRC | 18:56 | |
| *** toscalix has quit IRC | 19:59 | |
| *** tristan has quit IRC | 20:40 | |
| *** ernestask has quit IRC | 20:43 | |
| *** mcatanzaro has quit IRC | 21:01 | |
| *** mcatanzaro has joined #buildstream | 21:02 | |
| *** mcatanzaro has quit IRC | 21:06 | |
| *** mcatanzaro has joined #buildstream | 21:06 | |
| *** xjuan has quit IRC | 22:02 | |
| *** valentind has quit IRC | 22:12 | |
| *** aday has quit IRC | 22:47 | |
| *** aday has joined #buildstream | 22:52 | |
| *** aday has quit IRC | 23:43 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!