| *** tristan has joined #buildstream | 06:04 | |
| *** ChanServ sets mode: +o tristan | 06:04 | |
| benschubert | Morning! tristan, juergbi I banded my head this weekend on this umask error, and I don't really have a solution in a multi-threaded environment. | 07:38 |
|---|---|---|
| benschubert | One thing I could imagine is to set the umask at the scheduler level and run the entire time with it? Otherwise... we might need to set the permissions manually / in a subprocess :/ | 07:38 |
| tristan | I'm not sure I understand what the umask problem is | 07:39 |
| benschubert | ah let me show you, one sec | 07:39 |
| tristan | I discovered last week that we lose writable permission when we stage files in CAS and then later stage those CAS files to a FileBasedDirectory, but I think this is different | 07:40 |
| benschubert | https://gitlab.com/BuildStream/buildstream/-/jobs/741315891#L1823 | 07:41 |
| benschubert | we use umask() calls to have the permissions done correctly in that | 07:41 |
| benschubert | and umask() is set at a process level, so per-threads context managers are stepping on each other and removing it before it should be done | 07:42 |
| benschubert | and that also means we actually have no idea of what else is impacted | 07:42 |
| tristan | So | 07:48 |
| tristan | All builds happen in buildbox right ? | 07:48 |
| tristan | But, we do the staging of files ? | 07:48 |
| tristan | we stage sources, thats the crux of it, even if the builds happen in buildbox, we stage the sources | 07:49 |
| tristan | the main crux of the matter is that when sources get staged (essentially input of everything is there), we need to use a deterministic umask to do so | 07:49 |
| tristan | As buildbox and RE gets enhanced over time, it will more and more respect permission bits so that remote or local workers use exactly the permissions we used to initially store files in CAS | 07:50 |
| tristan | So we need to make sure that we continue to store files in CAS with deterministic permissions | 07:51 |
| tristan | benschubert, taking a step back from this, the only reason we use utils._deterministic_umask() is for this: getting the permissions of files created during Source.stage() to be deterministic | 07:52 |
| tristan | benschubert, I think that things have changed in such a way that when we stage sources, we put them into cas, and then we later tell buildbox to stage those sources and build (intermediate source cache) - so it would be possible to change the policy | 07:53 |
| benschubert | so: | 07:53 |
| benschubert | 1) Could we get the buildbox-casd to get which permissions it should use on staging? | 07:53 |
| benschubert | 2) Are we fine with setting umask at the scheduler level? Or will that break things | 07:53 |
| tristan | such that we ensure determinism using Directory APIs when checking in staged sources into the source cache, instead of when asking the Source implementation to stage files into a temp dir | 07:54 |
| benschubert | I think that would be the best and most efficient way indeed | 07:55 |
| tristan | Asides from this, we want to have the inherited umask in effect | 07:55 |
| tristan | i.e. when we do things like `bst workspace open` or `bst source checkout` or `bst artifact checkout` | 07:55 |
| tristan | in those cases we want host umask | 07:55 |
| benschubert | oh wait, the umask is only at stage time, not ingestion time, right | 07:56 |
| benschubert | so a global version doesn't work | 07:56 |
| tristan | I don't completely understand what is wrong with test_pull_access_rights() | 07:56 |
| benschubert | So delegating this to the DirectoryAPI and have it done in casd would probably be easiest for us | 07:56 |
| benschubert | I need to dig more into it still, I am not 100% sure either | 07:57 |
| tristan | benschubert, I could use a review of https://gitlab.com/BuildStream/buildstream/-/merge_requests/2075 btw | 08:01 |
| tristan | it's a gift for you :) | 08:01 |
| benschubert | o/ Thanks ! | 08:02 |
| benschubert | I'm still running around a lot today/tomorrow, will try to do that before then, but EOW will be better :D | 08:02 |
| benschubert | Also, I saw you merged the fix for the logging? Did you manage to add a test? Or is there a way I can know if I broke it? | 08:03 |
| tristan | it's quite a simple patch, and you requested it | 08:03 |
| tristan | No, I merged a separate simple fix, I still have that branch open | 08:03 |
| tristan | 2075 is dependency filename allowed to be a list | 08:03 |
| benschubert | great, so that works for the 'config' key too right? That would be my first comment if you could add an example with 'location' for example? | 08:04 |
| benschubert | this is going to save people hours of typing :P | 08:04 |
| tristan | Its tested with config and its tested with junction | 08:04 |
| tristan | and it will "just work" if you use it with build-depends, runtime-depends or depends | 08:05 |
| benschubert | right, I meant if we could also have an example in the docs about the 'config' part, I'm not sure it will be straighforward when reading? | 08:07 |
| tristan | Oh, I also have this: https://gitlab.com/BuildStream/bst-plugins-container/-/merge_requests/75, I don't know who wants to review that | 08:07 |
| tristan | maybe I should "just merge it" if nobody wants to review container | 08:07 |
| benschubert | oh thanks! chandan and I are definitely interested in that one | 08:07 |
| benschubert | I'll ensure you get a review soon :) | 08:07 |
| tristan | I put an example in the docs, but not a specific one for config no | 08:07 |
| tristan | I think the docs are pretty clear the way it is though | 08:08 |
| tristan | basically it says you can use a list for the filenames in order to declare multiple dependencies with common attributes | 08:08 |
| benschubert | Ok, fair. I guess I'm biased about the docs since I've spent a lot of time reading ansible's docs and they have tons of examples everytime :) | 08:08 |
| tristan | Yeah I would not want to go that route | 08:09 |
| tristan | I would like to have many examples, separate from the terse reference docs | 08:09 |
| *** toscalix has joined #buildstream | 08:23 | |
| juergbi | benschubert: all staging in the buildstream process as preparation for assemble is now purely virtual. i.e., umask shouldn't affect anything at all (I'm wondering whether we already ensure that spawned subprocesses have a deterministic umask but the thread-based scheduler shouldn't be a problem for that) | 08:24 |
| juergbi | I think the test is rather checking whether umask is respected for bst artifact checkout, which should be outside the scheduler | 08:25 |
| benschubert | juergbi: oh :/ I would not have changed that part | 08:25 |
| juergbi | have to take a closer look what's going on | 08:25 |
| benschubert | Ok, I'll try to look this weekend then :/ thanks! | 08:25 |
| *** phildawson has joined #buildstream | 08:32 | |
| *** toscalix has quit IRC | 08:34 | |
| *** tristan has quit IRC | 08:49 | |
| *** santi has joined #buildstream | 08:51 | |
| *** chipb has quit IRC | 09:00 | |
| *** chipb has joined #buildstream | 09:00 | |
| juergbi | benschubert: !2074 fixes the pull_done() issue we've discussed in context of the thread-based scheduler. still have to fix up a test race condition, though | 09:07 |
| gitlab-br-bot | MR !2074: Artifact pull code improvements https://gitlab.com/BuildStream/buildstream/-/merge_requests/2074 | 09:07 |
| benschubert | juergbi: right, I still have to add the asserts in my branch too | 09:08 |
| benschubert | I hope to have some time EOW for that | 09:08 |
| gitlab-br-bot | BenjaminSchubert approved MR !2075 (tristan/dependency-multi-filename->master: Allow specifying multiple filenames in a dependency) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2075 | 09:14 |
| gitlab-br-bot | BenjaminSchubert approved MR !2074 (juerg/artifact-pull->master: Artifact pull code improvements) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2074 | 09:21 |
| benschubert | oh the bot is alive again! Thanks whoever did that :D | 09:21 |
| *** tristan has joined #buildstream | 09:21 | |
| *** ChanServ sets mode: +o tristan | 09:21 | |
| *** tomaz has joined #buildstream | 09:35 | |
| *** tomaz has quit IRC | 10:07 | |
| *** tomaz has joined #buildstream | 10:18 | |
| gitlab-br-bot | marge-bot123 merged MR !2075 (tristan/dependency-multi-filename->master: Allow specifying multiple filenames in a dependency) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2075 | 11:18 |
| douglaswinship | Here's a weird one. I'm getting an exception thrown when I try to create a workspace for a Freedesktop-sdk element: | 11:24 |
| douglaswinship | "[--:--:--][][] BUG Primary URL marked twice with different URLs" | 11:24 |
| douglaswinship | But I ONLY get it when i do it inside the freedesktop-sdk docker image | 11:24 |
| douglaswinship | when I create a workspace on my own system, outside the container, it works fine. | 11:25 |
| douglaswinship | The problem happens in CI on freedesktop-sdk too. (Where, naturally, it's running the freedesktop-sdk docker image). | 11:26 |
| douglaswinship | So it's defintiely something about the docker image that does it. | 11:26 |
| douglaswinship | What does it even mean when a url gets marked as 'primary' anyway? What part of the system is recording 'primary' urls? | 11:27 |
| douglaswinship | (i'm running buildstream 1.4.3, in each scenario) | 11:29 |
| coldtom | that rings a bell.... aha https://gitlab.com/BuildStream/buildstream/-/issues/757 | 11:33 |
| coldtom | i thought someone had patched that, but perhaps not | 11:33 |
| douglaswinship | coldtom: that sounds close | 11:34 |
| douglaswinship | but it's still strange that it only happens in the docker image | 11:35 |
| douglaswinship | i suppose the patch would be in the git_tag plugin? | 11:35 |
| douglaswinship | Perhaps the docker image isn't using the version with the fix. | 11:35 |
| douglaswinship | though, that would be strange | 11:36 |
| coldtom | https://gitlab.com/BuildStream/bst-external/-/merge_requests/118 yes! | 11:36 |
| * coldtom forgot that it might be in bst-external | 11:36 | |
| douglaswinship | coldtom: thanks. Big help. | 11:41 |
| douglaswinship | docker image needs updating with the newest bst-external | 11:41 |
| douglaswinship | i'll track and do an MR | 11:42 |
| douglaswinship | Okay, important question: is the master branch of bst-external compatible with bst 1.4.3? | 12:02 |
| douglaswinship | And If not, do we have any record of which bst-external tags are compatible with which buildstream versions? | 12:02 |
| coldtom | it should be compatible, the plugin API should be stable for all 1.x | 12:04 |
| douglaswinship | interesting. | 12:05 |
| douglaswinship | The freedesktop-sdk docker images have two different versions of bst-external.bst, one for bst 1.4, and one for bst 1.6 | 12:05 |
| douglaswinship | That made me think there was a compatibility issue | 12:05 |
| douglaswinship | Then I noticed that they're both pointed at the same commmit of bst-external anyway, and I started getting confused | 12:06 |
| douglaswinship | Oh well, I'll just try an MR updating them both to the current head of master and see what happens. | 12:06 |
| douglaswinship | oh, no. I see what's going on now. | 12:11 |
| *** tristan has quit IRC | 12:38 | |
| *** tristan has joined #buildstream | 13:25 | |
| *** ChanServ sets mode: +o tristan | 13:25 | |
| *** tristan has quit IRC | 13:34 | |
| gitlab-br-bot | marge-bot123 merged MR !2074 (juerg/artifact-pull->master: Artifact pull code improvements) on buildstream https://gitlab.com/BuildStream/buildstream/-/merge_requests/2074 | 14:19 |
| *** santi has quit IRC | 17:35 | |
| *** benschubert has quit IRC | 19:49 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!