[13:08:43] I'm in ur upstream, fixing ur commits [13:33:07] That Avro code is a mess [13:33:57] And where should issues be actually filed for these random wikimedia vendor repos we have? [13:34:00] https://phabricator.wikimedia.org/T119192 [14:04:09] * @return bool|null True if loaded, null otherwise [14:04:12] * Reedy blinks [14:09:14] Reedy: usually should be filled in Phabricator [14:09:22] hoping you get a matching project tag [14:09:27] There isn't one [14:09:32] Vendor doesn't feel right [14:10:22] we have the git project avro-php [14:10:24] in gerrit [14:10:56] good luck :) [14:15:49] legoktm: Those failing tests are doing something time-related. No idea why they might be out by over five minutes though. [14:16:20] legoktm: I rewrote the unit tests to work with the old version of phpunit, since it didn't seem like we'd get phpunit updated any time soon. [14:28:34] composer validate is stupid [14:28:36] give warnings [14:28:38] fix warnings [14:28:42] give different warnings [15:09:08] * @return $this [15:09:10] That's valid? :/ [15:09:40] seems it is [15:18:21] Of course you can return $this [15:18:54] Most Message class function do it so you can chain calls. [15:19:05] $msg->inLang( 'en' )->useDatabase( false ) [15:19:07] That sorta thing [15:22:09] it's better written as @chainable. (in JS, at least) [15:22:39] ostriches: I know you can returnt his [15:22:43] return this [15:22:47] But as phpdoc? [15:23:23] hehe [15:23:32] I would put the class/interface name there [15:23:52] but if tools suppor that, why bother changing [15:24:26] just looks a bit weird [15:24:34] Guess it works nicely if you make subclasses etc [15:24:39] Ah, heh [15:32:11] * @param boolean $config['selected'] Whether the radio button is initially selected [15:32:14] Is that valid too? :/ [15:32:24] It's already got a @param boolean $config above [15:34:50] that looks like someone using the javascript documentation pattern in PHP :) [15:35:55] mmm, indeed [15:36:04] PhpStorm doesn't like it... [15:36:17] Probably should just file an oojs-ui bug and have them fix them [16:25:38] Committing patches to diffusion, gerrit and github [16:25:39] lol [16:28:39] I do like it when upstream are proactive [17:48:36] Reedy: If you want to push the "fix all this shit" button in phpstorm for the avro lib I'll review it. Otherwise I might be able to work on it myself next week. I think we are going to want/need the avrojson encoding support that is missing anyway. [17:48:58] (this "little" logging project keeps growing) [17:49:02] haha [17:50:45] Let me clone the source repo and see what phpstorm wants to do to it [17:53:23] tgr|away: On https://gerrit.wikimedia.org/r/#/c/254080, I think Aaron and I both think skipping the cache in idFromName when READ_LATEST is used is a good idea. If we can do that, I'm happy to merge/deploy that patch today so James can setup votewiki for the election. [17:54:40] And tgr magically emerges on summoning :) [17:55:00] phone notifications [17:55:42] shouldn't that rather be done in a separate patch? [17:58:47] tgr: I'm not sure? My thinking is that as aaron said, after this patch, if something calls addToDatabase, then idFromName (without READ_LATEST), then idFromName (with READ_LATEST), they get the wrong anser. [17:59:41] I don't think we have any code paths that do that, but I didn't want to assume it can't happen [18:00:14] that's already the case though, not really related to this patch [18:00:37] Good point. [18:01:17] anyway, I can add it to the patch if you are not worried about idFromName performing badly because of it [18:02:06] the current patch just seemed a smaller change [18:02:27] I'm a little worried about it, and it sounds like Brad and Aaron both think it would be a good idea-- I'll be honest, I haven't been involved enough to say I know the right answer. [18:02:41] alternatively, I could go back to the first version of the patch and set the cache instead of clearing it [18:03:32] I would like a patch that passes all tests, unless the test was fixed before the last cut? [18:03:38] I agree it's a good idea, I'm just not sure it's a good enough idea to be deployed on friday :) [18:03:45] the test was fixed since, yeah [18:07:19] anomie: Do you think the current patch on https://gerrit.wikimedia.org/r/#/c/254080 is ok enough for a friday deploy? [18:08:53] * greg-g looks in [18:09:18] Does grep get pinged on "deploy?" [18:09:24] * greg-g has a tingly sense [18:09:28] csteipp: PS4 is going to fail unit tests. [18:10:08] wasn't the BlockTest patch merged? [18:10:29] oh, it wasn't, my bad [18:11:08] anomie: any objections against merging that? I'll fix the other weird test in a separate patch [18:11:45] tgr: The more I think about it, the more I like the idea of just not messing with changing user_id in the database at all. [18:20:03] So not to put artificial pressure on you, but tgr/anomie, I need to go afk for about 2 hours soon, and James really needs to create votewiki users before monday. Can you guys get something merged (or if you know of a workaround, that would be fine too) by noon? I'm happy to help, I just haven't been keeping up with all the recent changes in User. [18:21:17] create them server-side? [18:34:34] which database user do CLI mwscript invocations run as? [18:46:55] also for James: https://gerrit.wikimedia.org/r/#/c/254375/ and https://gerrit.wikimedia.org/r/#/c/254446/ [18:47:10] code review appreciated [18:47:15] anomie: can you give it another look? [18:47:23] https://gerrit.wikimedia.org/r/#/c/254309/5 https://gerrit.wikimedia.org/r/#/c/254080/ [18:49:21] tgr: +2inated [18:50:29] csteipp_afk: ^ [18:53:35] ori: you mean wikiadmin or wikiuser? [18:53:41] yeah [18:54:28] I presume wikiadmin... as most of the maintenance scripts would need it [18:55:31] I'd look, but gotta go out... [18:55:32] Laters [19:16:13] bd808: What's the status on composer-merge-plugin 1.3.0? [19:16:19] * ostriches has https://phabricator.wikimedia.org/T117059 on the 1.26 board [19:32:09] ostriches: we just need to backport it I think. [19:32:15] \o/ [20:12:49] ostriches, legoktm: yeah I guess just backports. Do we make a "release" branch of vendor or do we just need to add it to the mw/core composer.json? [20:13:04] there is a REL1_26 branch in vendor [20:15:51] cool. I'll make the backport patches then [20:16:54] go team! [20:20:52] ostriches: I'm doing https://phabricator.wikimedia.org/T115658 right now, should I document things if they were backported to a 1.25.x release? e.g. https://gerrit.wikimedia.org/r/#/c/231980/ [20:22:16] AaronSchulz: any suggestions for creating the event representing changes to revision visibility? [20:22:40] ArticleRevisionVisibilitySet is the only (obvious) hook I see, and it doesn't have information about the restrictions themselves (which this event is supposed to contain) [20:23:01] (the (tentative) schema is here: https://github.com/d00rman/restevent/blob/6a4b7180698d3707ceec14656c5d84aabb47b05b/schemas/mw-revision-visibilityset.yaml) [20:26:16] legoktm: Nah [20:34:25] urandom: you can add a parameter to the hook invocation in core [20:39:52] ori: ok; yeah that's what i'm looking into now, thought i'd ask though in case there were a Better way [20:41:25] ostriches: https://gerrit.wikimedia.org/r/254474 [20:43:49] tgr: So to deploy that, I need to cherry pick both patches back to wmf7, right? [20:46:19] Oh, do we run tests on deployment branches? [20:48:52] yes [20:50:49] csteipp: yes [20:50:58] to both questions [20:51:38] So will jenkins eventually look at https://gerrit.wikimedia.org/r/#/c/254480/ ? [20:52:28] it just -1'd it [20:53:32] the other patch needs to go first [21:03:57] legoktm: I did https://gerrit.wikimedia.org/r/#/c/252386/ the other day [21:04:09] In service of helping us bundle less pear crud to support mailing [21:09:01] greg-g: So.. can I deploy the two patches from tgr to fix T119021 for James A? I'm pretty sure enough people have looked at it that there is fairly low risk of anything going wrong. [21:09:36] csteipp: doit [21:13:52] csteipp: tgr if https://gerrit.wikimedia.org/r/#/c/254480/ depends on another patch, maybe you can try using Depends-On: [21:14:07] csteipp: tgr in theory Zuul/Jenkins/CI should inject the dependency even if not merged [21:14:33] Oh, that would have been very cool. [21:15:05] i announced it yesterday on wikitech-l / engineering list [21:15:32] * csteipp sheepishly admits to deleting a lot of emails lately... [21:15:32] Depends-On has barely been used in our community yet :/ [21:15:54] hashar: Do I give it a change-id? or gerrit patchset? [21:16:25] Ah, I see the email [21:16:41] a change-id [21:23:03] csteipp: so if you look at https://integration.wikimedia.org/zuul/ [21:23:09] under *test* [21:23:19] your change https://gerrit.wikimedia.org/r/#/c/254480/ [21:23:27] Oh, there it is [21:23:32] is marked as dependent of 254483,1 [21:23:52] and at the top of https://integration.wikimedia.org/ci/job/mediawiki-phpunit-hhvm/19691/consoleFull [21:24:02] there is a command name zuul-cloner [21:24:07] it fetch the patches from zuul server [21:24:18] 00:00:19.366 INFO:zuul.Cloner:upstream repo has branch wmf/1.27.0-wmf.7 [21:24:18] 00:00:19.675 DEBUG:zuul.Cloner:Fetched ref refs/zuul/wmf/1.27.0-wmf.7/Zadb103cb68904a6b8c4e14ba060adf0f from mediawiki/core [21:24:20] 00:00:19.681 DEBUG:zuul.Repo:Checking out bbec74d8839379135db8d7c21f7bf10bfb7fb721 [21:24:20] 00:00:20.182 INFO:zuul.Cloner:Prepared mediawiki/core repo with commit bbec74d8839379135db8d7c21f7bf10bfb7fb721 [21:24:32] the ref has been prepared by Zuul server [21:24:43] it is a merge of the dependent patch + your patch on tip of o wmf/1.27.0-wmf.7 [21:24:54] so the job run AS IS both patches got sent one after the other [21:25:43] csteipp: though in this case both patches are against mediawiki/core so you could just have chained the commit and rely on Gerrit dependency [21:26:02] but if you had a dependent patch in another repo ... Depends-On: is a good workaround [21:26:19] Yeah, that's very cool. Thanks for the heads up :) [22:01:00] WMFers: reminder that today is the last day to respond to the engagement survey [22:02:51] hashar: so what happens when something with Depends-On is cherry-picked but the dependency is not? does Zool V-2 it? [22:02:51] Krenair: thanks for the AhoCorasick stuff [22:03:24] tgr: it runs the tests [22:03:39] tgr: and craft some magic references to get the dependency to be fetched by zuul-cloner [22:04:03] tgr: if your CR+2 a change that has depends-on but the dependency is not merged / not in the gate-and-submit [22:04:08] tgr: then zuul does nothing [22:04:33] tgr: as soon as the dependency receive a +2 the +2ed patch that depends on it will be magically added to gate-and-submit [22:04:40] I don't see how that's even possible when the dependency is in the same repo [22:04:42] at least, on paper [22:04:54] does Zuul rewrite history? [22:05:10] not really [22:05:16] it anticipate on the future state of the repository [22:05:21] lets take an example [22:05:48] mediawiki/core has two patches set A and B both having for parent the same commit which is the tip of master [22:05:55] so you have: master <-- A [22:05:58] master <-- B [22:06:05] now you mention that B Depends-On: A [22:06:25] you CR+2 B. Zuul does nothing because A is neither merged nor in the gate-and-submit [22:06:32] you then CR+2 A [22:06:52] Zuul craft a commit that is a merge of master + A, run jobs for A that test that [22:07:10] and immediately it also craft a merge change which is master + A + B and run jobs for B [22:07:22] but B really is tested AS IS A already got merged [22:07:33] so essentially B is tested as is A already merged [22:07:43] and if A and B jobs pass, they are merged almost at the same time in sequence [22:08:12] so this only happens when B is +2-ed? [22:08:13] resulting history in the repo is : master <- A <- B (with B commit time being only a few seconds after A ,i.e. not 10 minutes or so) [22:08:19] until then A cannot be tested? [22:08:32] well A is independent [22:08:48] so it can be tested however you want [22:09:03] and B (which depends on A) can still be tested even if A is not merged yet [22:09:17] sorry, I meant B [22:09:23] because when you propose the patch B, Zuul test it as if A already got merged (just like when you CR+2) [22:09:42] lets do it again. [22:09:46] You propose a patchset A [22:09:54] then propose a patch B depending on A [22:10:18] Zuul enqueue it in the test pipeline, craft a merge commit which has A + B and run the tests for B as if A has already been merged [22:10:33] that's pretty slick [22:10:44] but on the same repo, it is probably better to just have a chain of commit : A ---> B [22:11:01] which works pretty nicely in Gerrit when you are on the same repo [22:11:14] the zuul thing basically extend it to support different repos / multiple patches [22:11:27] so in theory you can have an octopus or a graph of dependencies [22:11:50] sounds like Depends-On is slightly better in that you cannot accidentally cherry-pick only half of a dependency [22:12:16] but I am not sure how it works with patches cherry picked to multiple branches [22:12:32] if you have change A merged in mediawiki/core @master [22:12:43] then change B that depends-on: A and get merged [22:12:49] if you cherry change B to a wmf/branch [22:12:53] I am not sure what happens [22:13:11] most probably, Zuul consider the dependency A as merged, since it is present in @master [22:13:51] tgr: the whole http://docs.openstack.org/infra/zuul/gating.html is an interesting read (disclaimer I wrote part of it) [22:25:05] tgr: gotta sleep sorry. Feel free to follow up on wikitech-l [22:54:28] https://github.com/apache/avro [22:54:31] mirrored from git://git.apache.org/avro.git [22:54:42] Is that a github feature if you use github to do the syncing? [22:54:50] Rather than our way of pushing to github? [23:07:15] Reedy: I think it is a special thing you have to have the GitHub side setup. [23:07:27] Shame, it looks kinda cool/useful [23:07:32] ie "this isn't the canonical repo" [23:07:41] * bd808 vaguely remembers looking this up at some point [23:08:05] heh [23:08:16] I see various uses of long in the avro-php code... [23:08:16] https://help.github.com/articles/about-github-mirrors/ [23:08:24] should they just be int? [23:09:04] probably, yes. That code is a train wreck. [23:09:16] Just a bit [23:09:25] I wonder if we should just get an IDE to reformat it all while we're at it [23:09:44] Don't care spaces/tabs etc, but I don't think they're even consistent with the amount of spaces they use [23:09:58] I was thinking about that or running php-cs-fix on it [23:10:12] I couldn't care either way [23:10:26] Just inconsistency gives me an ocd esque tick [23:10:39] looking at poorly organized code makes me itch :/ [23:10:50] Is it auto generated from some other language or something? [23:11:39] the 'mirrored from' requires manual intervention from github staff [23:11:51] and apparently they are no longer accepting such requests [23:11:54] I couldn't find another one lang that looked similar but it does have a generated feel in a lot of places [23:11:54] Yeah, that's what the page bd808 linked essentially says [23:13:25] https://gerrit.wikimedia.org/r/#/q/status:open+topic:rel-notes,n,z bunch of easy RELEASE-NOTES fixes [23:18:29] legoktm: Will it merge? [23:18:29] xD [23:19:49] csteipp: what do you mean by a safe way to run composer? [23:19:53] (in https://gerrit.wikimedia.org/r/248661) [23:19:59] /** [23:19:59] * @var long lower bound of long values: -(1 << 63) [23:19:59] */ [23:19:59] const LONG_MIN_VALUE = -9223372036854775808; [23:19:59] /** [23:20:01] * @var long upper bound of long values: (1 << 63) - 1 [23:20:03] */ [23:20:05] const LONG_MAX_VALUE = 9223372036854775807; [23:20:10] lol. [23:20:44] tgr: https://github.com/Stype/packagistproxy [23:21:41] csteipp: I think legoktm was planning on using that project as a "feature" to add a packagist cache to Labs :) [23:21:52] Yeah.... [23:22:23] * bd808 is not endorsing it as a great idea [23:24:34] Is gerrit on a go slow? [23:28:05] csteipp: if you care about security you can use https://www.mediawiki.org/wiki/Special:ExtensionDistributor/OAuthAuthentication [23:28:39] which does use composer in the background, but MITM-ing a labs->packagist.org connection is a lot harder than my laptop->packagist.org [23:29:34] or is your concern that the "standard" way of installing the extension (Vagrant and such) becomes insecure? [23:30:13] because I don't think vendor changes that, mw-vagrant always prefers composer [23:32:34] Reedy: they better...I spent an hour putting it all together... [23:34:31] tgr: Part of my concern is just selfish-- I use git, and I want to be able to keep using and developing that extension, and I will not use composer in my normal workflow... [23:35:10] Since the extension install instructions say to use git, last I checked, I'm guessing that's what most people use? [23:48:05] well, the instructions just say "Download the extension into your extensions directory" [23:49:28] is there a policy for what can be added to mediawiki/vendor? [23:49:46] I'm all for adding it there if that helps someone's workflow [23:49:49] code that we are using in WMF production/beta [23:51:10] looks like I'll have to come up with some justification to deploy it first :) [23:52:17] which extension are we quibbling over? [23:52:31] OAuthAuthentication [23:55:02] csteipp: so ... what do we need to do to fix composer? Do we have an open phab task that gives the criteria you would need to see satisfied? [23:56:09] bd808: composer just needs a flag to use https, and validate the cert, when getting the first package.json from packagist.org [23:56:47] That sounds possible to do upstream [23:57:40] I'm sure they would want the default to stay permissive but a global config setting and cli flag shouldn't be too difficult [23:57:53] unfortunately, https://github.com/composer/composer/issues/1074 seems like they are blocking it on the grounds that it's too hard. [23:58:05] Maybe, yeah