[05:17] morning [05:35] school run, back in 30 [06:20] re [06:23] welcome back mborzecki [06:23] zyga: I added some comments to 7700 but I think we should merge it [06:23] zyga: well, maybe merge or push my suggestions, either way, I think this should move forward :) [06:24] zyga: and sorry that I left you hanging there for so long [06:24] mvo: zyga: hey [06:28] hrm, I wish we had a "tell pawel once he joins" feature, hm, maybe mup supports this [06:29] PR snapd#9270 closed: wrappers, systemd: allow empty root dir and conditionally do not pass --root to systemctl [06:29] PR snapd#9390 closed: boot: fix debug bootloader variables dump on UC20 systems [06:33] mvo: mattermost? [06:36] mborzecki: yeah [06:39] i've always complained that core20 prepare takes long, but after workign with nested suite, i can't really complain anymore [06:40] good morning [06:40] mvo: let me look at 7700 [06:40] it's such a low number I suspect it's one of the older r-a-a branches [06:40] zyga: it is [06:41] zyga: should be very easy what I'm suggesting there [06:41] ah [06:44] mvo: I replied to most comments [06:44] mvo: if you feel like it, I always wanted to do inotify [06:45] mvo: but I will do inotify after the desktop notificatons api [06:45] zyga: probably a followup in any case but it feels nice. it will be a lot of work, yes? [06:45] as we may also use that logic here [06:45] zyga: right [06:45] inotify? not at all [06:45] for a single file that's really easy to use [06:46] it's only bad/hard api for unbound directories [06:46] thank you for the review :) [06:46] I thought this PR will have a cake [06:46] 2019 Oct [06:47] * zyga had late PT at 20:00 last night and had quite a workount [06:47] *workout [06:53] zyga: haha, let's try to avoid that it get's a cake [06:53] haha [06:53] good plan [06:53] I'm making good progress on notifications, hopefully first PR doing this tomorrow [06:54] mvo: wanna land a PR? [06:54] zyga: I replied, but please consider the suggestions with an open mind, I'm happy to do the modificiations if you feels burdensome [06:54] https://github.com/snapcore/snapd/pull/9407 needs a 2nd +1 [06:54] PR #9407: overlord: explicitly set refresh-app-awareness in tests [06:54] zyga: we can land it but please consider the ideas in there for a followup [06:55] mvo: I meant the more recent one [06:55] zyga: I am not at this one yet, working my way up slowly :) [06:55] ok :) [06:55] zyga: and great to hear that inotify is easy, that's cool [06:56] ah, I wasn't aware of the progress meter thing [06:56] mvo: I'll do a small pass as it's ancient and needs master merge anyway [06:56] mvo: but I'd like to keep the time duration anyway, 1s is really okay for this, snap refresh with a single aa compile is on the same order of magnitude [06:57] I'd actually keep it up *longer* after a brief period of being invisible [06:57] as this is user interaction [06:57] a window that blinks in and out of existence is poor UI [06:57] but this was really proposed to start a discussion [06:57] I'm just glad we got one now [06:57] it's not the end game [06:58] zyga: sure, that's fine [06:58] zyga: the blink window is a good point, makes me wonder if the initial delay should actually be longer, oh well - bikeshed land :) [06:59] mvo: we could do a headless UI for 3 seconds [06:59] and then open snap store with the refreshing snap instead [06:59] that shows progress already [06:59] zyga: but yeah, let's keep it at 1s and maybe even add a code comment like: "// initial display should not be too short or the user just sees some irritating flashing if the refresh is almost finished" [06:59] zyga: yeah, anyway, that is definitely followup material :) [07:00] ok [07:01] zyga: anyway, I think we are in agreement here, let's try to do minimal changes and land it as it's better than before [07:01] pl [07:01] ok [07:07] morning [07:07] o/ pawel [07:14] mvo: can you take another look at https://github.com/snapcore/snapd/pull/9393 ? [07:14] PR #9393: spread: drop vendor from the packed project archive [07:21] mborzecki: sure [07:24] PR snapd#9393 closed: spread: drop vendor from the packed project archive [07:25] mvo: thanks! [07:42] mvo: something you asked for https://github.com/snapcore/snapd/pull/9408 [07:42] PR #9408: tests/core/snap-debug-bootvars: spread test for snap debug boot-vars [07:43] pstolowski: what's the plan for snapshots and the setID of the zip files? I was just looking at the snapshot import PR again and tweaked some tests [07:44] pstolowski: quick HO maybe if it's cumbersome to type? [07:44] PR snapd#9408 opened: tests/core/snap-debug-bootvars: spread test for snap debug boot-vars [07:45] mvo: inside the zip we want to store 0 and use set id inferred from the file name. with the changes that landed already we will ignore snapshots where set id from filename != set id inside (so, reverting to current snapd from future snapd will not show such snapshots) [07:46] mvo: does this make sense? [07:47] pstolowski: yeah, that's great [07:47] pstolowski: that means the import PR is actually pretty close, if the filename is now the authority [07:49] pstolowski: so now we just need to change the code in master to use the filename for the sid? that sounds pretty neat and straightforard. [07:49] pstolowski: or are there more complications? [07:50] mvo: yes, that's mostly it. however i think we want to land setting id=0 inside the zip in a release following current changes [07:51] zyga@kaveri:~/go/src/github.com/snapcore/snapd/desktop/notifications/cmd/snapd-desktop-notify$ ./snapd-desktop-notify [07:51] server capabilities: [actions body body-markup icon-static persistence sound] [07:51] :-) [07:54] pstolowski: good point [07:57] ok, i've defocnflicted services PR with master.. now need to run spread tests to see if nothing broke [07:57] *deconflicted [07:59] PR snapd#9236 closed: kernel: remove "edition" from kernel.yaml and add "update" [08:38] re [08:38] zyga: this was the notifications code I started on way back: https://github.com/jhenstridge/snapd/tree/session-agent-notification-lib/usersession/agent/notification [08:39] jamesh: looking [08:39] (and thanks for sharing that!) [08:39] pstolowski: mborzecki: #9400 and then #9404 are ready for reviews [08:39] PR #9400: o/assertstate: support refreshing any number of snap-declarations [08:39] PR #9404: asserts: deserialize grouping only once in Pool.AddBatch if needed [08:39] pedronis: ack [08:39] neat, some things I will steal from that, if you don't mind [08:40] jamesh: do you have a link to the gtk spec? [08:40] I can google if not [08:41] zyga: there's something on the GNOME wiki, but I think the glib source is considered authoritative (for better or worse) [08:42] https://wiki.gnome.org/Projects/GLib/GNotification <- actually, this is pretty much it [08:42] what that page doesn't mention is that it is including the D-Bus activatable desktop spec by reference [08:43] jamesh: are application IDs the appstream IDs or something else? [08:43] that's how an (application_id, action) pair translates to a call back to the application [08:44] zyga: it is a D-Bus bus name, which also needs to be a desktop ID with the ".desktop" suffix removed [08:45] zyga: clicking on a notification turns into an Activate or ActivateAction call on the bus name using this interface: https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s08.html [08:47] I hadn't finished off the action handling in that code. [08:47] jamesh: what decides if ActivateAction is used? can one convey the action somehow? [08:48] zyga: if you click on a notification with no default action, it activates the app [08:49] which the app would usually respond by raising itself [09:20] Hello the version of I need ogre3d disappeared [09:21] There is only latest/stable: 1.12.7 2020-06-20 (151) 151MB [09:21] while mine program needs 1.12.5 are there any archives [10:01] degville: in a meeting still, will join soon [10:01] degville: sorry! [10:02] mvo: no problem at all! [10:13] PR core18#172 opened: Add finalrd to core18 [10:26] * zyga takes a break for coffee [10:26] good progress on notifications API [10:26] now need to play with demo program on some systems to see how things behave [10:26] then need to draft some text for actual messages [10:26] then start working on user agent changes [10:45] PR core20#87 closed: Revert "hooks: mv docker user/group definition to extrausers" [11:05] PR snapd#9407 closed: overlord: explicitly set refresh-app-awareness in tests [11:05] thanks mvo :) [11:43] mborzecki: I reviewed #9401 but I have a question there (if it makes sense) [11:43] PR #9401: gadget: allow content observer to have opinions about a change [11:43] pedronis: thanks [11:59] pedronis: i'll drop that mock and remove GetRecoverySystemEnv from the interface [12:20] PR snapd#9409 opened: cmd/snap: implement 'snap validate' command <⛔ Blocked> [12:30] PR snapd#9235 closed: tests: new tests for nested and external <⛔ Blocked> [12:30] pedronis: still in a meeting, will join in 1-2min [12:37] ijohnson: do you mind if I squash merge 9379? will make the cherry picking easier [12:39] mvo: go for it, thanks [12:40] ijohnson: done and cherry-picked [12:40] PR snapd#9379 closed: cmd/s-b/initramfs-mounts: use ConfigureTargetSystem for install, recover modes [12:43] PR snapcraft#3295 closed: specification: desktop extension font hook [13:00] PR snapd#9410 opened: desktop/notification: add bindings for FDO notifications [13:21] jamesh: review welcome https://github.com/snapcore/snapd/pull/9410 [13:21] PR #9410: desktop/notification: add bindings for FDO notifications === seb128_ is now known as seb128 [13:36] * zyga breaks to take the dog out [14:13] ijohnson: hey, #8960 is ready for review [14:13] PR #8960: o/snapstate,servicestate: use service-control task for service actions (9/9) [14:13] pstolowski: great thanks I will try to take another look today/tomorrow [14:14] pstolowski: is that going into 2.47 or 2.48 ? [14:14] ijohnson: not into 2.47, no [14:14] pstolowski: ack thanks for clarifying [15:08] zyga, could you please take a look to that one? #8986 [15:08] PR #8986: tests: new snaps-state command - part1 [15:08] cachio in a call, I'll look in ~ hour [15:08] zyga-mbp, sure, no hurry [15:39] #9405 is super simple and green, only 3 lines deleted which enables kvm for another nested test [15:39] PR #9405: tests/nested/manual/grade-signed-above-testkeys-boot: enable kvm === oer is now known as oerheks [15:57] thanks pstolowski ! [15:57] zyga-mbp: i'm seeing https://pastebin.ubuntu.com/p/jW3jnbXrH2/ on 20.10; apparmor 3? for full context - https://github.com/snapcore/snapd/pull/9405 [15:57] PR #9405: tests/nested/manual/grade-signed-above-testkeys-boot: enable kvm [15:57] I saw that too, likely yes [16:01] PR snapd#9405 closed: tests/nested/manual/grade-signed-above-testkeys-boot: enable kvm [16:15] cachio https://github.com/snapcore/snapd/pull/8986#pullrequestreview-495731254 [16:15] PR #8986: tests: new snaps-state command - part1 [16:15] cachio only two things I'd change, otherwise LGTM [16:15] zyga-mbp, tx [16:15] cachio thanks, please ping me for another pass or reply if you disagree strongly [16:15] I'm going to EOD now [16:17] zyga-mbp, sure, I'll ping you tomorrow [16:17] thanks [16:18] zyga-mbp, I answered on #9398 [16:18] PR #9398: tests: split the nested helper in 2 parts [16:18] for tomorrow [16:18] you don't have to create the tool this way [16:18] right now nested.sh is a library [16:18] keep using it as such [16:18] but only _from_ the tool [16:19] so write a shim tool that sources nested and invokes some functions [16:19] has a CLI and can be used from tests [16:19] port one test [16:19] iterate [16:19] never touch nested.sh unless required [16:19] so you don't care if it gets changed [16:19] at the end of the day you should be able to just merge nested.sh into the tool, since nobody else is using it [16:19] * zyga-mbp packs the last VM for the dy [16:19] *day [16:20] cachio it's your choice, that's just my advice [16:22] cachio: I agree with zyga-mbp on this one, your current pr is just too difficult to review [16:22] I started to review it but it's just hard to do a good review considering so much is changing, even if the changes are mostly simple [16:23] zyga-mbp, ok, makes sense, what I don't want to do is to maintain 2 thinks [16:24] so if the idea is to maintain just the tool it is ok [16:25] cachio: you don't need to maintain 2 versions of the same file, you keep nested.sh as-is, and add a nested-tool.sh or something which has subcommands which call the various functions defined from nested.sh instead [16:25] ijohnson, yes, agree on that [16:25] cachio: then gradually add commands and such to nested-tool.sh and move tests from using nested.sh to using nested-tool.sh gradually [16:25] ok, nice [16:25] happy to help get started on that if you need any help [16:25] zyga-mbp, ijohnson thanks for the advice, I'll create a new pr [16:25] thank you cachio [16:26] thank you! [16:29] * cachio lunch [16:41] zyga: fyi, pawel's denial has to do with the new kernel. snap-confine has: @{PROC}/[0-9]*/attr/current r, [16:42] zyga: that shoudl be changed to @{PROC}/[0-9]*/attr/{,apparmor/}current r, [16:44] zyga: that should happen in the the 'w' in snap-confine too, as well as cups_control.go, lxd_support.go, browser_support.go. ./sandbox/apparmor/process.go should be adjusted to see if proc/%v/attr/apparmor/current exists and if not, fallback to proc/%v/attr/current [16:44] ijohnson: it seems zyga and pawel signed off. can you make sure they see ^ [16:44] jdstrand: sure I can also propose a pr for that, to be clear that is from apparmor3 in groovy though right ? [16:45] oh sorry you said it's the new kernel [16:45] but yes I can take care of that [16:46] ijohnson: yes and no. the LSM stacking support has been moving forward. selinux claimed @{PROC}/[0-9]*/attr/current for themselves and so apparmor couldn't reuse it and moved to @{PROC}/[0-9]*/attr/apparmor/current [16:46] I see [16:46] ijohnson: so libapparmor will first look at @{PROC}/[0-9]*/attr/apparmor/current if the kernel has it [16:47] that makes sense [16:47] ijohnson: and that libapparmor change is in aa3 [16:47] right yeah I seem to remember that aa3 was imminent in groovy [16:47] yeah [16:47] jdstrand: do you expect any other regressions in groovy we should be on the look out for w/ respect to apparmor3 landing ? [16:48] ijohnson: I didn't mean for you to 'take care of' the above, but if you want to, be all means, do :) [16:48] yeah it doesn't sound like a lot of work I can take care of it [16:48] ijohnson: no. I've been using aa3 for weeks [16:49] ijohnson: and there was extensive tests. tbh, I would've alerted you to this had I known that groovy's kernel moved to @{PROC}/[0-9]*/attr/apparmor/current (and that ./sandbox/apparmor/process.go needed a corresponding update) [16:49] great, glad to hear that. we were all kinda nervous about aa3 in groovy regressing snapd since it was mentioned there would be work to make snapd work with aa3, but perhaps what was meant was more just moving snapd to take advantage of new things in aa3, not fixing snapd to work the way it did before aa3 [16:49] but in terms of policy/etc, it should be ok with how we setup /etc/apparmor.d/abi [16:49] and also considering how late in groovy cycle aa3 was landing [16:50] oh, yes, there is just this small stuff. the work to do is for cross distro and taking advantage of new aa3 features [16:51] ijohnson: we're committed to fixing bugs with the highest urgency if you see problems in apparmor [16:52] cool, sounds good [16:52] ijohnson: we do hope to do PRs for snapd to take advantage of aa3 next cycle [16:52] +1 [17:16] PR snapd#9411 opened: cmd/snap/auto-import: stop importing system user assertions from initramfs mnts === ijohnson is now known as ijohnson|lunch === ijohnson|lunch is now known as ijohnson === Eighth_Doctor is now known as Conan_Kudo === Conan_Kudo is now known as Eighth_Doctor [18:09] cachio: looks like tumbleweed is working again to run tests now, though there are some failures, but we now have at least 57 tests passing there now :-) [18:13] jdstrand: pr for the apparmor 3 attr path change is up now at #9412 [18:13] PR #9412: many/apparmor: adjust rule for reading apparmor profile for new kernel [18:16] PR snapd#9412 opened: many/apparmor: adjust rule for reading apparmor profile for new kernel [18:26] ijohnson: ack, approved [18:27] thanks! [18:33] ijohnson, yes, yesterday I updated again that image [18:33] thanks cachio [18:33] ijohnson, I'll check today or tomorrow why the tests are failing [18:34] cachio: well it's a start since we have some tests running successfully there [18:48] jdstrand: hey [18:49] jdstrand: do you have any time to review snap-confine udev patch? [18:49] zyga: hey :) [18:49] zyga: yikes, no, I don't. but maybe I can find someone. which PR? [18:50] jdstrand: https://github.com/snapcore/snapd/pull/7614 [18:50] PR #7614: cmd/snap-confine: implement snap-device-helper internally [18:50] it's the one you reviewed before [18:52] amurray: hey, would you mind taking the security review to the finish line with https://github.com/snapcore/snapd/pull/7614 ? I did a review some time ago and it looks like the comments were addressed. having someone give it a once over would be helpful. if you can't let me know and I can find someone else [18:52] PR #7614: cmd/snap-confine: implement snap-device-helper internally [18:52] thank you! [18:52] I know time is hard to find [19:43] cachio: can you also take a quick look at https://github.com/snapcore/snapd/pull/9413? it is also really simple [19:43] PR #9413: tests/lib/nested.sh: more little tweaks [19:47] PR snapd#9413 opened: tests/lib/nested.sh: more little tweaks [19:47] ijohnson, sure [19:55] jdstrand: do we also need to allow for /proc/<>/attr/apparmor/exec ? I see that is used now in groovy too [19:55] jdstrand: for example [19:55] 2020-09-24T19:37:19.1095835Z [Thu Sep 24 19:36:48 2020] audit: type=1400 audit(1600976207.587:66): apparmor="DENIED" operation="open" profile="/snap/core/10045/usr/lib/snapd/snap-confine" name="/proc/38092/attr/apparmor/exec" pid=38092 comm="snap-confine" requested_mask="w" denied_mask="w" fsuid=0 ouid=0 [20:18] jjohansen: hey, can you answer ijohnson ^ (in meetings) [20:19] jdstrand: sorry which channel? [20:19] jjohansen: #snappy on frenode. this one :) [20:21] ah, ijohnson, jdstrand: /proc/<>/attr/{apparmor/,}exec is used by apparmor for change_onexec [20:21] jjohansen: with the new aa3 on groovy, to transition profiles, libapparmor will write to /proc//attr/apparmor/exec now instead of /proc//attr/exec correct ? [20:21] jjohansen: thanks [20:21] the new apparmor/exec path is the LSM stacking aware interface that is dedicated to apparmor [20:22] the old attr/exec (no apparmor) is a shared interface [20:22] the apparmor/exec is only on kernels 4.5 and newer [20:23] ijohnson: apparmor3, will try both, it prefers the new apparmor/exec path [20:23] so it does it first [20:23] things should work if it isn't allowed, but if we stack with another LSM we are going to want the newer path allowed [20:24] jjohansen: ok, that makes sense, I will update snap-confine to allow both paths then [20:26] jjohansen: do you want to quickly review https://github.com/snapcore/snapd/pull/9412/commits/0e2113642e4310c8e8c91cd58a00d01648f594d5 too? I pushed that commit after jdstrand gave +1 [20:26] PR #9412: many/apparmor: adjust rules for reading profile/ execing new profiles for new kernel