| *** tristan has quit IRC | 02:21 | |
| *** tristan has joined #buildstream | 02:44 | |
| *** ChanServ sets mode: +o tristan | 02:44 | |
| juergbi | tomaz: based on the log it seems to be using build tools from /cross/x86_64-unknown-linux-gnu. the default include path of that compiler might be something in /cross and not /usr/include | 06:24 |
|---|---|---|
| juergbi | if you need more details, #freedesktop-sdk is probably a better place to ask | 06:25 |
| tristan | So a Digest is a remote_execution_pb2.Digest ? Couldn't we have a toplevel __init__.py somewhere where we can import important types which we interface with from the core ? | 06:52 |
| * tristan is enjoying mypy | 06:53 | |
| juergbi | the toplevel __init__.py would be public, though | 07:16 |
| juergbi | tristan: I've discovered an interesting bug (I think) with the filter plugin | 07:17 |
| juergbi | if `pass_integration` is set, it assumes the underlying element is always available in the cache when integrate() is called on the filter | 07:18 |
| juergbi | even though the underlying element is a build-only dependency | 07:18 |
| juergbi | as it calls integrate on the underlying element, which attempts to get public data | 07:19 |
| juergbi | I think the proper solution is to copy the integration-commands from the underlying element into the filter element at assemble time | 07:20 |
| juergbi | tristan: does this make sense to you or am I missing something? | 07:20 |
| juergbi | (and stop overriding core Element.integrate() in Filter) | 07:21 |
| tristan | Interesting | 07:23 |
| tristan | Is that the simplest way for a filter element to behave in the way people wanted it to behave ? | 07:24 |
| tristan | As I recall, a filter is mostly a "pass-through" element, commands are routed to the element it filters | 07:24 |
| tristan | In the dependency graph the filtered element is build-only indeed, but for anything to work, it would seem imperative that the filter element be present | 07:25 |
| tristan | Maybe not it's artifact | 07:25 |
| tristan | juergbi, You are either missing something, or it is a larger problem than just this. | 07:26 |
| tristan | We're talking about public data here | 07:27 |
| juergbi | tristan: redirect/pass-through is only used for track and workspace commands | 07:27 |
| juergbi | in all other cases it acts as standalone element | 07:27 |
| tristan | We're talking about inheriting public data from the filtered element... that is an element which is out of scope for a reverse dependency | 07:28 |
| juergbi | I see the issue for bst artifact checkout with the default runtime dependency selection | 07:28 |
| tristan | Ok no it's not more complex than this | 07:28 |
| tristan | I think that; it's public data we should be thinking about: Not integration commands specifically | 07:28 |
| tristan | juergbi, I was worried, and maybe it's still a relevant worry ? ... that if build-only dependencies are not present, is there data we could be missing ? | 07:29 |
| tristan | Once an element has been built, we no longer need the artifact in some cases, in those cases, is it still possible to need the public data ? | 07:29 |
| tristan | I think, hope not: No. | 07:29 |
| juergbi | same here | 07:30 |
| juergbi | my branch with explicit cache queries would definitely detect such issues if there are more | 07:31 |
| juergbi | regarding just integration commands vs. all public data: right now we only have a pass-integration flag in the filter element | 07:32 |
| juergbi | maybe that needs to be extended/generalized? not sure | 07:32 |
| juergbi | however, this can be considered afterwards. I think we can retain the current behavior and just fix the cache issue by copying integration-commands in Filter.assemble() | 07:33 |
| juergbi | that said, maybe it is worth thinking about the whole thing before | 07:34 |
| tristan | It's a bit of a tough call, maybe it needs to be granular indeed | 07:38 |
| tristan | I can imagine you want to filter an element, and maybe redefine split domains at the same time | 07:38 |
| tristan | juergbi, I would certainly be happy with your suggested fix in advance of anything else though | 07:38 |
| tristan | What's the story with SourceRef anyway ? | 07:39 |
| juergbi | ok, will try to fix this and write a test for it | 07:39 |
| tristan | How come it doesnt have _SourceRef if it's gonna be private ? | 07:39 |
| * tristan overlooks this shortcoming for now | 07:40 | |
| tristan | Maybe it's a good idea to make SourceRef a strong type in the API I don't know | 07:40 |
| juergbi | it is indeed already used in public API | 07:40 |
| juergbi | e.g. return type of Source.get_ref() | 07:40 |
| tristan | But allowing plugins to reach into the guts of buildstream to import it from buildstream.types, _only_ so that they can run mypy, is not good | 07:41 |
| tristan | Exactly | 07:41 |
| tristan | it's either a thing or it's not, though | 07:41 |
| juergbi | is this also rather a historic definition? | 07:41 |
| tristan | it's in the mypy decorations and that's it, I don't know if that makes it public, or if the mypy decorations need to be fixed | 07:41 |
| tristan | Ummm, definition of what ? | 07:42 |
| tristan | The ref ? | 07:42 |
| juergbi | could it just be Dict[str, Any] ? | 07:42 |
| tristan | It can really be any of the Union afaict | 07:43 |
| juergbi | oh, the definition is even incorrect, afaict. plain `str` is also used by plugins despite not being allowed | 07:43 |
| tristan | Indeed | 07:43 |
| tristan | good point | 07:43 |
| juergbi | could it be a Node instead? | 07:43 |
| juergbi | that might be problematic to construct, I suppose | 07:44 |
| juergbi | but conceptually that's pretty much what it needs to be, isn't it? | 07:44 |
| tristan | A Node ? | 07:48 |
| tristan | Well | 07:48 |
| tristan | That depends | 07:48 |
| tristan | Are we happy enough with mutable public data ? | 07:48 |
| tristan | I don't know, what is your feeling about that ? Can we trust it ? ... | 07:48 |
| tristan | Trying to think realistically about how much we can corner plugins and disallow them from manipulating data | 07:49 |
| tristan | People *are* composing lists and dicts and blobs of data in public data, passing them on to other plugins, which consume these and "do stuff" with them | 07:49 |
| tristan | All of this is outside of the sandbox | 07:49 |
| tristan | Now that we've sealed off runtime dependencies | 07:50 |
| tristan | Another point, is that we still don't have an assert afaics, ensuring that Element.get_public_data() is *never* called outside of Element.{stage,prepare,prepare_sandbox,assemble}() | 07:51 |
| tristan | I.e. "build time": the only time when an element can be sure that public data is prepared and ready | 07:51 |
| tristan | All bets are off if you call Element.get_public_data() inside Element.configure_dependencies() or Element.preflight() or Element.get_unique_key() | 07:52 |
| tristan | And all of that is because public data decided to be mutable. | 07:52 |
| tristan | juergbi, If public data becomes immutable; I would say we remove all that side of Node API, and as such, a unique key is certainly not a Node (as a Node would really be a readonly YAML configuration loading mechanism) | 07:53 |
| gitlab-br-bot | tristanvb opened MR !2072 (tristan/remove-bst-key-requires-stage->master: source.py: Remove BST_KEY_REQUIRES_STAGE) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2072 | 07:54 |
| juergbi | I don't think these two aspects are that related. ideally a ref is indeed an immutable object. however, dicts are also mutable, so not as different | 07:54 |
| tristan | Well | 07:55 |
| juergbi | making public data immutable would fix the filter issue, though, afaict | 07:55 |
| juergbi | as then get_public_data() would not require the artifact to be available | 07:55 |
| tristan | It would require a Node to support from_dict() and such at least | 07:55 |
| tristan | Which is just a hoop for a plugin to jump through, if they already have a dict (or 2 hoops if they have an 'str') | 07:56 |
| juergbi | yes, Node as ref type might indeed not be the best choice but I think this discussion is separate from public data | 07:56 |
| tristan | I think that was 2 discussions ago | 07:56 |
| tristan | Oh no sorry got mixed up | 07:56 |
| tristan | Sure, it's separate from public data, I do feel that public data being immutable (or more accurately read-only) is more of a cause for it to be an inappropriate choice | 07:57 |
| tristan | but that's just... meh, I think make SourceRef public would be acceptable too | 07:57 |
| tristan | Just the current definition is a mess for SourceRef | 07:58 |
| tristan | it says it's private, omits it's underscore, neglects to be imported in toplevel __init__.py (the public API source of truth), doesnt have a docstring, yet plugins access it in violation of it's privateness | 07:59 |
| tristan | just a mess :) | 07:59 |
| tristan | Anyway, !2072 might pass tests, it's a nice little refactor (that we discussed yesterday) | 07:59 |
| tristan | I found a single context manager was more effective than 2 APIs | 08:00 |
| tristan | With self._cache_directory() as directory: ... put stuff in directory, and directory._get_digest() ... | 08:00 |
| tristan | With self._cache_directory(digest=self._digest): as cached_directory: ... directory.import_files(cached_directory) ... | 08:01 |
| juergbi | tristan: the _is_trackable call needs to be removed from trackqueue, afaict | 08:03 |
| juergbi | the context manager lgtm | 08:03 |
| juergbi | I'm not happy about the duplication between local and workspace but it's not that much, and I definitely like the reduction of conditionals in core code | 08:06 |
| juergbi | wondering whether a common base class to LocalSource and WorkspaceSource would be worth it | 08:07 |
| juergbi | probably not | 08:07 |
| tristan | oops, was just thinking that as I stepped out to smoke (_is_trackable()) | 08:08 |
| *** santi has joined #buildstream | 08:44 | |
| gitlab-br-bot | tristanvb opened (was WIP) MR !2068 (tristan/fix-overnight-tests->master: .gitlab-ci.yml: Use ported plugins and fdsdk for overnight tests) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2068 | 08:52 |
| gitlab-br-bot | tristanvb approved MR !2068 (tristan/fix-overnight-tests->master: .gitlab-ci.yml: Use ported plugins and fdsdk for overnight tests) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2068 | 08:52 |
| tristan | Damn, the refactor is failing integration tests for workspaces... lets see | 08:58 |
| * tristan hoped he could get this little refactor out of the way in a matter of an hour or two... | 08:59 | |
| gitlab-br-bot | tcanabrava opened issue #1396 (Add a way to force a local build) on buildstream https://gitlab.com/BuildStream/buildstream/-/issues/1396 | 09:08 |
| gitlab-br-bot | juergbi opened MR !2073 (juerg/filter-pass-integration->master: filter.py: Combine integration commands in assemble()) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2073 | 09:48 |
| tristan | juergbi, I'm having a hard time with this strange side effect of the patch: It seems that files are staged without write permission (at least where workspaces are involved), any clue what could result in staged files missing their permissions ? | 10:02 |
| tristan | Interestingly, FileBasedDirectory says "if executable: os.chmod(very verbose version of 755)", without any "else: os.chmod(verbose version of 644)" | 10:07 |
| tristan | Ah, I get it | 10:13 |
| tristan | The way it was working before, it kept using the real stage method when opening a workspace | 10:13 |
| tristan | Still, this seems to indicate a deeper problem: that writable bits are getting lost somewhere while importing files into cas and then exporting them to FileBasedDirectory | 10:14 |
| juergbi | we don't currently store writable bits | 10:28 |
| juergbi | this should be considered as part of #38 | 10:28 |
| gitlab-br-bot | Issue #38: Lost file metadata in artifacts and images https://gitlab.com/BuildStream/buildstream/-/issues/38 | 10:28 |
| gitlab-br-bot | marge-bot123 merged MR !2068 (tristan/fix-overnight-tests->master: .gitlab-ci.yml: Use ported plugins and fdsdk for overnight tests) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2068 | 10:28 |
| juergbi | individual permission bits don't really make sense without the rest, imo (except for the executable bit) | 10:28 |
| tristan | juergbi, if we don't consider them, then I believe they are either 0644 or 0755 | 10:29 |
| tristan | Not read-only | 10:29 |
| juergbi | I think we should indeed export them as one of those two, yes. or rather, 0666/0777 & ~UMASK | 10:30 |
| tristan | Surely, at build time, we don't stage files and build with -r--r--r-- | 10:30 |
| tristan | that sounds prone to failure, all read-only files | 10:30 |
| juergbi | at build time that would be dealt with by buildbox. we just hand the CAS tree without those extra permission bits | 10:31 |
| juergbi | however, for the non-FUSE case where we use hardlinks, they will have to be read-only | 10:31 |
| tristan | juergbi, Not & ~UMASK I think, unless umask is hardcoded in BuildStream (or BuildBox) of course | 10:31 |
| tristan | I recall adding tests to ensure that file permissions were not susceptible to host umask | 10:32 |
| juergbi | as part of the build we definitely want it to be as deterministic as possible | 10:32 |
| tristan | At workspace checkout perhaps host umask makes sense | 10:32 |
| juergbi | however, for bst artifact checkout umask can make sense | 10:32 |
| tristan | that too | 10:33 |
| juergbi | btw: buildbox-fuse always sets the mode to 0644 or 0755 | 10:33 |
| juergbi | i.e., builds with buildbox-fuse should be identical to what we had before buildbox | 10:33 |
| juergbi | however, without fuse it's not really possible, unfortunately | 10:34 |
| tristan | juergbi, anyway I've updated !2072 by restoring the old behavior for local source workspace open, with a FIXME comment | 10:34 |
| gitlab-br-bot | MR !2072: source.py: Remove BST_KEY_REQUIRES_STAGE https://gitlab.com/BuildStream/buildstream/-/merge_requests/2072 | 10:34 |
| tristan | juergbi, I think it just seems odd to me that (A) cache in CAS... (B) Checkout to local FS, results in read-only files for regular files | 10:36 |
| tristan | I think that it would make sense to tack on that extra else clause in FileBasedDirectory(), assuming that is the thing that is only used for artifact checkout and workspace open | 10:36 |
| juergbi | yes, I thought for checkout (with copies, not hardlinks) we create 0644/0755 files (or possibly umask-based) | 10:37 |
| tristan | I guess not workspace open for other things either, but it *should* think, you should be able to open a workspace on sources in the CAS which you never downloaded | 10:37 |
| tristan | juergbi, if artifact checkout is implemented as `FileBasedDirectory(external_directory=checkout_dir).import_files(the artifact files)`, then we'll have read-only files | 10:38 |
| * tristan is at the end... will be running now... | 10:38 | |
| juergbi | it uses export_files() | 10:39 |
| juergbi | which uses safe_copy with copystat=False | 10:40 |
| juergbi | that should result in creation of writable files | 10:40 |
| tristan | sounds strange... no time to try to figure out the justification of having both import_files and export_files (at surface value, looks like only one of those should be needed, just use the right abstraction to import *to*, differing implementations here could be a source of bugs) | 10:42 |
| * tristan escapes... | 10:42 | |
| *** tristan has quit IRC | 10:45 | |
| robjh | hi gang, anyone have any idea which piece of the buildstream puzzle is broken here? https://gitlab.com/CodethinkLabs/lorry/lorry-buildstream/-/jobs/753420888 | 14:18 |
| robjh | looks like its more remote cache issues? | 14:18 |
| juergbi | robjh: the NOT_FOUND message is wrapped in an UNKNOWN error for some reason. not sure whether that's due to a bug in buildstream (1.4) or a server issue | 14:38 |
| juergbi | I don't recall seeing "Exception iterating responses: <_MultiThreadedRendezvous" before | 14:39 |
| robjh | well, apparently the cache server has a failing ssd. i dont know what the nature of the failures is, but I think its probably not worthwhile to try and diagnose this while the hardware is faulty | 14:45 |
| robjh | i do think it would be more sensible for buildstream to treat failing to retrieve from the cache as if it were a cache miss instead of outright failing though | 14:46 |
| tomaz | hey, if I'm on xterm or gnome-terminal or whatever terminal, how can I determine if I'm on a buildstream shell or the local shell? (programatically) | 15:01 |
| juergbi | robjh: we discussed yesterday that it would make sense to fall back from pull to build even for unexpected failures as long as we still mark the pull job as failed (not skipped) in that case | 15:01 |
| tomaz | I'm doing some tests and I need to be able to say "I'm on my local bash", "I'm on my bst bash" | 15:02 |
| juergbi | in interactive mode, it would be up to the user ('continue' would trigger the fallback to local build) | 15:02 |
| juergbi | otherwise it would depend on the --on-error setting | 15:02 |
| juergbi | tomaz: hm, not sure whether there is a clean way to do this. could potentially check mounts or the PID tree but I don't know a good check off the top of my head | 15:04 |
| tomaz | juergbi: cool, thanks. :) | 15:14 |
| juergbi | tomaz: ah, actually, the hostname could be an option | 15:14 |
| juergbi | in master with buildbox-run-bubblewrap the hostname is always `buildbox` | 15:15 |
| juergbi | and iirc, in buildstream 1 it was always `buildstream` | 15:15 |
| tomaz | ah, that's perfect, thanks. | 15:27 |
| tomaz | <3 | 15:27 |
| *** WSalmon has quit IRC | 17:13 | |
| *** WSalmon has joined #buildstream | 17:14 | |
| *** santi has quit IRC | 17:41 | |
| *** benschubert has joined #buildstream | 18:06 | |
| *** tomaz has quit IRC | 19:20 | |
| *** lchlan has quit IRC | 20:50 | |
| *** jjardon has quit IRC | 20:50 | |
| *** benschubert has quit IRC | 20:50 | |
| *** jward has quit IRC | 20:50 | |
| *** lchlan has joined #buildstream | 20:50 | |
| *** benschubert has joined #buildstream | 20:51 | |
| *** jjardon has joined #buildstream | 20:52 | |
| *** ChanServ sets mode: +o jjardon | 20:52 | |
| *** jjardon has quit IRC | 20:53 | |
| *** jjardon has joined #buildstream | 20:53 | |
| *** ChanServ sets mode: +o jjardon | 20:53 | |
| *** jward has joined #buildstream | 20:53 | |
| *** benschubert has quit IRC | 22:04 | |
| *** benschubert has joined #buildstream | 22:04 | |
| *** benschubert has quit IRC | 22:49 | |
| *** jjardon has quit IRC | 22:49 | |
| *** lchlan has quit IRC | 22:49 | |
| *** jjardon has joined #buildstream | 22:53 | |
| *** ChanServ sets mode: +o jjardon | 22:53 | |
| *** benschubert has joined #buildstream | 22:53 | |
| *** jjardon has quit IRC | 22:54 | |
| *** lchlan has joined #buildstream | 22:54 | |
| *** jjardon has joined #buildstream | 22:54 | |
| *** ChanServ sets mode: +o jjardon | 22:54 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!