| *** mohan43u has joined #buildstream | 04:40 | |
| *** tristan has joined #buildstream | 04:48 | |
| *** tristan has quit IRC | 05:01 | |
| *** tristan has joined #buildstream | 05:34 | |
| *** ChanServ sets mode: +o tristan | 05:36 | |
| *** leopi has joined #buildstream | 06:14 | |
| *** brlogger has joined #buildstream | 06:56 | |
| gitlab-br-bot | buildstream: merge request (tristan/fix-cache-exclusivity-1.2->bst-1.2: Tristan/fix cache exclusivity 1.2) #779 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/779 | 06:57 |
|---|---|---|
| *** iker has joined #buildstream | 07:01 | |
| *** iker has quit IRC | 07:02 | |
| *** iker has joined #buildstream | 07:06 | |
| gitlab-br-bot | buildstream: merge request (tristan/fix-cache-exclusivity->master: _scheduler/queues: Mark build and pull queue as requiring shared access to the CACHE) #775 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/775 | 07:16 |
| tristan | juergbi, I would appreciate it if you could look over my latest cleanup job | 07:19 |
| *** leopi has quit IRC | 07:20 | |
| *** leopi has joined #buildstream | 07:21 | |
| tristan | Sheesh, and it's not the end of it :'( | 07:24 |
| tristan | Why are we adding more state to Element ? Element._build_log_path ? (not even local private with double underscore, but has an accessor ?) | 07:25 |
| tristan | This can be returned through Element._assemble(), it's only used in BuildQueue, which calls Element._assemble() | 07:25 |
| tristan | No reason to pollute the element with more state for this... and that's ignoring the "# Bypass queue processing entirely the first time it's tried." ... wtf ??? | 07:26 |
| tristan | That looks like fallout from caching failed builds, bst-1.2 is unaffected by that madness | 07:27 |
| tristan | tlater[m], btw, I was able to preserve the never_delete_dependencies test case by configuring it with "builders: 1" | 07:28 |
| tristan | tlater[m], I think that's a good middle ground for now; accept that we will keep queueing build jobs after assessing that we *might* have to cleanup, until we know that we *must* cleanup | 07:29 |
| tristan | in practice, it hardly makes a difference, the cache size calculation is much quicker than a build | 07:30 |
| tristan | tlater[m], interestingly, ArtifactCache.append_required_artifacts() approach is *also* broken | 07:31 |
| tristan | I will change that to ArtifactCache.mark_required_elements(), and check their keys at cleanup time instead | 07:31 |
| tristan | tlater[m], `bst build --track-all core.bst` for instance... results in deleting required artifacts - because cache keys are not calculated yet | 07:32 |
| tristan | Also, I've found the source of non-determinism of pylint; we need to pin astroid, mccabe, and isort | 07:33 |
| tristan | Those specific deps effect the behavior of pylint, I'm less convinced about isort but the others definitely | 07:34 |
| *** finn has joined #buildstream | 07:47 | |
| *** toscalix has joined #buildstream | 07:49 | |
| *** tiagogomes has quit IRC | 07:53 | |
| gitlab-br-bot | buildstream: merge request (tristan/fix-cache-exclusivity->master: _scheduler/queues: Mark build and pull queue as requiring shared access to the CACHE) #775 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/775 | 07:54 |
| *** tiagogomes has joined #buildstream | 07:56 | |
| iker | skullman, that's the error I was talking about on Friday https://paste.gnome.org/peqxmp83b | 08:03 |
| *** rdale has joined #buildstream | 08:05 | |
| iker | https://paste.gnome.org/ph3xnjkxk | 08:06 |
| tiagogomes | !779 conflicts a lot with !671 | 08:08 |
| tristan | tiagogomes, I expect it would | 08:08 |
| tristan | tiagogomes, assuming 671 were the optimizations you had | 08:08 |
| tiagogomes | 671 it had some cleanups as well | 08:09 |
| tristan | tiagogomes, that said; 779 does not do any further optimizations on what is there | 08:09 |
| tiagogomes | Like having assemble returning the bytes were on my PR | 08:09 |
| tristan | tiagogomes, sorry for duplicating that detail | 08:10 |
| tiagogomes | Or protecting setting the cache on success=True only | 08:10 |
| tristan | I do expect you will have to re-implement what it was you were doing | 08:10 |
| tristan | tiagogomes, it's already done; I'm more concerned that this is happening at all | 08:11 |
| tristan | rather than who gets to be the one who fixes it | 08:11 |
| toscalix | tiagogomes: can you elaborate in the MR about the deduplicate files, so I can modify the paragraph you are referring to | 08:11 |
| tiagogomes | utils._get_dir_size(assembledir) is also wrong due the file deduplication stuff on the CAS | 08:11 |
| tristan | This nonsense of get_artifact_cache().cache_size = foo, should *never* have landed | 08:11 |
| tristan | tiagogomes, that is intentionally that way, I did not change it | 08:12 |
| tristan | tiagogomes, if that is going to change, we'll discuss that | 08:12 |
| tiagogomes | yeah, that's too ugly (modifying the member directly) | 08:12 |
| tristan | It completely breaks modularity, people are making life difficult for themselves by placing bandaid over bandaid, addressing only symptoms | 08:13 |
| tristan | The first thing people should be thinking of is "How does this change make *sense*, how does this buy me clarity ?" | 08:13 |
| tristan | Regressions and bugs we can deal with | 08:13 |
| tristan | A slowly churning spaghetti will send the codebase into a deadend | 08:13 |
| * tristan will writeup a mail on this subject, we need to do better, both in review and while coding | 08:14 | |
| tiagogomes | and it is too early in the morning for spaghetti in UK | 08:14 |
| * tlater[m] wonders what scone code looks like | 08:16 | |
| tristan | kind of like a cross between a cookie and a muffin I guess ? | 08:16 |
| paulsher1ood | mucky? | 08:19 |
| tristan | tiagogomes, one of the more scary things I found was CasCache.commit() doing: self.cache_size = None | 08:20 |
| tiagogomes | yeah, and the tests were failing if you removed that | 08:21 |
| tristan | I know, your branch probably fixed that with proper mutator/accessor too, it's obvious | 08:21 |
| tristan | Of course, because you now had separate 'estimated_size' and 'cache_size', which do subtly different things; and setting it to None force triggers a re-calculation later | 08:21 |
| tristan | And of course, you have to read bits of code in 10 files to have any chance of figuring out what is going on | 08:21 |
| tiagogomes | yup | 08:22 |
| tristan | At which point you wonder, what is intended here ? | 08:22 |
| tristan | Then you realize; nothing is intended | 08:22 |
| tiagogomes | I gave up of trying to reason about the code. Merged 'estimated_size' and 'cache_size' and cleaned the code so that pull and build have identical code paths when changing the cache size | 08:23 |
| tristan | Another thing was elementjob.py and queue.py base class, sharing the 'cache_size' implicitly; removing that breaks tests because the self.cache_size = None, is later captured in the main process | 08:24 |
| tristan | Except when you step back and think about it; it really is *only* the BuildQueue job which needs to report anything back, and all logic happens in the main process anyway | 08:24 |
| tristan | Well, CacheSizeJob also of course | 08:25 |
| tristan | So there are two distinct points, where we anyway have a codepath for reporting these | 08:25 |
| tristan | I'm seeing a regression; the elements in the pipeline shown at session startup are no longer showing the elements with unresolved cache keys | 08:31 |
| tristan | For instance, `bst track --deps all foo.bst`... shows at startup "Pipeline" ... <blank stare> ... "[....] Track START ..." | 08:31 |
| * tristan has cleanup running, and: while sleep 1; do echo `cat ~/.cache/buildstream/artifacts/cache_size` ; done | 09:02 | |
| tristan | ... slooowly going down | 09:02 |
| tiagogomes | insert here <you can not have file deduplication without slow pruning> meme | 09:07 |
| *** abderrahim has quit IRC | 09:11 | |
| *** abderrahim has joined #buildstream | 09:12 | |
| *** phildawson has joined #buildstream | 09:14 | |
| tristan | Well, it wasnt *that* bad this time around | 09:16 |
| tristan | And... no crashes or anything bad | 09:17 |
| tristan | There were multiple cleanup jobs, harmless but annoying | 09:17 |
| tristan | They will be fixed by the FIXME comment I added in the scheduler; it needs to ensure that there is only ever one CacheSizeJob pending at a time | 09:17 |
| tiagogomes | Or just get rid of the cache size job and dynamically update it all the time :) | 09:18 |
| tristan | Ok well, I've tested this now and found the edge cases, I'll fix `bst build --track-all` in a separate MR | 09:18 |
| tristan | tiagogomes, Yeah I'm not holding my breath | 09:19 |
| tristan | tiagogomes, in fact, I'm not entirely sure which is more optimal, if we had ->commit() tell us the size by which the cache actually increased, we're basically trading stat() syscalls from one place to another | 09:20 |
| tristan | then again, since we do them at ->commit time for the estimates anyway, it should be cheaper to have ->commit return the number of actual bytes added | 09:20 |
| tristan | That is also tricky though, because of concurrent commit() | 09:20 |
| tristan | Multiple parallel commits do not *possibly* add the same file multiple times, they are *likely* to | 09:21 |
| tiagogomes | tristan but the syscalls would be over the new files that were added only, and not the full artifact cache, so it scales much better | 09:21 |
| tristan | It would still be imperfect, though; how do you ensure that separate calls to ArtifactCache->commit() do not report added size for the same deduplicated object ? | 09:22 |
| tiagogomes | My MR handles that case | 09:22 |
| tristan | Maybe with the atomicity of renaming the files into place it becomes possible ? but there is still a race | 09:22 |
| gitlab-br-bot | buildstream: merge request (tristan/fix-cache-exclusivity-1.2->bst-1.2: Tristan/fix cache exclusivity 1.2) #779 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/779 | 09:22 |
| gitlab-br-bot | buildstream: merge request (tristan/fix-cache-exclusivity->master: _scheduler/queues: Mark build and pull queue as requiring shared access to the CACHE) #775 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/775 | 09:23 |
| tiagogomes | Basically try the rename, if file exists, don't add up the bytes | 09:23 |
| tristan | Ah atomic rename can tell you if it did replace or not | 09:23 |
| tristan | Right, that makes sense indeed | 09:23 |
| tiagogomes | Hold on a sec | 09:24 |
| tiagogomes | Ah, I remember now | 09:25 |
| tiagogomes | We don't rename to the file destination, but rather link, which gives a FileExistsError if already exists | 09:26 |
| tiagogomes | I think my code is safe | 09:26 |
| tristan | There is no way that linking a file would not be an atomic syscall, however it *does* imply new constraints on the CAS machine's FS | 09:28 |
| tiagogomes | Which constraints? That the FS must support hardlinks? | 09:29 |
| tristan | I.e. I suspect that with the current design, all file attributes are intended to be carried separately to the objects; while the current implementation (at a glance of `ls case/objects` reveals), this is not yet being done | 09:29 |
| tristan | So identical executable and non executable files are stored separately | 09:29 |
| tristan | tiagogomes, Yeah, I sort of expect this to be a portable implementation | 09:30 |
| tristan | at least the store, with file linking, it becomes non-portable; unless I'm missing something else which causes it to already be non-portable | 09:30 |
| tristan | (I know we already link at checkout/stage time, but I think this is a separate detail from the store; and I suspect jmac and juergbi are already thinking about portability with their virtual directory design) | 09:32 |
| *** lachlan has joined #buildstream | 09:34 | |
| *** cs-shadow has joined #buildstream | 09:37 | |
| tiagogomes | The code looks portable, assuming the link does a copy on unsupported file systems | 09:38 |
| gitlab-br-bot | buildstream: merge request (mac_fixes->master: WIP: Resolve "os.sched_getaffinity() not supported on MacOSX Blocks #411") #726 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/726 | 09:42 |
| tiagogomes | But as it stands, it would be less safe is copy was used | 09:43 |
| tristan | tiagogomes, the copy/link is *out* of the artifact cache as far as I know; if there is a possibility of copy from the CAS objects to the extract/ directory, that is a problem, it should go into a tmp directory and then get atomically renamed into place | 09:46 |
| tristan | copy/link into the build directories are already guaranteed to be unique | 09:46 |
| tristan | but copy is *sloooow* | 09:46 |
| tristan | That said, builddir != artifactdir (on separate volumes) in the user config should be well supported, but remains untested; it is anyway *sloow* if used | 09:47 |
| tiagogomes | The CAS does some link operations, which might fallback to copy operations in some filesystems | 09:53 |
| *** alatiera_ has joined #buildstream | 09:54 | |
| tristan | It's unclear to me at this time whether those can be considered safe or not | 09:54 |
| tristan | (when falling back to copy) | 09:54 |
| tiagogomes | They probably aren't, as there could be two processes writing data to the same file simultaneously | 09:55 |
| tristan | right, that would mean that if it's happening at CAS -> extract/ time, it should be happening in CAS -> extract-tmp452355/ with a final os.rename into the intended extract directory (for copy safeness) | 09:56 |
| tristan | Like we do in most places actually | 09:57 |
| tristan | I think we were actually doing that with ostree | 09:57 |
| gitlab-br-bot | buildstream: merge request (juerg/cas-batch->master: WIP: _artifactcache/casserver.py: Implement BatchReadBlobs) #785 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/785 | 09:57 |
| tristan | So I'm surprised if we're not doing that with CAS | 09:57 |
| tiagogomes | Then there's also the issue of multiple bst instances running in parallel. So I don't think we can dodge having to use flock | 09:58 |
| tristan | Yes yes, that is something we've been putting off | 09:59 |
| tiagogomes | The code would have to be reworked a bit, because you can't os.rename a file created with tempfile.NamedTemporaryFile | 09:59 |
| tristan | The temporary directories however should be unaffected by this detail | 09:59 |
| tristan | The cleanup of CAS, not so much | 09:59 |
| tristan | Temp dirs we use in source caches and the like are created with support from the OS which guarantees us atomicity/uniqueness | 09:59 |
| juergbi | tristan: CAS uses os.rename for extract atomicity | 10:01 |
| tristan | tiagogomes, tempfile.NamedTemporaryFile appears to be used only in CAS, actually | 10:01 |
| juergbi | tiagogomes: but os.link can be used | 10:01 |
| tristan | juergbi, Yes I would be surprised if it did not | 10:01 |
| juergbi | ah, need to read scrollback... | 10:01 |
| tiagogomes | juergbi the point is, we can't be sure that os.link does an atomic hard link, and not just a plain copy on some filesystems | 10:02 |
| juergbi | eh, no, that would be incorrect | 10:02 |
| juergbi | POSIX guarantees it | 10:02 |
| juergbi | theoretically, the filesystem could not support hardlinks, but then this would completely fail | 10:02 |
| juergbi | that's not an issue on any supported platform, though, is it? | 10:02 |
| juergbi | (and I don't think we care about FAT filesystems on linux for buildstream cache or anything like that) | 10:02 |
| tristan | juergbi, I think we're moving towards portability and such - and it should be our aim | 10:03 |
| juergbi | Windows has tons of other limitations. atomic rename of open files also doesn't exist, at least not always, etc | 10:04 |
| * tlater[m] wonders if the Windows-Linux thing works with ext4 or fat | 10:04 | |
| tristan | juergbi, even though for *now* we're just doing puny remote execution only on windows, we will have win32 builds right ? | 10:04 |
| tristan | atomic rename has always existed on windows hasn't it ? | 10:04 |
| Kinnison | 99% of systems running WSL will be running NTFS anyway, not FAT | 10:04 |
| juergbi | at some point this would be nice, yes | 10:04 |
| juergbi | tristan: no, I think regular rename still doesn't guarantee that | 10:04 |
| * tristan recalls using atomic rename for a self extracting installer / updater in the 90s, on windows | 10:04 | |
| tristan | Not atomic ? | 10:05 |
| tristan | Just "probably atomic" ? | 10:05 |
| *** lachlan has quit IRC | 10:05 | |
| juergbi | there is no such thing... | 10:05 |
| juergbi | the whole point of atomic is that it avoids race conditions | 10:05 |
| juergbi | https://lwn.net/Articles/682988/ | 10:05 |
| tristan | So *anyway*, dont we at least have atomic rename on NTFS ? | 10:06 |
| Kinnison | One of the safest most atomic things is mkdir() sadly | 10:07 |
| Kinnison | it even works on NFS :-D | 10:07 |
| tristan | I mean, disregarding WSL, which doesnt really interest me at all; I mean for building windows software on windows platforms | 10:07 |
| juergbi | tristan: yes, afaik, NTFS itself supports it | 10:08 |
| juergbi | and it might be exposed in some APIs but I don't know otoh | 10:08 |
| juergbi | but Windows local builds will be very tricky in any case | 10:08 |
| juergbi | (I'm thinking of the bootstrap issue) | 10:08 |
| juergbi | so I try to focus on more realistic things for now | 10:09 |
| tristan | juergbi, I don't expect we'll get bootstrapping done, but I expect we'll be able to build on vendor SDKs and such | 10:09 |
| tristan | Unless we can convince Microsoft to relinquish their C library and compiler, bootstrapping wont happen | 10:09 |
| tristan | But the use case "I have an App, and I want to build it on windows, OSX and Linux, using BuildStream and a unified set of build metadata with some conditional statements", is certainly something to aim for | 10:10 |
| juergbi | yes, it would definitely be useful | 10:11 |
| juergbi | I don't see it as short term goal, though | 10:12 |
| tristan | I don't think it's that dramatically out of reach | 10:13 |
| tristan | The question is only, if we code with the idea in mind that "We are linux only; a port to Windows will *have* to be a big deal that requires a lot of funding/resourcing" then we are much less likely to ever get it done | 10:14 |
| tristan | Sandboxing and everything will be tricky on windows though, that is true on it's own; probably not much more than BSD, though | 10:14 |
| tristan | Pathing (i.e. os.path.join()) is probably our biggest obstacle | 10:15 |
| juergbi | in general I agree that it's better to write portable code from the beginning. however, as long as we know it doesn't work yet, I don't think we should do lots of extra work for unsupported platforms | 10:15 |
| tristan | juergbi, This relates back to the decision of whether to rely on os.link() working or not | 10:16 |
| juergbi | NTFS actually supports hardlinks.. | 10:17 |
| tristan | I mean, at least that's how it came up - does it ? | 10:17 |
| tristan | That is interesting | 10:17 |
| juergbi | and Python 3.2+ os.link() works on Windows | 10:17 |
| *** lachlan has joined #buildstream | 10:21 | |
| tiagogomes | juergbi what does POSIX exactly guarantees? That the the syscall works and the FS driver just doesn't just do a copy? | 10:23 |
| tristan | Wow, more regressions - ^C my build and terminate... the session ends with FAILURE, and shows 4 failures in the build queue | 10:25 |
| tristan | Those were not failures, they were terminated, there is code in app.py to ensure that doesnt happen | 10:25 |
| * tristan thinks he should solicit qinusty to fix these logging regressions | 10:26 | |
| tiagogomes | I've addressed your comments on https://gitlab.com/BuildStream/buildstream/merge_requests/678 btw | 10:30 |
| tristan | tiagogomes, I don't understand test_use_of_protected_var_project_conf | 10:37 |
| tristan | tiagogomes, How is 'sources' and 'elements' in the plugins section a protected variable ? | 10:37 |
| tiagogomes | I don't understand your last question. Sources and elements plugins can declare some specific variables on project.conf. The test is exercises that using a protected variable there is disallowed | 10:40 |
| tristan | tiagogomes, How can source and element plugins introduce protected variables, though ? | 10:41 |
| tiagogomes | name: foo | 10:41 |
| tiagogomes | elements: | 10:41 |
| tiagogomes | cmake: | 10:41 |
| tiagogomes | variables: | 10:41 |
| tiagogomes | max-jobs: 2 | 10:41 |
| tiagogomes | project.conf example ^ | 10:41 |
| tristan | Ah, so you validate it there directly | 10:42 |
| tristan | I see | 10:42 |
| tiagogomes | I validate everywhere where variables are defined | 10:42 |
| tristan | except that in the test case, the protected var is 'sources' or 'elements' | 10:43 |
| tristan | https://gitlab.com/BuildStream/buildstream/merge_requests/678/diffs#36aa50e17b25fc1aabcf9e316de484ba4592aeae_411_426 | 10:43 |
| tristan | tiagogomes, I don't understand that | 10:43 |
| tristan | ohhh no | 10:43 |
| tristan | tiagogomes, I'm misreading the test case, now I get it | 10:43 |
| tristan | tiagogomes, made only one comment, but the docs could also lose the newline between the ~~~~~~~~~~~~~~~ title and the beginning of the section, to be consistent with how we write docs everywhere | 10:51 |
| tristan | tiagogomes, Also, I just filed https://gitlab.gnome.org/GNOME/gnome-build-meta/merge_requests/76 | 10:51 |
| tristan | We need to ensure that freedesktop-sdk is also updated before we introduce this in bst-1.2 | 10:52 |
| tristan | So maybe we can merge master first, and keep track of downstreams before forcing an error on them | 10:52 |
| tristan | I think valentind already updated the llvm6 build to explicitly do -j2 for their purpose rather than hijack max-jobs | 10:53 |
| *** lachlan has quit IRC | 10:55 | |
| tiagogomes | tristan, I prefer not_get_configuration_variables because it is identical to node_get_project_path | 10:59 |
| tristan | tiagogomes, I find node_get_configuration_variables to be pretty loaded, I can't tell what it's doing; looks quite obscure | 11:01 |
| tristan | variables = node_get(node, 'variables') && node_validate_variables(variables), looks very clear an readable | 11:01 |
| juergbi | tiagogomes: POSIX guarantees that it creates a hardlink (or fails). hardlinks have different semantics than copies (content changes affect all links), so fallback to copy would be completely wrong | 11:03 |
| tiagogomes | I don't see a problem of having a more specialized node_get. You always have to have a separate function to validate it | 11:03 |
| tiagogomes | And by having specialized node_get, you can write self.some_member = node_get_some_specialized_method(…) | 11:04 |
| tiagogomes | juergbi ack | 11:04 |
| *** lachlan has joined #buildstream | 11:05 | |
| tristan | tiagogomes, Its okay to have 2 lines of code in a couple places instead of one; what is not okay is having to go all the way to _yaml.py and read the docs comment to understand the purpose of the function | 11:07 |
| tristan | The latter, is at least more important than the former | 11:07 |
| tristan | tiagogomes, the return value of this validation routine, is also only observed in one of the conditions where it is called | 11:07 |
| *** bochecha has joined #buildstream | 11:11 | |
| tristan | tiagogomes, actually, I wonder if we should be checking it in so many places manually at all | 11:13 |
| tristan | tiagogomes, Wouldn't it be better to just have a single check, after having composited the Element.__variables, which says: | 11:14 |
| tristan | If the is a node provenance, then it was set in some configuration, and it's an error, raise the error with the discovered unexpected provenance | 11:14 |
| tiagogomes | You loose provenance if you do that | 11:14 |
| tristan | What do you mean ? | 11:15 |
| tristan | tiagogomes, If you have found a place where you lose the provenance, then *that* is a serious bug on it's own | 11:15 |
| *** lachlan has quit IRC | 11:15 | |
| tiagogomes | If you composite everything and validate, how do you know that the offending variable is in project.conf, the plugin defaults or in some bst file | 11:15 |
| tristan | tiagogomes, provenance is designed exactly such that we know where everything came from, after composition | 11:15 |
| tristan | provenance carries it, that is exactly what Provenance is for | 11:16 |
| tristan | tiagogomes, https://gitlab.com/BuildStream/buildstream/blob/master/tests/yaml/yaml.py#L167 tests the provenance pretty heavily, though; I think it works | 11:17 |
| tristan | even with list prepend/append and all the whacky combinations, we always know where the winning config originally comes from in any composition | 11:18 |
| tiagogomes | But max-jobs et friends will be always in the final composition right? How can I validate that they were not set beforehand | 11:20 |
| *** lachlan has joined #buildstream | 11:21 | |
| tristan | tiagogomes, you can just check node_get_provenance() | 11:25 |
| tristan | it it returns None, or... maybe a dummy Provenance which is added by _yaml.node_composite(), then we're golden, we know it didnt come from yaml | 11:25 |
| tristan | tiagogomes, if it has a provenance with a filename, then we know it's an error | 11:25 |
| tristan | tiagogomes, right, if it's a provenance from `ensure_provenance()`, then it means we automatically added it in composition | 11:26 |
| tristan | I think None is impossible at that stage | 11:26 |
| tristan | so basically a provenance with filename == '', means it was never set in yaml | 11:27 |
| tiagogomes | It might work then. But it is not better to validate early | 11:31 |
| *** lachlan has quit IRC | 11:32 | |
| tristan | tiagogomes, commented on the issue, we should use that method | 11:35 |
| tristan | And it is early | 11:35 |
| tristan | Really, sprinkling the checks around pre-composition is paranoid, since it is already intended to work this way (and quite heavily tested) | 11:36 |
| tristan | I.e. it is early because; all of that composition will happen, even before preflight; before Element.configure(), it is still right at the start in the load process | 11:36 |
| tiagogomes | But then you can have buildstream to work with an offending project.conf if the command doesn't consume any element. Like `workspace list` | 11:41 |
| *** toscalix has quit IRC | 11:42 | |
| *** lachlan has joined #buildstream | 11:44 | |
| tristan | tiagogomes, it's the case either way, actually | 11:51 |
| tristan | tiagogomes, there was however a request for something like `bst validate`, which would crawl the project and load everything | 11:51 |
| tristan | good for CI | 11:51 |
| *** lachlan has quit IRC | 11:52 | |
| tristan | jjardon, requested at some point, I suspect there is an issue somewhere | 11:52 |
| *** solid_black has joined #buildstream | 11:53 | |
| *** solid_black has quit IRC | 11:56 | |
| *** solid_black has joined #buildstream | 11:56 | |
| cs-shadow | We are trying to do something similar and `bst show` does more or less what `bst validate` might do except the problem that we are hitting is that BuildStream currently has no way of recognizing what are bst elements given a project directory | 11:59 |
| *** solid_black has quit IRC | 12:00 | |
| *** solid_black has joined #buildstream | 12:00 | |
| *** lachlan has joined #buildstream | 12:01 | |
| *** ikerperez has joined #buildstream | 12:03 | |
| *** solid_black has quit IRC | 12:03 | |
| *** solid_black has joined #buildstream | 12:04 | |
| tristan | tiagogomes, oh fwiw, of course the result of the previous conversation is that it is fine to rely on `os.link()`, I agree that that approach for having ArtifactCache.commit() return the added bytes is a good one | 12:04 |
| *** iker has quit IRC | 12:04 | |
| tiagogomes | cs-shadow `cd elements && find -name "*bst" -exec bst show {} \;` ? | 12:04 |
| tiagogomes | Not efficient though | 12:05 |
| tristan | yeah, bst show would just be very slow, though | 12:05 |
| tristan | Also, I don't think it's enough | 12:05 |
| tristan | I think that `bst validate` would have to also try every possible project option branch | 12:05 |
| tristan | because of conditional statements | 12:05 |
| cs-shadow | tiagogomes: Yes, that works but it would be nice if could BuildStream could _find_ files. Also, `-name *.bst` assumes that all files will have bst extension while BuildStream itself doesn't enforce that | 12:06 |
| tiagogomes | Then there is no race condition when returning the size, and everything should™ be accurate | 12:06 |
| tristan | For the moment though, as a poor man's solution; having an element which depends on everything and launching bst show is not bad | 12:06 |
| tristan | cs-shadow, I think that the convention will have to stand for that, though | 12:07 |
| tristan | cs-shadow, otherwise, there is no way for BuildStream to ensure what *it* found was a bst file | 12:07 |
| tristan | cs-shadow, even if we had like, some kind of shebang or YAML encoded file type; we would probably want `bst validate` to tell you when you forgot to mention the encoded file type | 12:08 |
| cs-shadow | tristan: I was actually looking at it from slightly different perspective. If, for example, projects had a way of restricting possible extensions for bst elements, then we could safely ignore things that don't have that extension. At preset, this is something that has to be implemented outside of BuildStream | 12:10 |
| cs-shadow | this is fine, but if BuildStream understood .bst extension, then we could also do other cool things like `bst build --all` | 12:10 |
| *** lachlan has quit IRC | 12:11 | |
| tristan | cs-shadow, myeah I'm not sure what --all does, does it also build it for every architecture supported by the project, for instance ? | 12:12 |
| tristan | I guess it would, but better wait for remote execution for that | 12:12 |
| cs-shadow | tristan: hadn't thought much about that but i guess yes. The need for something like `build --all` arises when one is managing a large project and wants to ensure that at every commit, all elements are building. | 12:14 |
| tristan | cs-shadow, yeah I understand how it can be interesting | 12:15 |
| tiagogomes | I don't see much case for that. If your project have multiple leaves, just do the build for each one or create a stack element including all | 12:15 |
| cs-shadow | tiagogomes: that approach isn't very scalable when your project has thousands of leaves and the structure of the graph keeps changing on a regular basis :) | 12:17 |
| tiagogomes | Is there a concrete example? Why would you've multiple leaves? | 12:18 |
| cs-shadow | yes, but I am not allowed to share it here. But, in general, one may need multiple leaves when you are using BuildStream to generate packages (of some sort) and not system images | 12:19 |
| tristan | cs-shadow, I would say that at least; having a stack element that you use to "generate all packages" is still recommended | 12:21 |
| gitlab-br-bot | buildstream: merge request (richardmaw/subprocess-PWD->master: Ensure PWD is set in process environment) #782 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/782 | 12:21 |
| tristan | cs-shadow, however, it doesn't prevent someone to forget the convention of adding the new package to the main stack element for CI | 12:21 |
| tristan | cs-shadow, so yeah; I do understand the use case :) | 12:21 |
| gitlab-br-bot | buildstream: merge request (mac_fixes->master: WIP: Implement compatibility fixes for MacOSX and WSL Blocks #411 and #412") #726 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/726 | 12:25 |
| gitlab-br-bot | buildstream: merge request (mac_fixes->master: WIP: Implement compatibility fixes for MacOSX and WSL Blocks #411 and #412") #726 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/726 | 12:25 |
| * cs-shadow couldn't find existing issues `bst validate` so will create new issues | 12:26 | |
| gitlab-br-bot | buildstream: merge request (mac_fixes->master: WIP: Implement compatibility fixes for MacOSX and WSL Blocks #411 and #412") #726 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/726 | 12:26 |
| gitlab-br-bot | buildstream: merge request (richardmaw/subprocess-PWD->master: Ensure PWD is set in process environment) #782 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/782 | 12:29 |
| Nexus | juergbi: How would you propose we check for whether the host supports local b | 12:29 |
| Nexus | uilds | 12:29 |
| Nexus | rather than simple missing installed packages | 12:29 |
| gitlab-br-bot | buildstream: issue #638 ("RFE: Add `bst validate` command") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/638 | 12:32 |
| *** solid_black has quit IRC | 12:36 | |
| gitlab-br-bot | buildstream: issue #640 ("RFE: Allow easy way of building all elements") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/640 | 12:37 |
| *** solid_black has joined #buildstream | 12:37 | |
| *** solid_black has quit IRC | 12:38 | |
| *** solid_black has joined #buildstream | 12:39 | |
| *** solid_black has quit IRC | 12:39 | |
| *** tristan has quit IRC | 12:49 | |
| jmac | I'm getting a lot of "RemovedInPytest4Warning: MarkInfo objects are deprecated" warnings - is this something people have seen or solved in the past two weeks I've been away? | 12:55 |
| *** tristan has joined #buildstream | 13:01 | |
| *** lachlan has joined #buildstream | 13:01 | |
| *** ChanServ sets mode: +o tristan | 13:02 | |
| juergbi | Nexus: this is a per-platform question. for Darwin/macOS it's clear as we know it won't work yet. for Linux/WSL, one option would be to test whether bwrap works or not (we already test user namespace but we don't test basic functionality yet) | 13:05 |
| Kinnison | jmac: are you somehow a major version of pytest ahead? I'm using 3.7.4 and that seems okay | 13:07 |
| juergbi | Nexus: it could theoretically also be a specific check for WSL but checking whether bwrap and FUSE are not working/available is better, imo, as this also covers e.g. old Linux and it will also be the right thing for future WSL | 13:07 |
| * Nexus is on 3.5.3 | 13:07 | |
| jmac | Looks like pytest 3.8.0 here | 13:08 |
| tristan | Kinnison, about to run out to the gym but, fwiw I've been hunting down the "why does pylint behave differently in different places" thing today in background | 13:08 |
| Nexus | 3.6.4 | 13:08 |
| Kinnison | tristan: cool, hopefully you'll work it out. Have fun at the gym | 13:09 |
| tristan | Kinnison, I was recommended to check the versions of astroid, mccabe and isort (pylint deps) but mine are the same as in CI, yet my tests fail | 13:09 |
| Kinnison | blerp, less cool :( | 13:09 |
| tristan | just as an update, in case someone goes down that path, I tried pinning those, but fail | 13:09 |
| *** leopi has quit IRC | 13:09 | |
| tristan | yeah, maybe a specific version of pytest might change things, we are minimal bound 3.7 right now, and 3.8 brings in a ton of pytest-datafiles related warnings | 13:10 |
| Kinnison | presumably readying for the pytest4 API changes? | 13:11 |
| *** toscalix has joined #buildstream | 13:16 | |
| gitlab-br-bot | buildstream: merge request (richardmaw/test-config-fixes->master: Fix tests that attempt to access the home directory) #780 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/780 | 13:18 |
| tristan | Kinnison, I am getting recommended to try tox/nox for running tests; not sure if we should change to that harness instead of setup.py | 13:21 |
| Kinnison | I have used nose in the past, I've never tried tox/nox | 13:22 |
| tristan | Also, of course everybody in #python loves venvs, but I don't like to impose that hassle on every developer :-S | 13:22 |
| * SotK recommends tox whenever he gets the chance | 13:22 | |
| * Kinnison strongly recommends venvs for all python work | 13:22 | |
| Kinnison | version madness is reduced dramatically | 13:23 |
| SotK | +1 | 13:23 |
| persia | tox -evenv is a good combination | 13:23 |
| tristan | Kinnison, the problem I see with venvs though; is ultimately you should get BuildStream through your distro, integrated, with bash completions and man pages "just working", and no fuss and no typing | 13:27 |
| tristan | Kinnison, if we go the route of venvs, we paint ourselves into the "python bubble" corner | 13:27 |
| tristan | So I worry about that too | 13:27 |
| Kinnison | tristan: True, but working with distro devs to ensure things are good for the next release of a distro is different from people needing to run things on older/diverse distros | 13:28 |
| tristan | Then again, I'm trying to think of how we can bundle all our exact deps into a single package for distros, might be a middle ground | 13:28 |
| Kinnison | Debian, at least, won't like that | 13:28 |
| persia | At this point, most distro devs are proficient at finding combinations of versions that let them deliver python modules primarily tested with venvs to users as native packags. | 13:29 |
| jmac | Pinning pytest to 3.7.0 in dev_requirements.txt removes the RemovedInPytest4 errors, but now I get a load of pylint errors | 13:29 |
| tristan | jmac, so does 3.7.any_version | 13:29 |
| persia | Also, yeah, distro folk tend not to like it when third-parties try to be fancy. Something with pip and venv doesn't mess with other distro stuff, so is a safer recommendation to a user (from a distro support perspective). | 13:30 |
| tristan | jmac, what pytest version did you have *before* you got pylint errors, that is very interesting to know | 13:30 |
| jmac | tristan: I was using 3.8.0, but I couldn't really tell if those pylint errors were there, as my output was swamped by the RemovedInPytest4 errors | 13:31 |
| tristan | jmac, ah yeah, they probably were | 13:31 |
| * Kinnison has 3.7.4 and wasn't getting lint messages, but is mid an integration test run so will be a while before can report | 13:32 | |
| *** lachlan has quit IRC | 13:34 | |
| *** lachlan has joined #buildstream | 13:39 | |
| *** lachlan has quit IRC | 13:43 | |
| *** toscalix has quit IRC | 13:43 | |
| *** lachlan has joined #buildstream | 13:50 | |
| Kinnison | Okay, so I've finished my integration run and I can confirm some odd lint errors | 13:55 |
| Kinnison | Or rather, some lint errors which I'm fairly sure I didn't get before | 13:55 |
| Kinnison | things like: | 13:55 |
| Kinnison | _____________________________________________________________________ [pylint] buildstream/plugin.py _____________________________________________________________________ | 13:55 |
| Kinnison | R:755, 8: Consider using dict.get for getting values from a dict if a key is present or a default if not (consider-using-get) | 13:55 |
| jmac | Kinnison: Yes, I'm getting that. You don't need an integration run to get it. | 13:57 |
| Kinnison | Yeah, I just happened to be doing one :-) | 13:57 |
| Kinnison | pylint 0.12.3 for me I think | 13:59 |
| *** lachlan has quit IRC | 14:00 | |
| *** lachlan has joined #buildstream | 14:01 | |
| *** lachlan has quit IRC | 14:04 | |
| tiagogomes | Hmm, source plugins don't use variables | 14:05 |
| *** lachlan has joined #buildstream | 14:25 | |
| gitlab-br-bot | buildstream: merge request (tiagogomes/issue-287->master: Add validation of configuration variables) #678 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/678 | 14:30 |
| gitlab-br-bot | buildstream: merge request (juerg/cas-batch->master: WIP: _artifactcache/casserver.py: Implement BatchReadBlobs) #785 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/785 | 14:32 |
| gitlab-br-bot | buildstream: merge request (juerg/cas-batch->master: _artifactcache/casserver.py: Implement BatchReadBlobs) #785 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/785 | 14:33 |
| jmac | Looks like pytest-pylint >= 0.12.2 causes the new pylint warnings. Running `pip3 install pytest-pylint==0.12.1` will get rid of them. | 14:33 |
| * Kinnison guesses fixing the lints would be sensible over-all, but thanks for working that one out jmac | 14:34 | |
| *** lachlan has quit IRC | 14:35 | |
| jmac | Took me 20 minutes to realise pylint and pytest-pylint were separate things too, grumble | 14:36 |
| *** lachlan has joined #buildstream | 14:36 | |
| juergbi | finn, jmac, mablanch: if/when one of you has some time for reviews: https://gitlab.com/BuildStream/buildstream/merge_requests/785 and https://gitlab.com/BuildStream/buildbox/merge_requests/3 | 14:36 |
| jmac | I will take a look | 14:40 |
| jmac | at !785 | 14:40 |
| finn | will have a quick look | 14:43 |
| juergbi | ta | 14:45 |
| gitlab-br-bot | buildstream: merge request (richardmaw/subprocess-PWD->master: Ensure PWD is set in process environment) #782 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/782 | 14:47 |
| jmac | juergbi: I'm guessing setting something in the ServerCapabilities object causes BatchReadBlobs to be called? | 14:51 |
| juergbi | jmac: on the buildstream side, this is only server support so far. client support in buildstream is still pending | 14:52 |
| juergbi | but I've tested it with buildbox client support | 14:52 |
| *** lachlan has quit IRC | 14:53 | |
| tiagogomes | how do I run only the pep8 checker? | 14:54 |
| jmac | ./setup.py test --addopts "-m pep8" | 14:54 |
| tiagogomes | perfect, thanks | 14:55 |
| jmac | juergbi: I'm just wondering what adding the BatchReadBlobs method does. Was there a default method in the superclass being called before? | 14:55 |
| juergbi | jmac: no, it was completely missing, i.e., clients trying to use it would get an error so far | 14:56 |
| jmac | Aha, thanks | 14:56 |
| juergbi | the auto-generated stub is there, of course, but no implementation | 14:56 |
| *** lachlan has joined #buildstream | 14:57 | |
| gitlab-br-bot | buildstream: merge request (tiagogomes/issue-287->master: Add validation of configuration variables) #678 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/678 | 15:00 |
| *** phildawson has quit IRC | 15:01 | |
| *** phildawson has joined #buildstream | 15:02 | |
| *** lachlan has quit IRC | 15:02 | |
| *** finn_ has joined #buildstream | 15:04 | |
| *** finn has quit IRC | 15:05 | |
| *** lachlan has joined #buildstream | 15:18 | |
| gitlab-br-bot | buildstream: merge request (tiagogomes/issue-287->master: Add validation of configuration variables) #678 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/678 | 15:27 |
| Nexus | I came across an interesting issue, where i had a project.conf file in my `/user/` directory, which buildstream picked up as being a bst project, but then started failing tests like the `test_missing_project_conf` test | 15:32 |
| Nexus | is there some way we can limit this? also do we want to nested projects? | 15:33 |
| * tlater[m] has noticed this before | 15:33 | |
| Nexus | to have* | 15:33 |
| tlater[m] | I think project.conf is a little generic of a file name to recognize something as a buildstream project. | 15:33 |
| tlater[m] | Maybe we should check for a .buildstream directory instead? | 15:33 |
| Nexus | agreed | 15:34 |
| tlater[m] | Nexus: I don't see any problem with nested projects. It may be a really strange thing to do, but there's no immediate problem with it. | 15:35 |
| Nexus | it just seems really messy to me | 15:37 |
| Nexus | but i can't really explain why | 15:37 |
| *** lachlan has quit IRC | 15:40 | |
| gitlab-br-bot | buildstream: merge request (juerg/cas-batch->master: _artifactcache/casserver.py: Implement BatchReadBlobs) #785 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/785 | 15:42 |
| jmac | juergbi: !785 makes sense to me | 15:42 |
| juergbi | thanks | 15:42 |
| gitlab-br-bot | buildstream: merge request (juerg/cas-batch->master: _artifactcache/casserver.py: Implement BatchReadBlobs) #785 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/785 | 15:43 |
| *** finn_ is now known as finn | 15:46 | |
| finn | juergbi, can't spot anything bad on 785 | 15:47 |
| juergbi | finn: have you removed the question or is gitlab acting up? BuildStream doesn't have an action cache, so we also don't advertise updates | 15:47 |
| juergbi | thanks | 15:47 |
| finn | I removed a question, I was about to ask why cache_capabilities.action_cache_update_capabilities.update_enabled = False but then realised it was a silly question | 15:47 |
| gitlab-br-bot | buildstream: merge request (juerg/cas-batch->master: _artifactcache/casserver.py: Implement BatchReadBlobs) #785 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/785 | 15:48 |
| *** lachlan has joined #buildstream | 15:54 | |
| gitlab-br-bot | buildstream: merge request (willsalmon/outOfSourecBuild->master: WIP: out of source builds) #776 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/776 | 15:55 |
| gitlab-br-bot | buildstream: merge request (richardmaw/builddir-sockets->master: WIP: Fix: While caching build artifact: "Cannot extract [path to socket file] into staging-area. Unsupported type.") #783 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/783 | 16:00 |
| tristan | skullman, Nexus; I think this was merged pre-maturely https://gitlab.com/BuildStream/buildstream/merge_requests/782#note_100153822 | 16:01 |
| tristan | really, we have to start doing better in review; this one isn't horrible but from my experience this weekend, we're letting too much nonsense slip in | 16:02 |
| gitlab-br-bot | buildstream: issue #632 ("CAS server: Implement BatchReadBlobs") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/632 | 16:07 |
| gitlab-br-bot | buildstream: merge request (juerg/cas-batch->master: _artifactcache/casserver.py: Implement BatchReadBlobs) #785 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/785 | 16:07 |
| *** lachlan has quit IRC | 16:08 | |
| tiagogomes | tristan I think only the main process should write to the cache_size file | 16:10 |
| tristan | tiagogomes, I agree | 16:11 |
| tristan | tiagogomes, in fact if you fix that; I would venture an assert utils._is_main_process() in ArtifactCache._write_cache_size() | 16:11 |
| tristan | assertions is mostly what _is_main_process() is for | 16:12 |
| tiagogomes | Does that already written | 16:12 |
| tristan | I dont understand, is it a question ? | 16:13 |
| tristan | Or, you have a patch, and it already raises that assertion ? | 16:13 |
| tiagogomes | sorry, is utils._is_main_process() a concrete thing now? Or needs to be implemented | 16:14 |
| tiagogomes | it exists! | 16:15 |
| tristan | yeah | 16:15 |
| tristan | tiagogomes, I would prefer that it be used as an assertion rather than a check | 16:16 |
| tristan | tiagogomes, that encourages code that knows what it's doing, and gets enforced | 16:16 |
| tristan | rather than hap-hazardly calling ArtifactCache._write_cache_size() | 16:16 |
| tristan | Does ArtifactCache.set_cache_size() have a chance of being called in a task anyway ? | 16:17 |
| tristan | Maybe *that* is where the assertion should go, rather | 16:18 |
| tristan | We usually call it in response to the completion of a task which calculates either a size to be added, or calculates the whole size | 16:18 |
| tristan | I guess it shouldnt really make sense to call it from a task | 16:18 |
| tiagogomes | yes, you calling it on the cleanup | 16:25 |
| *** lachlan has joined #buildstream | 16:33 | |
| *** lachlan has quit IRC | 16:41 | |
| *** lachlan has joined #buildstream | 16:42 | |
| *** lachlan has quit IRC | 16:46 | |
| gitlab-br-bot | buildstream: merge request (richardmaw/subprocess-PWD->master: WIP: Address post-merge review of Ensure PWD is set in process environment) #788 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/788 | 16:46 |
| *** lachlan has joined #buildstream | 17:06 | |
| *** lachlan has quit IRC | 17:14 | |
| *** leopi has joined #buildstream | 17:20 | |
| *** lachlan has joined #buildstream | 17:26 | |
| tristan | tiagogomes, ah right; it's not particularly harmful as it stands; but it could be nicer to send it back to the main process (which I think we do anyway) and set it once there | 17:28 |
| tristan | will require a bit different handling of the cleanup loop, though | 17:28 |
| *** lachlan has quit IRC | 18:20 | |
| *** lachlan has joined #buildstream | 18:31 | |
| gitlab-br-bot | buildstream: issue #641 ("Error out if `%{max-jobs}` is substituted in `Element.node_subst_member()`") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/641 | 19:00 |
| *** lachlan has quit IRC | 19:19 | |
| *** cs-shadow has quit IRC | 21:25 | |
| *** tristan has quit IRC | 21:29 | |
| *** leopi has quit IRC | 21:52 | |
| *** leopi has joined #buildstream | 21:53 | |
| *** leopi has quit IRC | 21:57 | |
| *** leopi has joined #buildstream | 21:57 | |
| *** bochecha has quit IRC | 22:27 | |
| *** alatiera_ has quit IRC | 22:48 | |
| gitlab-br-bot | buildstream: issue #642 ("Incorrect error when malformed project.conf is found") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/642 | 23:01 |
| *** leopi has quit IRC | 23:08 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!