| wieczorek1990 | Updated my plugs | 01:24 |
|---|---|---|
| wieczorek1990 | https://forum.snapcraft.io/t/please-allow-use-of-personal-files-for-gitl-was-classic-confinement-for-gitl/9420/30 | 01:24 |
| df00z | Is there a reason snapcraft seems to pass "-prefix=" with the autotools plugin? | 02:29 |
| df00z | it seems to hork up a lot of builds | 02:29 |
| df00z | unless /usr is specified | 02:29 |
| hunterk | I'm trying to add a scriptlet to symlink a library in my snap, but it doesn't seem to be doing anything. That is, I don't see any trace of it in the build log on snapcraft: https://github.com/libretro/retroarch-snap/blob/master/snapcraft.yaml#L20 | 02:36 |
| hunterk | can anyone tell me if I formatted that properly in the yaml? | 02:36 |
| df00z | sorry, i am not quite there yet, havent done scripts | 04:16 |
| mborzecki | morning | 05:35 |
| === mwhudson_ is now known as mwhudson | ||
| === harrisj_ is now known as harrisj | ||
| mborzecki | duh, clearlinux is weird, it says it supports cloud-init, but makes no use of it when i attach a disk with vfat labeled ucidata | 06:35 |
| mborzecki | turns out they have their own implementation https://github.com/clearlinux/micro-config-drive/ that takes config-2 openstack style disks only :/ | 06:35 |
| === pstolowski|afk is now known as pstolowski | ||
| pstolowski | morning | 07:06 |
| mborzecki | pstolowski: hey | 07:11 |
| zyga | Good morning | 07:14 |
| mborzecki | zyga: hey hey | 07:16 |
| zyga | mborzecki: can you re-review https://github.com/snapcore/snapd/pull/7026 | 07:30 |
| zyga | mborzecki: I have some more patches but per agreement with jdstrand those will go to another PR | 07:30 |
| zyga | mborzecki: those add a few more unit tests and allow the memory buffer to be capped | 07:30 |
| zyga | mborzecki: with those test coverage for the new code is 100% | 07:30 |
| mborzecki | zyga: ok, looking | 07:31 |
| zyga | thanks! | 07:32 |
| zyga | pstolowski: how are you? :) | 07:32 |
| zyga | weekend coming up, weather is slightly saner (at least in poland, here's it is going to be much hotter) | 07:32 |
| pstolowski | zyga: hey! i'm good, and weather is sane, yes | 07:33 |
| zyga | today is going to be blazing hot, cannot wait till 10:00 when the library opens | 07:44 |
| ackk | hi, to confirm, the "refresh" hook is called both on first install and every time a snap is updated right? | 08:16 |
| * zyga breakfast | 08:17 | |
| zyga | pstolowski: ^ | 08:17 |
| pstolowski | ackk: no. there is no "refresh" hook, there are "pre-refresh" and "post-refresh" hooks, they are only run if snap is already installed and is refreshed, not on initial install. see https://docs.snapcraft.io/supported-snap-hooks | 08:21 |
| ackk | pstolowski, ah, thanks | 08:22 |
| ackk | pstolowski, shouldn't post-refresh run when you first install the snap? | 08:36 |
| ackk | ah, it doesn't seem so :/ | 08:39 |
| zyga | ackk: no, it runs after refresh, which is not the initial install | 08:39 |
| pstolowski | ackk: the idea with pre- and post- is to allow snap to do something with "old" snap resources before switching to new snap, and then afterwards in the new snap | 08:39 |
| ackk | This hook is executed for the newly installed snap, | 08:39 |
| ackk | pstolowski, so doc is wrong I think ^ | 08:40 |
| pstolowski | ackk: are you sure you didn't have old revision around? i checked the code before actually looking up the doc | 08:40 |
| ackk | pstolowski, if I do have an old revision, it is run | 08:40 |
| zyga | ackk: are you trying or installing? | 08:40 |
| zyga | there's a bug with snap try in this case | 08:41 |
| ackk | pstolowski, I'm saying what I see matches what you said (only run on refresh, not on new install), but the doc says otherwise | 08:41 |
| ackk | zyga, try | 08:41 |
| zyga | in try case it's broken because it's all one virtual revision | 08:41 |
| pstolowski | ah, snap try | 08:41 |
| ackk | zyga, so, if I want a script to be run on new install and on subsequent refresh, I can use install + post-refresh right? and symlink one to the other | 08:41 |
| zyga | ackk: or configure | 08:42 |
| ackk | zyga, yeah that's what i have at the moment but I wanted to move away from that as it might break things if you run snap set | 08:42 |
| zyga | ok | 08:42 |
| ackk | also, it would run more times that it needs | 08:42 |
| ackk | zyga, is symlinking allowed? | 08:42 |
| zyga | I suspect so but I don't believe we test for it | 08:43 |
| ackk | I'll try | 08:44 |
| pstolowski | zyga: thanks for pointing out 'snap try', i wouldn't realize this could be a factor | 08:44 |
| zyga | pstolowski: we should warn if refresh hooks are present in snap try | 08:45 |
| zyga | it probably would be easy now | 08:45 |
| pstolowski | indeed | 08:45 |
| ackk | zyga, you mean post-refresh is broken if you use snap try? | 08:46 |
| ackk | why is it one virtual revision? if you upgrade via snap try you get x2,x3... | 08:46 |
| zyga | ackk: if you snap try snapd will think that you already have the hook | 08:50 |
| zyga | ackk: because it's one actual directory it is always looking at | 08:50 |
| zyga | ackk: I suggest that you snap pack and actually install to test this code | 08:50 |
| zyga | snap try cannot be used for that reliably | 08:50 |
| ackk | zyga, ok, thanks | 08:51 |
| ackk | zyga, I'm mostly using snap try because the squashfuse bug makes it really slow to use maas in a snap | 08:51 |
| zyga | bug? | 08:52 |
| zyga | you can compress with gz | 08:52 |
| ackk | it uses 100% cpu | 08:52 |
| zyga | it will be fast | 08:52 |
| zyga | it's not a bug, it's just expensive compression :) | 08:52 |
| ackk | zyga, uhm, squashfuse constantly using 100%cpu sounds like a bug | 08:52 |
| zyga | ah | 08:52 |
| zyga | sorry, I misread | 08:52 |
| zyga | squashfuse | 08:52 |
| zyga | it's not a bug, squashfuse is just really this slow | 08:52 |
| ackk | zyga, https://bugs.launchpad.net/snapd/+bug/1817276 | 08:52 |
| zyga | lots of overhead for access | 08:53 |
| ackk | zyga, it only happens inside containers | 08:53 |
| ackk | zyga, it makes it moslty unusable | 08:53 |
| zyga | because we only use squashfuse inside containers | 08:53 |
| zyga | fuse is just slwo | 08:53 |
| zyga | *slow | 08:53 |
| zyga | we'd need a new kernel implementation that is not fuse to be any faster | 08:54 |
| zyga | perhaps something could be improved in the current one but in general fuse is just really slow | 08:54 |
| Chipaca | good morning and greetings from The Sprint | 09:09 |
| pstolowski | hey Chipaca | 09:10 |
| Chipaca | zyga: what's the status of #6891? | 09:11 |
| Chipaca | pstolowski: 'sup :-) | 09:11 |
| pstolowski | Chipaca: how is the sprint? | 09:11 |
| zyga | Chipaca: hey, looking | 09:11 |
| Chipaca | pstolowski: I'm exhausted and I've only been here 24 hours | 09:11 |
| Chipaca | pstolowski: how mvo, pedronis and niemeyer cope with it I don't know | 09:12 |
| zyga | Chipaca: the status is that there's one undesired consequence of the fix that is present on core16 only, it needs to be debugged but debugging is painful (slow) because of core nature | 09:12 |
| zyga | Chipaca: I will task switch to it as soon as https://github.com/snapcore/snapd/pull/7026 is merged | 09:12 |
| zyga | Chipaca: the error is that on core16, the new test measures duplicated /snap/name/revision entries | 09:13 |
| zyga | probably a consequence of something else propagating | 09:13 |
| zyga | but I don't understand it yet | 09:13 |
| zyga | Chipaca: how is the sprint so far? | 09:13 |
| ackk | zyga, fwiw it seems a bit weird that snapd only prints "Running <hooname> hook if present" only when the hook actually exists. I mean why the "if present"? | 09:19 |
| ackk | zyga, FTR symlinking doesn't work :( | 09:20 |
| zyga | ackk: bug, it used to always do that | 09:20 |
| zyga | pstolowski: ^ | 09:20 |
| zyga | the message is out of date | 09:20 |
| pstolowski | right, we now optimize hooks out, message needs updating | 09:21 |
| ackk | ah actually my symlink test wasn't accurate, need to test again | 09:25 |
| ackk | oh actually it seems symlinks get converted, nice | 09:33 |
| === ricab is now known as ricab|bbl | ||
| * ackk stares at squashfuse running at 100% since 5m | 10:02 | |
| Chipaca | zyga, mborzecki, either of you feel like taking a look at os-release for clearlinux? wrt https://forum.snapcraft.io/t/snapd-on-clearlinux/12048?u=chipaca | 10:12 |
| mborzecki | Chipaca: tried booting clear linux image in the morning, was a funny experience | 10:14 |
| Chipaca | mborzecki: glad you're having fun | 10:14 |
| Chipaca | :-รพ | 10:14 |
| Chipaca | mborzecki: anything and everything clearlinux, I blame xnox for it | 10:15 |
| mborzecki | hm maybe i should at least paste os-release in that topic | 10:15 |
| Chipaca | mborzecki: zyga has a zoo | 10:15 |
| Chipaca | mborzecki: https://github.com/zyga/os-release-zoo | 10:16 |
| Chipaca | zyga: archived? :-( | 10:16 |
| zyga | Chipaca: I have a clearlinux VM | 10:18 |
| zyga | Chipaca: tried building snapd there | 10:18 |
| zyga | Chipaca: moved to gitlab.com/zygoon/os-release-zoo | 10:19 |
| zyga | since I was curious of how the grass looks like there | 10:19 |
| Chipaca | it looks clear, duh | 10:19 |
| ogra | and ? | 10:19 |
| ogra | was it greener ? | 10:19 |
| zyga | ogra: it was very fast indeed | 10:22 |
| zyga | I was surprised to see a brand new package manager | 10:22 |
| zyga | not related to anything else | 10:22 |
| zyga | and a matching package format aswell | 10:22 |
| wieczorek1990 | Can I get some love here: https://forum.snapcraft.io/t/please-allow-use-of-personal-files-for-gitl-was-classic-confinement-for-gitl/9420/32?u=wieczorek1990 | 11:28 |
| xnox | zyga: i love your zoo | 11:40 |
| zyga | xnox: pleasure, contributions welcome | 11:40 |
| zyga | I should be more active about adding stuff I have access to there | 11:40 |
| ogra | ppisati, where did System.map-*, abi-* and config-* go ? | 11:53 |
| ogra | wget -O- -q http://ports.ubuntu.com/ubuntu-ports/pool/main/l/linux/linux-image-4.4.0-154-generic_4.4.0-154.181_armhf.deb | dpkg -c - | grep boot | 11:53 |
| ogra | drwxr-xr-x root/root 0 2019-06-25 11:19 ./boot/ | 11:53 |
| ogra | -rw------- root/root 7310208 2019-06-25 11:19 ./boot/vmlinuz-4.4.0-154-generic | 11:53 |
| ogra | seems they are not in the deb anymore | 11:53 |
| ppisati | ogra: :O | 11:54 |
| ppisati | ogra: check the linux-modules-* package | 11:57 |
| ogra | ah, there are config-* and System.map-* .... but abi-* is still missing ... did we drop that ? | 11:58 |
| zyga | mborzecki: could you have another look at https://github.com/snapcore/snapd/pull/7026 please | 12:02 |
| === ricab|bbl is now known as ricab | ||
| zyga | mborzecki: if you are interested, on top I have one patch https://github.com/zyga/snapd/commit/eacf3860affb61f2ad6f8cd40bf5e6e56bb7bf91 that I decided to postpone until after this lands | 12:10 |
| * cachio afk | 12:11 | |
| mborzecki | zyga: nice, though jdstrand was ok with getline() :) | 12:12 |
| zyga | yeah but then I played and fed /dev/empty into it | 12:12 |
| zyga | this used all ram | 12:12 |
| zyga | so ... | 12:12 |
| zyga | here it is :) | 12:12 |
| zyga | I'll propose it as soon as this lands | 12:12 |
| mborzecki | haha | 12:12 |
| zyga | also need to propose a patch to get easy way to run lcov | 12:12 |
| zyga | but first, small break | 12:13 |
| zyga | then back to https://github.com/snapcore/snapd/pull/6891 | 12:13 |
| mborzecki | zyga: oh, looks like glib is using lcov too | 12:18 |
| mborzecki | zyga: i'd check of clang/llvm has something | 12:20 |
| zyga | honestly anything that works is ok | 12:24 |
| mborzecki | zyga: hahha, given that it's C land, you can put the bar too high :) | 12:28 |
| mborzecki | so dropping snap.ReadGadgetInfo() saves ~20k on snap binary, worth it? | 12:33 |
| zyga | mborzecki: yeah | 12:39 |
| === ricab_ is now known as ricab | ||
| zyga | trying to join... | 13:02 |
| jdstrand | zyga (cc mborzecki): wow, you added a lot of code to get rid of the variable list. I feel bad cause of all the code you wrote, but jeez, why can't we just do a simple loop and search like I originally suggested? | 13:15 |
| jdstrand | I mean, this is setuid code, parsing teensy files that we control | 13:16 |
| * setuid looks around | 13:16 | |
| jdstrand | lets regulate the input and be very strict (skipping if malformed/whatever) and keep the code simple without state | 13:17 |
| jdstrand | mborzecki: am I wrong in my assessment? | 13:18 |
| zyga | jdstrand: yeah but also made it much simpler (O(N) instead of O(N*N)) and much more testable | 13:18 |
| zyga | jdstrand: this will also allow me to replace the os-release parser with the same tested code | 13:19 |
| zyga | jdstrand: more is not necessarily worse (especially that the public code surface is smaller and even easier to use) | 13:19 |
| jdstrand | zyga: I think you mean s/simpler/faster/ | 13:19 |
| zyga | no, it is simpler to reason IMO | 13:19 |
| zyga | the logic in each function is more straightforward than before | 13:19 |
| jdstrand | I'm trying to reason about it while reviewing it and found that not to be the case | 13:20 |
| zyga | where it was all in one more complex function | 13:20 |
| zyga | the private function does a pass over the file | 13:20 |
| zyga | the public function calls the private function with a helper callback that extracts the single key we are looking for | 13:20 |
| zyga | everything else is tests | 13:20 |
| zyga | jdstrand: the structs avoid very long argument lists | 13:21 |
| jdstrand | I understand the one loop pass. I know why you are doing it. I'm saying it feels like a lot more than is required to extract a key/value pair from a file | 13:21 |
| jdstrand | zyga: I am not saying I don't understand the code. I am saying it seems like more than is needed. I'd like mborzecki's input as well | 13:22 |
| Chipaca | what key/value pair from what file? | 13:23 |
| zyga | ack, though I'd rather not rewrite it again :) | 13:23 |
| Chipaca | :) | 13:23 |
| zyga | Chipaca: key=value from a file we write | 13:23 |
| zyga | Chipaca: but also keeping it open to drop the custom code for /etc/os-release parser and use this one instead | 13:24 |
| jdstrand | zyga: I know, which is why I said I feel bad... | 13:24 |
| jdstrand | Chipaca: https://github.com/snapcore/snapd/pull/7026 | 13:24 |
| zyga | jdstrand: can you clarify if you feel bad because of how the code looks like or because of the initial suggestion, sorry I didn't get that | 13:25 |
| mborzecki | jdstrand: agree that it's bit more more than needed, but afaiu the idea is to reuse this elsewhere | 13:25 |
| Chipaca | oh in C | 13:25 |
| mborzecki | Chipaca: bleh C :/ | 13:25 |
| jdstrand | zyga: I feel bad for you writing a lot of code that I really want to NAK. I'm trying to figure out if I am beign reasonable | 13:26 |
| Chipaca | and in part of a 800-line PR | 13:26 |
| zyga | ack, I see | 13:26 |
| Chipaca | that touches 23 files in 34 commits | 13:26 |
| zyga | Chipaca: most of that is noise, the main part is just tests and a single .c file | 13:26 |
| * Chipaca steps slowly away from that | 13:26 | |
| zyga | Chipaca: I can move 90% of the files changed to another PR that can land without any impact | 13:26 |
| zyga | jdstrand: on top of that I also wrote a patch to use fixed size buffer | 13:27 |
| zyga | jdstrand: but I didn't include it in this PR as we discussed | 13:27 |
| jdstrand | zyga: again, I didn't care about fgets vs getline. I was just adding to the sidebar commentary | 13:27 |
| zyga | ack | 13:28 |
| * jdstrand takes a deep breath and tries to reread this in different orders to somehow rationalize is is worth it | 13:28 | |
| zyga | jdstrand: please make a decision, I will close and re-do this if necessary | 13:29 |
| jdstrand | uhh... of course? | 13:29 |
| jdstrand | though, if I'm being honest, if I do decide to let it through, I'm going to ask for Michael or Samuele to ack the added complexity | 13:30 |
| Chipaca | zyga: I'd say that anything that touched C should be very far away from anything that is noise | 13:32 |
| zyga | Chipaca: read the PR | 13:32 |
| Chipaca | zyga: I'm feeling particularly ornery today though | 13:32 |
| zyga | Chipaca: it's not noise per se, just trivial required changes to complete the feature | 13:32 |
| zyga | Chipaca: it's noise for the point of view of the discussion | 13:33 |
| zyga | *from | 13:33 |
| zyga | removal of .info files | 13:33 |
| zyga | spread test + data files | 13:33 |
| niemeyer | zyga: I've got a bug on a connected content interface | 13:34 |
| * jdstrand notes he is only talking about sc_infofile_get_key() | 13:34 | |
| jdstrand | and down | 13:35 |
| niemeyer | zyga: Hello btw :) | 13:35 |
| zyga | niemeyer: hey! | 13:35 |
| zyga | niemeyer: can you tell me more please | 13:35 |
| niemeyer | zyga: I've been speaking to people all week live here and suddenly I forgot you were not one of those :p | 13:35 |
| mborzecki | jdstrand: wondering, as a middle ground, we could live without scanner state & callback thing, plus enforce <key>=<value>\n format, but keep the error codes, then https://github.com/snapcore/snapd/pull/7026/files#diff-5dd49f9aa50cb48081dd1039d91e4e9aR243 becomes where value is extracted, wdyt? | 13:35 |
| zyga | I hope it's one of the known ones, but we'll see | 13:35 |
| niemeyer | zyga: The scenario is quite specific, so might be related | 13:35 |
| niemeyer | zyga: I had a local snap unsigned that I developed myself | 13:35 |
| zyga | niemeyer: there's a fix that is close to landing that happened to resolve a number of bugs at once | 13:36 |
| niemeyer | zyga: And then I refreshed into a proper snap from the store | 13:36 |
| * zyga listens | 13:36 | |
| niemeyer | zyga: Which had a new connection that did not exist in my local snap | 13:36 |
| niemeyer | zyga: The connection was established (and is), but the actual content doesn't show up in the content directory | 13:36 |
| zyga | niemeyer: ok, some questions: is this snap using the desktop interfaces by any chance? | 13:37 |
| niemeyer | zyga: This is the gnome platform interface | 13:37 |
| niemeyer | zyga: http://paste.ubuntu.com/p/qDX9jnn9qr/ | 13:37 |
| zyga | niemeyer: if you nsenter -m/run/snapd/ns/$SNAP_NAME.mnt - do you see the directory you were expecting? | 13:37 |
| jdstrand | mborzecki: I liked how the tests read (ie, the error codes) and found that a nice improvement. it is when I started looking at all the structs, caller_state, function pointers, etc that I was like "this is going to take me all day to prove to myself that it is bulletproof) | 13:38 |
| jdstrand | s/)/" | 13:38 |
| zyga | jdstrand: I'm happy you like the error handling, I think it is a good pattern to follow, even if the outcome is die() simply because of how testable that mode is, and that error forwarding also calls die if nobody is to pick up the error on the caller side. | 13:39 |
| niemeyer | zyga: Where do I run thta? | 13:39 |
| jdstrand | mborzecki: not that it would take all day, but having that feeling isn't great when you are looking at setuid file parsing | 13:39 |
| zyga | niemeyer: open a terminal, sudo nsenter -m/run/snapd/ns/foo.mnt # replace foo with the name of the snap | 13:39 |
| zyga | niemeyer: normally on your host | 13:39 |
| niemeyer | zyga: No | 13:39 |
| niemeyer | zyga: It's empty | 13:39 |
| zyga | niemeyer: ok, can you look at /run/snapd/ns/snap.$SNAP_NAME.fstab and see if we claim we made the connection there? | 13:40 |
| zyga | niemeyer: (do you see the mount point represented in that file) | 13:40 |
| niemeyer | zyga: Wait, but something doesn't add up | 13:40 |
| zyga | oh? | 13:40 |
| niemeyer | zyga: I may be running the command incorrectly | 13:40 |
| zyga | niemeyer: nsenter drops you into a shell inside the namespace | 13:40 |
| niemeyer | zyga: I don't have permission to run nsenter as a user on that ns | 13:40 |
| zyga | niemeyer: you have to sudo nsenter from the host | 13:41 |
| niemeyer | zyga: and runnign as root doesn't make sense | 13:41 |
| niemeyer | zyga: Doesn't that mean I'd get into a different namespace than my user would see normally? | 13:41 |
| zyga | niemeyer: it's just a debug tool, it is okay to run it as root | 13:41 |
| zyga | niemeyer: yes, but that's what I'm trying to debug at this moment | 13:41 |
| zyga | niemeyer: I strongly believe you are seeing a bug that is fixed by https://github.com/snapcore/snapd/pull/6891 | 13:41 |
| niemeyer | zyga: Sure, but root never run the snap command | 13:42 |
| zyga | niemeyer: sure but this is not the snap command, we are inspecting intermediate state | 13:42 |
| jdstrand | mborzecki: I'm also worried about future maintenance | 13:42 |
| zyga | niemeyer: if you see the mount point and stuff connected properly in this mount namespace this is the same bug | 13:42 |
| zyga | niemeyer: that is all I'm trying to establish now | 13:42 |
| jdstrand | mborzecki: I would be ok with a middle ground. do you think I am being unreasonable? | 13:42 |
| jdstrand | mborzecki: (in my gut reaction) | 13:43 |
| niemeyer | zyga: Okay, I must be forgetting about some detail related to namespaces.. the namespace needs to be user-specific, and this file path doesn't look user specific | 13:43 |
| niemeyer | Ah, no, it's not user specific | 13:43 |
| zyga | niemeyer: that's correct, this is the non-user-specific namespace; the bug is that because of sharing settings some changes don't propagate to the per-user namespaces | 13:43 |
| niemeyer | That's what I'm forgetting | 13:43 |
| niemeyer | Ack | 13:44 |
| zyga | niemeyer: the PR changes the settings to properly propagate into per-user mount namespaces | 13:44 |
| niemeyer | zyga: So yeah, the content is really missing | 13:44 |
| zyga | niemeyer: we're hoping it will be in 2.40 | 13:44 |
| niemeyer | zyga: Okay, cool | 13:44 |
| niemeyer | zyga: Thanks! | 13:44 |
| niemeyer | zyga: How's spain? | 13:44 |
| zyga | niemeyer: honestly, I'm just a piece of luggage here; I'm working all the time anyway. The kids seem to like it though, recalling memories and seeing friends | 13:45 |
| mborzecki | jdstrand: zyga: probably buggy middle ground? https://paste.ubuntu.com/p/nMVvYKpyQ2/ | 13:45 |
| niemeyer | zyga: Hope you can enjoy the evenings with them a bit at least | 13:45 |
| zyga | mborzecki: that would work but remember that the reason the callback exist is so that the os-release parser can extract ID and other fields that we need to look at without changing this function again | 13:45 |
| zyga | niemeyer: yeah, though evenings are the best moments to work for now, I take mid-day slow (I recall why siesta existed) and try to compensate in the evening | 13:46 |
| zyga | niemeyer: looking forward to a slower weekend though :) | 13:46 |
| jdstrand | mborzecki: that is like all that I was expecting from this PR | 13:47 |
| zyga | niemeyer: https://twitter.com/zygoon/status/1144347521613017106 | 13:47 |
| jdstrand | mborzecki: that verified with tests/etc is inline with what I would prefer. the fact that you loop through the whole file on each run is no big deal now (there is only one thing in the .info file) and isn't a big deal for os-release since it is small as well and one would want to extract like only a key or two anyway | 13:49 |
| mborzecki | zyga: jdstrand: ok, so perhaps we could work with that for now and then look at os-release once the fix lands? | 13:49 |
| zyga | jdstrand: does that mean that the door for the future where this code can be brought back is open or that you'd rather use the simpler implementation and just loop over input several times? | 13:50 |
| kenvandine | jdstrand: is this still blocked? https://github.com/snapcore/snapd/pull/5644 | 13:50 |
| jdstrand | mborzecki: a future optimization for that could be read it into memory and pass that stream into sc_infofile_get_key(). then everything stays clean. if we were talking about large files, that would be different and we'd need to consider something smarter | 13:50 |
| zyga | jdstrand: note, I wrote that version once, it was also nacked | 13:51 |
| zyga | jdstrand: one that read everything into memory and scanned that | 13:51 |
| jdstrand | mborzecki: but that isn't the case (we can even comment "To keep this simple, ..." | 13:51 |
| zyga | jdstrand: I have a feeling this is the 4th attempt at this key=value thing for snap-confine | 13:51 |
| jdstrand | zyga: I did say 'future' there | 13:51 |
| jdstrand | mborzecki, zyga: this PR seems like is is not focused, so yes, I like mborzecki's approach. do something simple and verifiable to fix the 'base changes' bug. then iterate to read os-release. we can land that sooner than later. for os-release, we can argue about the merits of efficiency vs simplicity/etc with all the right people looking at it | 13:53 |
| jdstrand | kenvandine: yes | 13:54 |
| jdstrand | kenvandine: it needs me to grandfather hundreds of snaps. it is still on for this cycle | 13:54 |
| kenvandine | jdstrand: ok, thanks | 13:55 |
| niemeyer | zyga: Nice! | 13:57 |
| niemeyer | zyga: How do I get the namespace to work again? | 13:58 |
| jdstrand | zyga: re open door> part of why we have peer reviews for this code is multiple viewpoints. I am always going to want simplicity in code for auditability, reliability and provability. if this was not setuid, it would be different. there is no obvious need (to me) for the optimizing, even when considering os-release. Michael and Samuele can overule that after hearing all sides | 13:58 |
| zyga | niemeyer: you'd have to discard it, I'm afraid | 13:58 |
| niemeyer | zyga: Already did | 13:58 |
| niemeyer | zyga: New namespace has the same issue | 13:58 |
| zyga | niemeyer: if it doesn't work now it may be some other issue | 13:58 |
| zyga | niemeyer: can you run this from the terminal with SNAP_CONFINE_DEBUG=yes | 13:58 |
| zyga | after discarding it | 13:58 |
| jdstrand | zyga: I'm concerned about-- what happens if the user is able to trick snap-confine into reading a file the user controls, and there is a bug | 13:59 |
| zyga | jdstrand: I share those concerns, I think we are on the same page here. I specifically made sure this robust (even to the point of fixed memory sizes in the follow-up) | 14:00 |
| jdstrand | zyga: I'm not saying the code has a bug. I'm just saying it takes a lot of mental thought to verify it is all ok and that thought will need to be expended every time this part of the code is touched when it lanes | 14:00 |
| jdstrand | lands* | 14:00 |
| niemeyer | zyga: https://pastebin.canonical.com/p/dC2YbSXMsr/ | 14:00 |
| * zyga fetches token | 14:01 | |
| jdstrand | zyga: I know you're being careful! :) you have great checks; it is just so much more than seems required I balked at it | 14:01 |
| zyga | jdstrand: ack | 14:02 |
| jdstrand | zyga: perhaps save it off for now, take the middle ground approach for now and revisit when everyone is back in town? | 14:02 |
| zyga | jdstrand: yeah, I think that's the only way forward | 14:02 |
| * jdstrand notes he is off Tue-Fri next week | 14:02 | |
| zyga | niemeyer: is this after you have discarded? I'm surprised to see "keep" changes there | 14:04 |
| zyga | (keep indicates that an existing entry is reused) | 14:04 |
| niemeyer | zyga: Yep | 14:07 |
| niemeyer | zyga: I assume that to discard I need to umount and rm | 14:07 |
| zyga | niemeyer: how did you discard if I may ask? | 14:07 |
| niemeyer | zyga: Am I missing something? | 14:07 |
| zyga | not quite, you must run the discard tool, it also removes the .fstab files | 14:07 |
| zyga | (those in /run/snapd/ns) | 14:08 |
| niemeyer | Aha | 14:08 |
| zyga | I mean, you must umount and rm stuff in /run/snapd/ns that matches the snap name) | 14:08 |
| niemeyer | There's nothing there | 14:08 |
| niemeyer | Uh, no, I'm lying | 14:08 |
| zyga | nothing in /run/snapd/ns/snap.*.fstab | 14:09 |
| niemeyer | zyga: Yeah, that fixed it | 14:09 |
| niemeyer | zyga: The rm of the fstab, that is | 14:09 |
| zyga | good, I'll look at fixing the propagation ASAP | 14:09 |
| niemeyer | kenvandine: Works, will be playing with it on the flight back home | 14:10 |
| niemeyer | zyga: Thank you! | 14:10 |
| kenvandine | niemeyer: awesome | 14:11 |
| jdstrand | zyga, mborzecki: ok, summarized my thoughts and irc outcome here: https://github.com/snapcore/snapd/pull/7026#pullrequestreview-255757563 | 14:19 |
| zyga | jdstrand: I'm almost done, one sec | 14:19 |
| zyga | I'll push the simplified version | 14:19 |
| zyga | jdstrand: pushed | 14:21 |
| jdstrand | zyga: I'm sorry for the extra work. we all want the same thing ultimately even if we have different opinions on how to get there. thanks for being patient (with all of the reviewers) | 14:21 |
| zyga | jdstrand in the extra test, you mean one that has a trailing newline? | 14:22 |
| zyga | jdstrand: because malformed input test 3 already does that without the newline | 14:23 |
| * zyga adds the extra test just in case | 14:23 | |
| jdstrand | zyga: yes | 14:26 |
| zyga | added now | 14:26 |
| jdstrand | zyga: ie, the file we read in has | 14:26 |
| jdstrand | foo= | 14:26 |
| jdstrand | bar=baz | 14:26 |
| zyga | jdstrand: https://github.com/snapcore/snapd/pull/7026/commits/701b333abbdec24af7eea53f8a1479bb43ff3dc1 is this what you expected? | 14:27 |
| jdstrand | and foo is assigned the empty string | 14:27 |
| jdstrand | (or if you don't want to support empty strings, a test that it isn't allowed) | 14:27 |
| zyga | I want to support empty values | 14:27 |
| zyga | I think it is useful | 14:27 |
| jdstrand | zyga: I do too. then, 'yes' that is what I had in mind, but it isn't malformed | 14:28 |
| zyga | yeah, I think those are "tricky" but not clearly malformed :) | 14:28 |
| zyga | I'll rename those | 14:28 |
| jdstrand | char empty_value_ok = "key=\n"; | 14:29 |
| jdstrand | ? | 14:29 |
| jdstrand | but choose whatever you want | 14:29 |
| zyga | pushed | 14:30 |
| zyga | I think this matches the discussion and can be approved now | 14:30 |
| * zyga needs to take the dog out | 14:32 | |
| ogra | ogra@pi3:~$ grep PRETTY /etc/os-release | 14:47 |
| ogra | PRETTY_NAME="Ubuntu Core 16" | 14:47 |
| ogra | ogra@pi3:~$ snap debug sandbox-features|grep confinement | 14:47 |
| ogra | confinement-options: classic devmode strict | 14:47 |
| ogra | why does that list classic on an UbuntuCore system ? | 14:47 |
| zyga | ogra: bug, should be fixed | 14:47 |
| ogra | k | 14:48 |
| zyga | mborzecki: https://github.com/snapcore/snapd/pull/7045 | 14:52 |
| zyga | just want to formally ack it, it's just a simple make fmt | 14:52 |
| zyga | ogra: https://github.com/snapcore/snapd/pull/7048 | 15:01 |
| ogra | zyga, ah, nice ! | 15:02 |
| zyga | jdstrand: not binding in any way: https://github.com/snapcore/snapd/pull/7049 | 15:11 |
| zyga | I don't know if it passes tests yet | 15:12 |
| zyga | just wanted to open it to see what happens and see what you think | 15:12 |
| jdstrand | zyga: approved both | 15:41 |
| === pstolowski is now known as pstolowski|afk | ||
| === ShibaInu is now known as Shibe | ||
| zyga | Oh, great, thank you :-) | 16:13 |
| === ricab is now known as ricab|bbl | ||
| === ricab|bbl is now known as ricab | ||
| hunterk | I'm trying to run a script as part of the snapification to symlink some libs that aren't being found. I'm doing it with override-prime, but that doesn't seem to be doing anything | 20:20 |
| hunterk | that is, i see no override message in the snapcraft build log and no symlinks | 20:20 |
| hunterk | is there some other, better way of doing this? | 20:20 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!