=== chihchun_afk is now known as chihchun === chihchun is now known as chihchun_afk === chihchun_afk is now known as chihchun === chihchun is now known as chihchun_afk === pbek_ is now known as pbek [05:07] morning [05:26] Hi [05:35] Mvo: I fixed it [05:42] zyga: \o/ [05:43] zyga: woha, nice [05:43] zyga: what was it? [05:43] Flipped logic [05:43] Look at the FIX patch [05:43] * mvo looks [05:43] Core test still need change but runtime is fine [05:45] zyga: what core test needs a change? [05:46] Or actually no. It doesn’t [05:46] Ignore me :-) [05:46] Not enough sleep [05:46] Don’t look at the time stamp [05:46] I will clean this up and propose to master [05:50] zyga: uhhh [05:50] * mvo hugs zyga [05:50] zyga: really not enough sleep [05:58] zyga: yay, works here, nice job [05:58] Same here [05:58] Cool, so one last issue ahead [05:58] And one that should be way [05:58] Easier [05:58] I’ll get some coffee and get right to it [06:00] mvo: hi, can you take a quick look at #5424 ? [06:00] PR #5424: tests/main/snapd-notify: use systemd's service properties rater than the journal [06:02] mborzecki: good morning [06:02] mborzecki: sure [06:16] PR snapd#5361 closed: snapstate: allow removal of snap.TypeOS when using a model with a base [06:19] PR snapd#5424 closed: tests/main/snapd-notify: use systemd's service properties rater than the journal [06:19] mvo: yay! [06:20] mborzecki: thank you, that was failing a lot recently [06:23] #5392 needs a 2nd review [06:23] PR #5392: snapstate,ifstate: wait for pending restarts before auto-connecting [06:37] abeato: hey, good morning! 5309 got a +1 just a tiny tweak to the filename is requested [06:37] mvo, morning! [06:38] mvo, yeah saw that, will change it and ping you back :) [06:39] abeato: great, thank you! I'm keen to land it :) [06:48] sil2100: hey, good morning. I noticed our /etc/hosts on core18 is empty, do you know where that files comes from nowdays? I wonder if we need to add a static one to our build tooling [06:48] sil2100: with localhost and the like [06:52] mvo, 5309 re-pushed [06:52] looking === pstolowski|afk is now known as pstolowski [06:59] morning [07:00] good morning pstolowski [07:00] pstolowski: hey [07:11] re [07:11] small horror story [07:12] what happens after: [07:12] 1) you talk with your wife about ways of making coffee [07:12] and about perhaps getting those portable espresso machines [07:12] 2) you work till 3AM [07:12] anyone wants to guess? :) [07:12] the coffee machine breaks :( [07:14] zyga: you go a buy a moka pot? :) [07:14] PR snapd#5420 closed: snap-mgmt: fix for non-existent dbus system policy dir, shellchecks [07:14] what's a moka pot? :) [07:14] so far I fed the children and tidied the kitchen, now I need to walk the dog [07:14] zyga: https://en.wikipedia.org/wiki/Moka_pot [07:15] but first, small rebase of that WIP+FIX patches so that I can send it to master [07:18] mvo: let me make sure [07:18] any PRs in need of review? [07:24] mborzecki: the 2.34 ones would be great [07:26] mvo: ok, I have a nice patch now [07:27] I force-pushed to my working branch [07:27] I will now open a master PR [07:29] mvo: could you review #5304 ? [07:29] PR #5304: overlord/ifacestate: simplify checkConnectConflicts and also connect signature [07:29] heh, sorry [07:30] mvo: I meant #5403 [07:30] PR #5403: many: use extra "releases" information on store "revision-not-found" errors to produce better errors [07:30] pedronis: sure [07:31] zyga: https://paste.ubuntu.com/p/vn9XJVcpKj/ [07:31] mborzecki: is match passing -E? [07:31] but ... odd [07:31] do you have a debug shell? [07:31] if so, type MATCH could be useful [07:32] zyga: yeah, i checked spred source code, it's using ... grep -q -E \"$@\" .. [07:32] zyga: https://github.com/snapcore/spread/blob/master/spread/client.go#L357 [07:33] PR snapd#5432 opened: cmd/snap-confine: fix snaps running on core18 [07:33] mvo: ^ this is for you [07:33] mborzecki: magic :) [07:33] mborzecki: yeah, this is strange, this tests passes most of the time [07:33] zyga: \o/ [07:34] mvo: I need to take the dog out and look after myself a little [07:34] I will be back around 10:30 perhaps [07:40] zyga, do you think we can have a fedora-based snap to demo by Flock? [07:40] yes [07:41] mvo, 5309 finished CI fine (thanks for the change in the spread test, I had not noted that one) [07:41] abeato: ta [07:42] \o/ [07:42] PR snapd#5309 closed: overlord/configstate: add watchdog options [07:45] Mornings [07:48] hey [07:48] niemeyer: hey, you're early, adjusting to new timezones? [07:49] mborzecki: Heya [07:49] mborzecki: Nah, just in the mood for night hacking [07:50] Well.. "hacking".. reviewing right now [07:51] mvo: #5392 paniced btw, not sure it's the panic fixed by the other PR [07:51] PR #5392: snapstate,ifstate: wait for pending restarts before auto-connecting [07:52] mvo: could be though, maybe they need to be merged [07:52] pedronis: indeed, I think its the same, I will merge the fix and see if that helps [07:53] pedronis: but its slightly odd as this panic happens when there are snaps in the seed, I will dig into it [07:53] pedronis: right after I reviewed your PR [07:54] PR snapd#5388 closed: tests: fix tests when no keyboad input detected [07:55] pedronis: aha, I think I know what is going on, silly me [07:59] ah, it was the previous commit [08:00] pedronis: yeah === chihchun_afk is now known as chihchun [08:23] pedronis: reviewed, looks fine and added some comments but we can land when green I think [08:26] mvo: thx, yes, need to tweak a couple of spread tests to get it green [08:39] * Chipaca afk for a while [08:40] cjwatson: when did "Manage this package in the store" link land in the store? I only just noticed it. It's super useful, thank you (if it was you, which I assume it was) that made it happen :) [08:41] s/store/launchpad/ [08:44] popey: 2016 :-) You're welcome [08:44] wow [08:51] mvo, it's happening! https://pagure.io/flock/issue/87 [08:51] Son_Goku: \o/ woah [08:51] nice! [08:52] I just filed the proposal since niemeyer gave me the all-clear [09:00] PR snapd#5427 closed: configcore: fix incorrect handling of keys with nubmers (like gpu_mem_512) [09:14] mborzecki: 5389 looks good, does this need any further design review or can I just land it? [09:14] mborzecki: at this stage, what is missing for parallels installs to get them working end-to-end? just curious [09:16] mvo: all the interface backends need some work (some a little, some a bit more) [09:16] mvo: i still have changes for snapstate/snapsup, some tests in overlord, small change in daemon and some spread tests, plus changes in store [09:16] pedronis: aha, thanks [09:16] mvo: oh and some bits in snap-confine too [09:17] PR snapd#5389 closed: snap: account for parallel installs in wrappers, place info and tests [09:18] PR snapd#5429 closed: osutil: import go-udev [09:20] hit a weird problem, if /etc/dbus-1/system.d does not exist and we dump a file there (creatign the directory first), does dbus daemon need a reload/restart to pick up the new files? [09:21] mborzecki: I bet it does [09:21] mborzecki: probably a inotify (or whatever it is nowdays called) restriction [09:21] so #5413 does some distro purge, in arch /etc/dbus-1/system.d is not owned by dbus but rather by individual pacakges [09:21] PR #5413: tests: purge packages installed by accounts, calendar, and contacts interface tests [09:21] mborzecki: i.e. I assume it sets up a watcher for this dir, but if the dir does not exists it will not setup this watcher [09:22] it's possible that we end up without /etc/dbus-1/system.d at some point [09:22] then when snap is installed, create the dir and dump the *.service file but it's not picked up [09:22] mborzecki: hm, that seems unfortuante [09:22] PR snapd#5392 closed: snapstate,ifstate: wait for pending restarts before auto-connecting [09:23] mborzecki: might be worthwhile to fix in the arch dbus package? it seems odd that it would not own the dir [09:23] mvo: i could own /etc/dbus-1/system.d in snapd package, but it's slightly meh, another empty dir [09:23] mborzecki: especially if the suspicion about the inotify watch is true [09:24] mborzecki: I don't know the conventions of arch so I might be off but I think the dbus arch package should own the dir [09:24] mborzecki: or at least co-own it [09:24] mborzecki: like if dbus is installed it should always be there [09:24] mvo: the 'convention' is no empty dirs shipped by the pacakge, but we're already not doing this [09:24] or doing, i.e. shipping empty dirs :) [09:25] mborzecki: heh, yeah, I understood (but parsing took a little longer) [09:25] mborzecki: so that is why dbus does not own the dir? because it would be empty? [09:25] mvo: yes [09:25] mborzecki: that seems to be slightly silly tbh, it could ship a README in there (again assuming the theory about inotify is correct) [09:26] mvo: it ships /usr/share/dbus-1/system.d, though we don't touch that [09:26] mborzecki: because dbus is buggy for other packages as well. install foo-that-uses-dbus.d and it won't get picked up until dbus is restarted [09:27] mborzecki: anyway, just my 2¢ [09:28] zyga, mborzecki can you take another look at https://github.com/snapcore/snapd/pull/5416 ? [09:28] PR #5416: interfaces/hotplug: add hotplug Specification and HotplugDeviceInfo [09:29] niemeyer: Q, what kind name we would use for the new single one (the old one cannot be reused because different expectations about Value) ? [09:31] pedronis: Was hoping we could just preserve what we have [09:31] pedronis: How has the Value changed? [09:31] pedronis: Is it incompatible? [09:31] niemeyer: Value is now a mapping of details, before it was just snapName [09:31] * niemeyer looks again [09:32] pedronis: But is it just more than what we had, or incompatible somehow? [09:32] it's incompatbile string vs mapping [09:32] pedronis: Ah, the entire "value" was a snap name? [09:32] yes [09:33] Ouch [09:34] niemeyer: we could use snap-revision-not-available-as-specified (to follow your suggested base error message) [09:34] pedronis: "snap-not-available" maybe [09:34] pstolowski: yes [09:35] niemeyer: sounds a bit too much like snap-not-found to me [09:35] Indeed [09:35] hmm [09:35] zyga: I took the liberty to push a very small spread test to your 5432 fixup pr, thanks again for this [09:36] sure, thank you [09:37] niemeyer: also to explain, I went for two kinds, not just because I needed a kind, but because so a naive client could give a slightly accurate message without parsing value [09:38] pedronis: Ah, indeed.. that will also be solved with the separate kind [09:38] I mean "there is nothign for this arch" vs "there is something but not in this channel" [09:41] pedronis: I see [09:41] pedronis: That's nice indeed [09:42] pedronis: and I guess the "nothing for this arch" is quite useful, even if there's nothing in this channel either [09:42] yes [09:42] It's more like "give up, you won't find anything here" [09:42] that was the reasoning [09:42] pedronis: +1 [09:43] I will switch to the shorter kind you proposed tough [09:43] pedronis: Btw, I think we should provide such a message from the server also [09:43] pedronis: In case the client is dumb [09:43] pedronis: Right now I think it's only providing the general "nothing found" one [09:43] let me see [09:44] niemeyer: no, it gives two different messages if (it has info) [09:44] no snap revision for the given channe [09:44] l [09:44] and [09:44] Or more specifically, "nothing for given constraints".. there's a note about this message being bad in the review, but it would be nice to at least hint at the delta between "nothing for this arch" vs "nothing for this channel", even if we don't go full blown for now [09:44] no snap revision for the given architecture [09:45] pedronis: Ah, okay.. I must have missed something then.. it thought it was just returning the rna.Error() message, which is a single one [09:45] that's used only as fallback [09:45] if some reason or corner case [09:45] Ack [09:45] the store doesn't provide the info [09:46] niemeyer: code is here: https://github.com/snapcore/snapd/pull/5403/files#diff-e1016939c627280d9f6c53bdab0530bfR399 [09:46] PR #5403: many: use extra "releases" information on store "revision-not-found" errors to produce better errors [09:48] pedronis: Cool, thanks, looks good.. suggested a minor tweak to the messages [09:48] ok, I'll proceed as discussed then [09:49] Thanks [09:49] about simplifying thanks, I'll first go over the other feedback, messages and kinds [09:49] and then see what i can do [09:49] (it's already a bit big and some of that is preexisting) [09:49] mvo: Three PRs from 2.34 to go.. but I'll try to get a couple of hours of sleep before the meeting marathon this morning.. may not be able to review these last ones before my afternoon [09:49] heh, s/thanks/things/ [09:49] niemeyer: no worries [09:50] niemeyer: we can always postpone them or merge them slightly later [09:50] niemeyer: thanks a lot for your reviews [09:50] np, see you all soon o/ [09:56] mvo: I'll go over the new feedback for 5403 after lunch [09:58] pedronis: thank you, also no worries, I plan to do 2.34~pre1 today to see if it builds everywhere and if autopkgtest are fine, we will probably need to do a ~pre2 to deal with that fallout anyway so if a PR is not ready today that is not too terrible [10:00] pedronis: I also updated 5428, should be an easy review I hope [10:09] zyga: can #5411 land? [10:09] PR #5411: many: remove core-support interface [10:09] mvo: it's red atm [10:10] pstolowski: I think so but I wanted jdstrand to see it too [10:10] ack [10:13] PR core#89 opened: hooks: fix 30-fix-timedatectl.chroot to DTRT when running on non-core devices [10:14] PR core18#44 opened: hooks: fix 030-fix-timedatectl.chroot to DTRT when running on non-core devices [10:22] PR snapd#5431 closed: interfaces: move ValidateName helper to utils [10:29] zyga: we are down to two failing test: ubuntu-core-18-64:tests/main/snap-disconnect (and one race) and tests/main/catalog-update which I am looking into right now [10:30] that's great, I will give you the patch for disconnect today so that will be just landing and polishing for next week [10:30] mvo why is catalog-update failing? [10:30] zyga: no idea, might be a fluke [10:30] and what happened to systemd and signals? did you find the issue? [10:30] zyga: checking now [10:30] zyga: this is the racy one [10:30] I see that test fail from time to time (~ once a week maybe) [10:30] zyga: still not sure what is going on there :( [10:30] aha [10:30] ok, good news though [10:31] I think core18 will rock :) [10:31] zyga: heh, eventually [10:31] zyga: but yeah, we are on the right path at least [10:48] mvo: so I just did a quick check - the /etc/hosts file is still generated by netcfg from what I see, which is a d-i package [10:48] sil2100: aha, so we probably should include something in our build given that we don't run d-i [10:48] mvo: I guess the easiest way would be just to prepare a static one [10:49] Let me do that [10:50] sil2100: \o/ thank you [10:50] zyga: catalog-refresh looks very much like a bug in get_journalctl_log [10:50] yeah [10:50] zyga: when I run journalctl -u snapd.service | MATCh things magically work [10:50] I think that it is buggy because of buffering [10:50] we don't see stuff in the "pipe" [10:50] zyga: I think we need to revert that [10:50] zyga: it might also the reason why the stop-mode test is failing [10:51] zyga: I can look after lunch [10:51] that would be a good thing to see actually [10:51] yeah [10:51] ok, thank you [10:51] zyga: my gut feeling is revert [10:51] I'm doing reviews, my head is too dizzy still [10:52] zyga: ok - if you want we can have a quick HO and I can do the disconnect work. I just need some info what you had in mind. but lunch first [10:52] no, that's fine, I will finish that one :) [10:52] zyga: I'm super motiviated to get the last bit in [10:52] zyga: ok [10:52] * mvo hugs zyga [10:52] yes, mee to :) [10:52] me too [10:52] just ... sleepy [10:52] * zyga takes a break from reviews and hacks that bit in [10:56] PR snapd#5433 opened: interfaces/repo: added AllHotplugInterfaces helper === pstolowski is now known as pstolowski|lunch [10:59] PR core18#45 opened: Provide a static simple /etc/hosts file [10:59] mvo: ok, testing the fix now [11:08] zyga: woah, thats impressively fast [11:09] sil2100: \o/ [11:12] mvo: that's a good joke for a 1st patch at 13:12 :) [11:12] * mvo hugs zyga [11:15] PR core18#45 closed: Provide a static simple /etc/hosts file [11:25] PR snapd#5434 opened: overlord: introduce InstanceKey to SnapState and SnapSetup, renames [11:50] mvo: some more tweaks after first round of testing [11:50] I'll break for 30 minutes now and get back to this before the standup [11:50] I think it will pass before the standup === pstolowski|lunch is now known as pstolowski [12:15] PR snapd#5408 closed: i18n: use xgettext-go --files-from to avoid running into cmdline size limits [12:36] mvo: I tweaked it now to be even easier, testing now === chihchun is now known as chihchun_afk [12:47] running more tests, it's funny how I ping pong around the cod [12:52] zyga: :) thanks a lot! [12:53] I forgot we have resolve disconnect that is full blown and used in daemon/api.go [12:53] so perhaps the tweaks to other places are actually not needed [12:53] (more self-contained change) [12:54] yes, the fewer the places the better, the easier is to reason about [12:54] pedronis: re 5422 - I agree with you, we need at least a proper test and I can give the idea of using the connections a go. I want it in 2.34 but in a way that is not too ugly [12:56] mvo: yes, a regression test would also be good [13:02] joining [13:07] oops, standup [13:07] * Chipaca runs [13:08] mvo: it passes now, I will remove my extra patches :) [13:13] mvo: I pushed two tiny patches to my integration PR [13:14] mvo: I will amend the latest one to have more tests [13:14] have a look [13:14] zyga: nice [13:25] mvo: question [13:25] mvo: why is snap-disconnect test _not_ listed for systems: [-ubuntu-core-16-64] [13:26] zyga: I don't know why its disabled there? [13:26] my guess was home is not auto connected but that's not relevant there [13:26] I'll re-enable it [13:26] zyga: thanks [13:26] zyga: lets check git blame [13:26] ha, good idea [13:26] checking [13:27] since day 1 [13:41] mvo: and it passes :) [13:41] zyga: \o/ [13:41] mvo: also pushed [13:45] mborzecki: where is that failure from? the MATCh one [13:46] Chipaca: https://github.com/snapcore/snapd/blob/master/tests/lib/prepare.sh#L482 is the relevant code [13:46] yes i've got that [13:46] Chipaca: aha, you mean from what PR? [13:46] yeah [13:46] Chipaca: i don't remember the actual PR [13:46] basically, two things could be happening that I can think of right now: [13:47] Chipaca: but if you pick one of the failed ones there's a high chance you'll run into it :) [13:47] 1. a non-printable character is in that file, before 'test' [13:47] note the first grep does >> instead of >, so if there was rubbish there it'll still be there [13:48] 2. something wrote to the file between the grep and the MATCH (I don't think so as nothing should be happening concurrently at this point) [13:48] Chipaca: one possibility is that the file >> sometimes doesn't end with newline [13:49] pedronis: right, that's what I mean, there might be a \0 or something in the file, and it gets test:yaddayadda appended [13:49] I meant the file >> to [13:49] if there's no reason for the first >> we should change it to a > and be done with it :) [13:50] pedronis: sorry i didn't quite follow you [13:50] pedronis: if you mean the left hand side of the >>, that's a grep so it'll be adding its own newlines [13:50] no [13:50] I mean the preexisting file ending [13:50] maybe we should add a check about that assumption [13:51] at least it woudl cleanly [13:51] would fail cleanly [13:52] Chipaca: so I've followed the suggestion to mount inside the VM, so carry on with the idea of 400 for snaps [13:52] sergiusens: *smooch* [13:53] Chipaca: fwiw, golang-1.10 builds fine on xenial in pbuilder, trusty looks a bit more involved, at least the build-depds need to be updated there but probably also not that much work given that we have 1.6 there already [13:54] Chipaca: something like either length is 0 or tail -c 1 /mnt/system-data/var/lib/extrausers/"$f"| some way to check for a single "\n" [13:54]