IRC logs for #buildstream for Thursday, 2018-03-01

*** xjuan has quit IRC00:50
*** mcatanzaro has quit IRC04:24
*** persia has quit IRC06:01
*** waltervargas[m] has quit IRC06:01
*** persia has joined #buildstream06:03
*** waltervargas[m] has joined #buildstream06:06
*** hase has joined #buildstream07:06
*** hase has quit IRC07:08
*** i0d3l4y has joined #buildstream07:08
*** i0d3l4y has quit IRC07:33
*** dominic has joined #buildstream08:09
*** dominic has joined #buildstream08:09
*** aday has joined #buildstream09:34
*** tristan has joined #buildstream09:47
*** jonathanmaw has joined #buildstream09:51
tristanjuergbi, I was able to run totem with access to /dev/dri, without severe tweaks to the core bwrap sandbox10:59
juergbi\o/10:59
juergbihow did you solve the permission issue?10:59
tristanWell, I didnt11:00
juergbisudo chmod a+rw?11:00
tristannope11:00
tristanThe devices show up as nobody:nogroup11:00
juergbiah but the gid check is still done outside the namespace?11:00
tristanThere is one tweak to the bwrap sandbox though, and I'm thinking; lets just apply it across the board for all mounts11:00
juergbiwhich is?11:01
tristanBasically, we are using --bind for all mounts that we setup11:01
tristanBut, it works if you use --bind-dev (doh!)11:01
tristanbwrap --help says: "--bind SRC DEST              Bind mount the host path SRC on DEST"11:02
tristanand "--dev-bind SRC DEST          Bind mount the host path SRC on DEST, allowing device access"11:02
juergbiah, --bind uses nodev11:02
tristanWell, in bwrap it will basically do... yeah that11:02
tristanSo, I feel like, allowing device access across the board, does not effect build sandboxes11:03
juergbias those mount sources anyway can't contain device nodes11:03
tristanbecause it's all stuff we stage there anyway11:03
tristanright, the alternative would be dual code paths and special casing11:03
juergbiyes, i can't think of an issue out of the top of my head11:03
tristanThere is something else I needed hehe (but not from core features)11:04
juergbia driver?11:04
tristanIf I only mount /dev/dri, then I get "MESA-LOADER: failed to retrieve device information"11:04
tristantwice, at startup11:04
tristanthen, if I try to play a video11:05
juergbidoes it use /sys to get device info?11:05
tristantotem just crashes with this message from mesa:11:05
tristanintel_do_flush_locked failed: Invalid argument11:05
juergbihm11:05
tristanMounting /sys fixes that, exactly11:05
juergbiso it seems it may make sense to support mounting a couple of directories via project conf after all11:06
tristanSo wonder if it's sane... mount /dev/dri and /sys11:06
juergbinot real filesystems but still11:06
tristanyeah, right now I'm testing on command line11:06
juergbifor the weak sandbox, i don't really see an issue11:06
juergbiwe don't want those in the build sandbox11:07
juergbibut should also not need them11:07
tristan(with my added --mount support branch)11:07
juergbidoes flatpak mount whole /sys as well or do they restrict it somehow?11:07
tristanflatpak does similar, I'm not sure what it's doing for /sys though11:07
tristanbut it will try to mount /dev/dri11:07
tristanif access to dri was requested (actually it has a hard coded list, including mali etc, which it tries)11:08
juergbihttps://github.com/flatpak/flatpak/blob/master/common/flatpak-run.c#L225311:09
tristanWith our approach; we could similarly list those in a project.conf, and it will just mount any of the things which exist, and (currently) warn if they dont exist11:09
juergbimounting a subset11:09
tristanah I see, I didnt get that far11:10
tristanpulseaudio looks like a nightmare11:10
tristanbut lets make dri work first :)11:10
juergbii don't like seeing a warning that mali driver is not loaded on non-mali systems11:10
tristanRight, me neither11:10
juergbibut we could start with /dev/dri11:11
tristanSo; currently we already support list-or-dict11:11
juergbiand look into that later11:11
tristanSo, we could have that configurable, whether something is expected or not ?11:11
juergbisure, sounds fine to mark some of those as optional to disable the warnings11:11
juergbior possibly replace the warning with an error if we support that11:11
juergbifor the non-optional cases11:11
tristanright, there is a big fuzzy area though; I think warning is better in this case11:12
tristani.e. with a warning we inform the user that things might not work as the project expects it to11:12
tristanbut we dont disallow them a shell environment entirely11:12
juergbiright, forcing the user to remove mounts in the project.conf sounds bad11:13
tristans/big/bit11:13
juergbisounds fine to me11:13
* tristan still has to deal with cleanup of stuff we add by creating directories11:14
juergbitristan: btw: quick other question, we never store absolute symlinks in artifacts, right?11:14
tristanbut that should close dri11:14
tristanproblems11:14
juergbinice11:14
tristanUmmm, no that is not true; we store whatever symlinks were in the collection directory as is11:15
juergbihm, but we have this function to create relative symlinks out of absolute ones11:15
tristanWe are careful to make them relative while staging, though11:15
juergbii thought we already did this before a commit11:15
tristanso it happens on the other side, not at artifact collection time11:15
juergbiwell, we use the same helper function for both11:15
tristanDo we ?11:16
tristanI think that is something which must have changed at some point then11:16
juergbipossible. we hardlink files from the output directory to a temp directory as commit preparation11:16
tristanAh11:16
juergbiand that recursive hardlinking uses the same process_list function, afaict11:16
tristanThat must have implicitly happened when splitting the artifact into files/ logs/ and meta/11:17
juergbithat's possible11:17
juergbii might need to rely on that for remote execution11:17
juergbido you anticipate any issues with such a restriction?11:17
tristan"All symlinks must be relative" restriction ?11:18
tristanAt all times now, basically ?11:18
juergbipackages are allowed to create absolute symlinks11:18
tristanWell, in the most pedantic sense, we are disallowing users from using absolute symlinks11:19
juergbibut artifacts would never contain absolute symlinks11:19
tristanWhich is a limitation11:19
juergbiwe already essentially did this for staging, though11:19
tristanPackages being allowed to create absolute symlinks I think is besides the point11:19
tristanIf we create a bunch of rpms which create symlinks in their metadata post-inst or whatnot, that is one thing11:20
tristanIf we run rpm inside a build sandbox to generate a firmware; absoluteness of those will be lost in the generated artifact11:20
juergbithere is currently no way to extract an artifact without going through this processing function, though, right? (assuming you don't poke at ostree directly)11:21
tristanIt would be good to "aim" for a day where every possible thing which can be expressed for a given platform/filesystem, can be preserved11:21
juergbiso the difference is not actually user-visible11:21
tristanBut we fall short of this now, of course11:22
tristanTo be honest I dont feel strongly about this today11:23
tristanIt would be nice to eventually get rid of this relative transformation stuff, which is originally there because of the way paths are expressed11:23
tristan(i.e. its marginally possible for buildstream to try to stage stuff onto your host because of symlinks)11:24
tristanthe transformation is a protective measure against that11:24
juergbiyes, this could theoretically be handled while preserving absolute symlinks11:25
tristanIf staging were to be more bullet proof, maybe we should not need this11:25
juergbibut it's a bit cumbersome11:25
tristanAll I'm saying is ideally we should shoot for "perfect" and admit that we are not11:25
tristanHow is this a problem for remote execution btw ?11:26
tristanI mean... if we eventually get around to solving this, we can solve it there too right ?11:26
juergbiyes, i was initially planning to fully support absolute symlinks but for the remote execution API there is a desire for canonical cross-platform support as far as possible11:26
juergbiand absolute symlinks don't necessarily make sense cross-platform11:27
tristanI'm not sure what canonical means in this sense11:27
tristanI mean, does it mean "Support only the common denominator of all possible platforms" ?11:28
tristanthat would not sound sensible11:28
juergbie.g., using / as path separator everywhere11:28
tristanright, that would not work on windows11:28
juergbiactually that does11:28
juergbibut absolute paths on windows don't start with / (or \)11:28
tristanOnly if you're not building native windows stuff with native windows buildstream, right ?11:28
juergbiwindows accepts / as alternative directory separator11:29
tristanOh11:29
juergbi(alternatively, the software could easily translate standardized / to \)11:29
juergbithe binary protocol would accept any kinds of symlinks11:30
juergbihowever, the API might officially only allow relative ones11:30
juergbi(not decided yet, just a proposal for now)11:30
juergbiso if we wanted to use absolute symlinks, we'd either have to get that restriction lifted in the API spec or use a server that allows it anyway11:31
tristanErrm, so the driving reasoning behind this is basically, symlinks on platforms which use paths which dont start with a path separator (e.g. volume letter) would break this canonical way of expressing paths ?11:32
tristanI can understand a strong desire for wanting to express paths in a canonical way11:33
tristanespecially since CAS and all of this is all expressing merkle trees and paths11:33
juergbiyes, that's the point here, you want the same thing to have the same digest11:33
tristanhowever, I have a thought...11:34
juergbiso you also want to reduce differences between platforms11:34
tristanIf you are building something... even on a platform where paths dont start with path separators, maybe absolute paths on those platforms should *still* be represented with a path separator11:34
juergbilike /c/foo?11:35
juergbithat's the mingw/cygwin way, i suppose11:35
tristanI.e.; the "Stuff you are building" is not.... "The layout of volumes on a target machine", it's just a payload11:35
tristanOne might build a bunch of stuff, and flash it to a drive somewhere, where it will appear as D:\...11:35
tristanor somewhere else as C:\...11:35
juergbii think in practice absolute paths should almost never matter at all for build actions on Windows (symlinks themselves are much rarer on Windows but let's ignore that)11:37
juergbias there aren't really absolute standard directories there11:37
tristanWell11:38
juergbiit would definitely also be an option to reject absolute symlinks just for Windows but allow them for POSIX11:38
tristanI wonder if "build activities" is different from "the output of the build"11:38
tristanWe are treating this as the same now11:38
tristanBut making concessions about how things will be staged, and storing the full output data, could eventually become different11:39
* tristan didnt word that very well11:39
tristanI.e. on windows, lets say a build tries to create a symlink to the absolute path E:\domain\config ... it's sort of at their on risk, during the build; it would be staged as a relative symlink, and ensured to be restored if necessary in the artifact ?11:41
tristancross-volume relative paths would however seem impossible, on windows11:42
skullmanthe last time I had a poke at creating volumes in windows there was the option of it not having a drive letter and being mounted on top of another drive's path11:43
skullmanI don't know if it would permit a symlink out of it in that form.11:43
tristanso possible, but not cross drive-letter11:43
* tristan also doesnt know about windows rules for symlinks11:44
juergbithey only got introduced in Vista or so, iirc11:45
juergbidon't know how much they're actually used11:45
juergbiok, so one way to look at it would be to say that absolute symlinks are allowed if the Action's input root directory is mapped to the filesystem root (of a sandbox as in our case)11:46
juergbias in that scenario, / would still not escape the specified input directory11:46
juergbiand this might even work on Windows in the case that you have a virtual drive letter for the sandbox or something like that11:46
juergbi(no idea how this is typically handled)11:47
juergbibut including symlinks to other drive letters doesn't make sense to me11:47
tristanit doesnt make sense to me in the context of a build11:47
tristanit makes sense to me in the context of a user11:47
juergbisure, but we're talking about builds here11:48
tristan(i.e. I have symlinks from my home dir to a huge volume in /store, that's typical)11:48
juergbibuild output should pretty much never depend on user config11:48
tristanright :)11:48
juergbiok, i'll try to make a case for this to keep the option open for us to preserve absolute symlinks11:49
juergbibtw: extended permissions are also not supported but as buildstream anyway doesn't support this, i'm not planning on writing a proposal for this at this point11:49
tristanI also think that, we probably never want to stray outside of filesystem data; drive layouting sounds to me to be out of scope for BuildStream11:49
tristanWhen we do know about that at all, we are editing a bootable image, but technically we're just creating a file, which happens to be a partitioned image11:50
tristanextended permissions probably need to be carried in an abstract way by CAS, and applied in filesystem/platform specific ways in remote workers11:51
tristannot immensely concerned about this right now, though11:52
juergbisame here. given the issues we see in #38, i'm tending towards handling extended permissions via public data. either manually (which is beneficial to have clear overview of what packages have setuid root, e.g.) or with a LD_PRELOAD helper that generates the public data11:56
jmacWhat do you mean by public data?12:04
tristanjmac, public: in an element12:06
tristanhttp://buildstream.gitlab.io/buildstream/format.html#public12:06
jmacOh, I see12:07
gitlab-br-botbuildstream: merge request (tristan/shell-mounts->master: Adding new --mount option to bst shell.) #302 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/30212:57
tristanjuergbi, I13:00
tristangah13:00
tristanI'm tempted to just allow directories in `host-files`, rather than adding a separate key13:01
tristanit's trivial though, any preference ?13:01
* tristan thinks project.conf files read better without that separation13:01
gitlab-br-botbuildstream: merge request (214-we-need-a-way-to-make-elements-depend-on-a-subset-of-an-element-s-output->master: Resolve "We need a way to make elements depend on a subset of an element's output") #284 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28413:01
tristanI suppose, since we're in an unstable cycle we *could* change `host-files` for `host-paths`, but the existing name seems ok for directories13:03
tristanif you share a directory, it's probably because you want to have access to the files inside it13:04
*** tristan has quit IRC13:12
*** tristan has joined #buildstream13:18
juergbii don't mind too much either way13:22
tristancool, I'm going to anyway add a note to the docs that one should not specify a location where user's important data is expected to be found13:27
tristanlike a /home dir13:27
tristanin either case, it's an important note13:27
gitlab-br-botbuildstream: merge request (214-we-need-a-way-to-make-elements-depend-on-a-subset-of-an-element-s-output->master: Resolve "We need a way to make elements depend on a subset of an element's output") #284 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28413:28
juergbiyes, that makes sense13:32
*** xjuan has joined #buildstream14:14
*** lantw44 has quit IRC14:26
*** lantw44 has joined #buildstream14:30
gitlab-br-botbuildstream: merge request (239-use-pylint-for-linting->master: WIP: Resolve "Use pylint for linting") #283 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28314:32
jennis_Would someone be able to review !283? skullman?14:33
skullmanI'll have a glance through while CI is running14:34
jennis_ok14:34
jennis_thanks14:34
skullmanjennis_: lint fails from whitespace and "too-many-ancestors"14:55
jennis_yes I expected that to happen14:55
jennis_tlater, has some more "strict" linting in the CI tests so I will work off the log output from that and make those corrections14:56
*** mcatanzaro has joined #buildstream15:07
gitlab-br-botbuildstream: merge request (jmac/configurable-logging->master: Configurable log line formatting) #282 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28215:13
juergbijmac: i think it looks good now. i was just wondering whether we should use elapsed_us or elapsed-us. i don't think we use any underscores in other format variables so far but - is used for workspace-dirs in the element format16:14
juergbiis _ instead of - intentional because it's a unit?16:14
juergbiif we say we want to use _ for units that's fine with me. but if it was an unintentional difference to existing variables, we should strive for consistency16:15
jmacoffs. No, not intentional, just my brain is frozen today16:16
jmacI'll change it16:17
*** dominic has quit IRC16:18
gitlab-br-botbuildstream: merge request (jmac/configurable-logging->master: Configurable log line formatting) #282 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28216:19
gitlab-br-botbuildstream: merge request (239-use-pylint-for-linting->master: WIP: Resolve "Use pylint for linting") #283 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28316:20
jennis_skullman ^^ I think that should pass the linting pipeline, let's wait and see!16:21
gitlab-br-botbuildstream: merge request (jmac/configurable-logging->master: Configurable log line formatting) #282 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/28217:08
jmacwoop17:09
jennis_nice one17:09
gitlab-br-botbuildstream: merge request (214-we-need-a-way-to-make-elements-depend-on-a-subset-of-an-element-s-output->master: Resolve "We need a way to make elements depend on a subset of an element's output") #284 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28417:11
jennis_my MR failed the pipeline because of 1 space :@17:11
gitlab-br-botbuildstream: issue #277 ("workspaces.yaml has no versioning mechanism") changed state ("opened") https://gitlab.com/BuildStream/buildstream/issues/27717:19
gitlab-br-botbuildstream: merge request (214-we-need-a-way-to-make-elements-depend-on-a-subset-of-an-element-s-output->master: Resolve "We need a way to make elements depend on a subset of an element's output") #284 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28417:27
*** jonathanmaw has quit IRC17:28
*** noisecell has quit IRC17:28
gitlab-br-botbuildstream: merge request (239-use-pylint-for-linting->master: WIP: Resolve "Use pylint for linting") #283 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/28317:32
gitlab-br-botbuildstream: issue #214 ("We need a way to make elements depend on a subset of an element's output") changed state ("closed") https://gitlab.com/BuildStream/buildstream/issues/21417:51
gitlab-br-botbuildstream: merge request (214-we-need-a-way-to-make-elements-depend-on-a-subset-of-an-element-s-output->master: Resolve "We need a way to make elements depend on a subset of an element's output") #284 changed state ("merged"): https://gitlab.com/BuildStream/buildstream/merge_requests/28417:51
*** Prince781 has joined #buildstream17:52
*** Prince781 has quit IRC18:06
*** Prince781 has joined #buildstream18:07
*** valentind has joined #buildstream18:45
*** Prince781 has quit IRC18:46
*** Prince781 has joined #buildstream18:57
*** Prince781 has quit IRC19:55
*** xjuan has quit IRC19:56
*** Prince781 has joined #buildstream19:56
*** Prince781 has quit IRC20:13
gitlab-br-botbuildstream: merge request (juerg/git-describe->master: git.py: Output version key with git describe output) #291 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/29120:34
*** tristan has quit IRC20:37
*** Prince781 has joined #buildstream21:28
gitlab-br-botbuildstream: merge request (juerg/git-track-tags->master: git.py: Support tracking annotated tags in a branch) #303 changed state ("opened"): https://gitlab.com/BuildStream/buildstream/merge_requests/30321:30
*** Prince781 has quit IRC21:35
*** aday has quit IRC21:46
*** aday has joined #buildstream21:46
*** aday has quit IRC21:50
*** slaf has quit IRC22:26
*** slaf has joined #buildstream22:31
*** xjuan has joined #buildstream22:32
*** valentind has quit IRC22:48
*** xjuan has quit IRC23:06
*** Prince781 has joined #buildstream23:19

Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!