22:59:25 <ahf> #startmeeting network team code style meeting 13. february 2020
22:59:25 <MeetBot> Meeting started Wed Feb 12 22:59:25 2020 UTC.  The chair is ahf. Information about MeetBot at http://wiki.debian.org/MeetBot.
22:59:25 <MeetBot> Useful Commands: #action #agreed #help #info #idea #link #topic.
22:59:30 <ahf> hello network-team
22:59:33 <ahf> who is here? :-)
22:59:37 <nickm> hihi
22:59:43 * nickm is here with bells on
22:59:50 <nickm> assuming that bells conform to our latest style guidelines
23:00:00 <dgoulet> hello, I'm able to make it this time!
23:00:07 <catalyst> o/
23:00:10 <dgoulet> (but the little one is running around me as well :P)
23:00:18 <ahf> it is the follow up for the meeting last week at 23 where we spend some time talking about code style
23:00:23 <ahf> but we didn't find the next steps i believe
23:00:48 <nickm> https://trac.torproject.org/projects/tor/ticket/32921 is the most active ticket here, but it has become mixed in its purpose
23:00:49 <ahf> one discussion item was around if (! x) and the if (! ! x) i think?
23:01:13 <nickm> my original purpose with that ticket was to resolve items on our code today that block running clang-format.  But it has also become a discussion of what works or doesn't work about clang-format.
23:01:47 <nickm> https://trac.torproject.org/projects/tor/ticket/32921#comment:20 has a list of issues noted by catalyst; teor follows up immediately below
23:02:10 * ahf looks
23:03:46 <ahf> ok
23:04:16 <ahf> is catalyst and teor here today? i think they are both important for this topic as they have been part of the discussion
23:04:23 * catalyst intermittently around; cats are being extremely naughty
23:04:25 <nickm> For an example of running clang-format on our current code see demo_clang_format_2020_02_12
23:04:38 <nickm> it does not go to 4-char indents yet, however, to make a diff easier.
23:04:46 <ahf> i'm on the "yay, tooling to enforce style"-team and less on the team that have strong opinions about the style itself
23:04:50 <nickm> i can make an example that does 4-space indents if people would like to look at one
23:05:02 <nickm> teor was on #tor-dev a moment ago; I expect they'll be here soon
23:05:08 <ahf> cool
23:05:33 <ahf> it is some well-spotted found in that ticket
23:06:19 <ahf> so, which parts is holding us back right now from next step? is it to make a choice on which form we want on those specific items, or?
23:07:11 <nickm> I don't know personally.
23:07:16 <nickm> I don't think we agree on next steps
23:07:29 <teor> I am here, and up to date with the backlog
23:07:30 <catalyst> maybe we can try to agree on what this ticket is _not_?
23:08:03 <ahf> so i think if the goal is to reach consensus on what a "good style" is then i doubt we'll get there. i think getting to the point where this becomes a part of folks workflow will *much* easier allow us to tune smaller things as we go?
23:08:08 <nickm> [for 4-char indents  see demo_clang_format_wide_2020_02_12]
23:08:40 <nickm> catalyst: by "this ticket" do you mean #32921 or #29226?
23:08:57 <teor> ahf: +1, I don't actually think style bikeshedding should be our focus at the moment
23:09:10 <catalyst> i thought we were talking about #32921
23:09:17 <teor> There's a bunch of technical challenges around actually getting the reformatting working:
23:09:35 <ahf> ok?
23:09:40 <teor> * where do we reformat? git hooks? editors? CI?
23:10:03 <teor> * which version of clang-format do we use?
23:10:26 <ahf> my intuiton says folks finds what is best for them (editor integration is what i will do on save-to-buffer) and then enforce it in CI (no-diff-when-applying-style-checks)
23:10:28 <teor> * how do we merge forward into a reformatted master?
23:11:13 <dgoulet> oh that last point is a good one ^ ...
23:11:14 <teor> I think CI enforcement will make a lot of once-off contributors unhappy, and less likely to help out
23:11:44 <teor> Overall, if we can't solve these challenges, it doesn't matter what style we pick
23:11:47 <ahf> without CI enforcement people will be submitting patches that might fix things that have landed by mistake?
23:11:55 <ahf> right
23:11:57 <ahf> agreed
23:12:30 <teor> I think all these challenges are solveable, but they have impacts on the specific style we can actually use
23:12:47 <atagar> (For what it's worth with Stem I found that enforcing code conventions by running pycodestyle as part of the tests put the topic of style to rest. Pull request discussions are simple (code is only merged if the tests pass). This has saved me a lot of time where previously folks would argue about minor stylistic preferences.)
23:13:15 <teor> For example, if we want to verify in CI, we are restricted to available clang-format versions
23:13:32 <gaba> hi! sorry that i'm 1/10 here today
23:13:32 <teor> And clang-format versions support different style options
23:14:10 <ahf> teor: we can always build and fetch a version we want there if we want something newer than what is in the image if that is the concern you are thinking of?
23:14:14 <catalyst> requiring developers to run clang-format isn't great either. i do most of my development in macOS and clang-format doesn't want to install for me out of homebrew for some reason
23:14:39 <ahf> hm, i don't get how we could do this without having developers run it?
23:14:43 <nickm> IMO, if we have a style tool and a means to run it regularly, it isn't so bad if some patches to our code onl approximate the official style
23:14:46 <nickm> *only
23:14:53 <teor> We could reformat PRs before merging
23:15:20 <ahf> i'd be okay with that
23:15:20 <nickm> we could have a cron job reformat at noon UTC  daily :)
23:15:47 <teor> #worksforme, I'm rarely coding at that time
23:15:50 <ahf> yeah, i guess that would slowly over time have less and less to do as more folks get it right with writing and/or by tuning their editors
23:16:15 <catalyst> i think repeatedly reformatting on a regular basis once we've committed the big reformat isn't nice to contributors
23:16:29 <nickm> I don't think so, if it's to the same style, and the changes are small?
23:16:59 <nickm> That is, if the only stuff that can change at noon UTC is stuff that became nonconformant over the last 24 hrs...
23:17:05 <ahf> but it will only be reformatting of what hasn't been reformatted properly, no?
23:17:14 <nickm> it would have to be for that idea to work
23:17:32 <nickm> IMO we should "pull the big switch" exactly once.
23:18:05 <catalyst> if you mean a daily job that automatically pushes commits that reformat non-conforming code that we've already pushed, i think that creates a lot of noise and we shouldn't do it
23:18:08 <ahf> i don't think we can have a setup where developers aren't doing it AND CI enforces it, OR we have a system where mergers do it, OR developers do it on a best-by-best basis and we have a cron job that applies to the tree IFF there is a diff?
23:18:43 <nickm> To a certain extent, if we have autofomatting, we can be _more_ tolerant of bad formatting in our CI: we don't need to insist on people getting the format exactly right.  Check-spaces can get smaller in this view.
23:18:56 <ahf> yeah
23:19:09 <nickm> catalyst: I think that reformatting at merge time is a good idea, but that doing a regular cleanup of nonconforming code is better than leaving it
23:19:20 <teor> Can't we just reformat master and the PR during every merge? Isn't that the easiest way?
23:19:31 <teor> We can automate that, we do merges using scripts.
23:19:44 <nickm> We could do a hook for mergers that won't push if the formatting is wrong.
23:19:47 <catalyst> we can make CI style checks more lenient on PRs than on master and release branches
23:19:55 <nickm> +1 there
23:20:04 <nickm> (to catalyst's lenient CI checks on PRs)
23:20:26 <ahf> what does lenient mean?
23:20:29 <nickm> tolerant
23:20:31 <ahf> ah
23:20:47 <catalyst> i'm still wondering if we agree about the scope of #32921
23:21:04 <nickm> catalyst: it may be that we don't.
23:21:09 <nickm> What do you think the scope is?
23:21:22 <teor> Whatever strategy we choose, we can start implementing it right now with "make autostyle", so we find any issues before the big refactor
23:21:27 <teor> *reformat
23:21:54 <dgoulet> My two cents: I would personally not make auto-commit reformat, it would clobber our git history, make bunch of branches based on "misformatted" code to fail to merge properly. I think C format should be done by the dev and enforced by the CI. And we can help contributors by providing tooling.
23:22:23 <nickm> auto-commit?
23:22:40 <dgoulet> autoformatting as a cron idea?
23:22:49 <nickm> ah.  yeah, that idea is not so great
23:23:10 <nickm> I think that trying to get everybody's editor style as close as possible to official, plus doing a restyle-before-push is a better choice
23:23:10 <catalyst> i think we can defer a lot of these decisions if we agree to a narrow scope for #32921
23:23:31 <dgoulet> nickm: agree
23:23:42 <nickm> catalyst: I had in mind for #32921 the scope that it starts adding tools and clang-format scripts, but that those are not considered to be final.
23:23:46 <dgoulet> CI should be our last defense and nothing should get merged if not formatted properly imo
23:24:00 <nickm> That is, the next step after #32921 is not "declare the style set and start figuring out how to enforce it"
23:24:21 <nickm> catalyst: would that be okay with you?
23:24:50 <catalyst> nickm: i think it would help to have that be explicit in the description in the ticket. maybe some other things too
23:25:57 <catalyst> there are a few config choices for clang-format that have larger impact than agreeing on style. minimum required clang-format version is one. workarounds in scripts. workarounds in C code. etc.
23:26:03 <ahf> start adding tools and scripts is that landing them to tor.git master?
23:26:51 <nickm> Okay. I've added some text to the ticket description
23:27:08 <nickm> any more I should specify there?
23:28:37 <nickm> ahf: right.
23:28:51 <ahf> cool
23:29:46 <nickm> catalyst: do you think making those choices is in-scope for #32921? I had not thought so, at least to the extent that we can change things afterwards.
23:30:03 <nickm> err that is
23:30:07 <ahf> so, we think it is possible to do this iteratively, where we land something that might not be perfect for all of us, but is something where we can tune it as we go?
23:30:15 <nickm> not exactly that
23:30:35 <ahf> ok?
23:30:37 <nickm> I'm hoping that we can get the formatting tools and clang-format scripts into tor-git, and iterate upon them there before we "turn them on"
23:30:54 <ahf> ah, close. that is cool
23:31:05 <dgoulet> +1
23:31:26 <nickm> we should not commit the reformatting of any code until we all like how the tools are working and their output is looking.
23:31:30 <ahf> i think david and i, as said last time, is probably gonna experiment with getting this into onion-vim and then we'll have to do some iterations there, once it all lands
23:31:37 <ahf> agreed
23:31:44 <dgoulet> +1
23:31:52 <catalyst> i will say i'm still a little worried that clang-format is too agressive and it's too hard to tell it to selectively ignore some things, rather than choosing among choices where none agrees with existing practice
23:32:19 <nickm> catalyst: do we have a better option than clang-format IYO?
23:32:24 <nickm> including the status quo
23:32:56 <ahf> i looked a bit at some projects on github that does auto-styling and all of the C and C++ projects i found used clang-format
23:33:15 <catalyst> nickm: last time i was faced with this problem, i unfortunately concluded that declaring a specific emacs C style and running a batch elisp script was the least bad alternative. not sure if that's changed
23:33:23 <nickm> I have also used uncrustify before.
23:33:36 <nickm> It is ... hard.
23:34:06 <nickm> I would not be horribly upset by having emacs as a required part of our dev tool chain, but I'm not sure how that would fly with everybody.
23:34:25 <ahf> i think if the alternative is for us to maintain a tool ourself, then i think we are entering a place where it's a bit outside of what we should be doing?
23:34:43 <ahf> ... is this some sneaky plan to get us all to use emacs? :-P <3
23:34:47 <catalyst> i wouldn't be thrilled by making emacs required either, but it's what i'm familiar with
23:35:37 <ahf> i think the idea of a homebrewed tool, whether it is with emacs, or python, or vim is not a good choice in the long run compared to something that my stomach feeling says more and more are moving to?
23:35:53 <catalyst> for what it's worth, i found that NetBSD had a project to make clang-format produce their variant of KNF. they only had to write code to add a small number of new formatting options
23:35:58 <ahf> i'd rather see the thing we talked about last week then with use clang-format + some tool that corrects the few issues we have with it
23:36:28 <catalyst> (one of the NetBSD changes to clang-format had to do with alignment of tabular stuff, as i recall)
23:36:34 <dgoulet> so clang-format is a set of config vs emacs we would need a custom script right?
23:36:39 <nickm> catalyst: that won't work for us if we're also insisting that we need to use old versions of clang-format
23:37:05 <nickm> dgoulet: either way we need at least some scripting. There are things clang-format does that I believe are not what we want, so my current candidate branch adds a postprocessing script
23:37:09 <teor> We can also move more tables to .inc files, like we're doing with config options
23:37:31 <nickm> or wrap them in //clang-format:off //clang-format:on
23:37:32 <catalyst> yeah, i'm mostly pointing it out as a "clang-format is almost good enough to produce KNF without postprocessing" data point
23:37:36 <nickm> (or whatever the syntax is)
23:37:59 <nickm> one thing that our postprocessor does is to replace this:
23:37:59 <ahf> catalyst: so the question is if we want KNF or not or?
23:38:00 <nickm> }
23:38:03 <nickm> SMARTLIST_FOREACH_END(x)
23:38:06 <nickm> with this:
23:38:12 <nickm> } SMARTLIST_FOREACH_END(x)
23:39:00 <catalyst> ahf: i think we don't want exactly KNF, but something as close as we can reasonably get within our historical practice and other constraints
23:39:27 <ahf> oki, i don't know KNF well enough to know where we differ, but it is also not the place i have strong opinions about
23:39:49 <dgoulet> ok I'm a bit lost to be honest. Nick has done lots of work with clang-format + custom script... so why is this not good enough ?
23:39:58 <ahf> nickm: is that what we have of post-processor right now?
23:40:16 <ahf> like is that the only thing it does or do we do a few other things?
23:40:19 <nickm> https://blog.netbsd.org/tnf/entry/gsoc_2019_report_adding_netbsd1 is what I see of the netbsd clang-format work
23:40:26 <nickm> ahf: I'll check...
23:40:39 <ahf> no, i can check it's fine
23:40:43 <ahf> it was just if you knew from top of your head
23:40:43 <catalyst> nickm: yeah i think that's what i found
23:40:51 <nickm> ahf: it also breaks MOCK_IMPL() so that ctags can work with it.
23:40:56 <nickm> and that's it for now
23:41:02 <ahf> ah
23:41:10 <nickm> it we decide to have it justify our \ continuations, it can
23:41:33 <ahf> what happened to the !! from last meeting? the post-processor was also up for discussion there?
23:42:37 <catalyst> i think we want to drop SpaceAfterLogicalNot:true because that bumps up our minimum required clang-format version to 9.0
23:42:56 <nickm> i already dropped that
23:43:03 <nickm> I don't care strongly about it
23:43:25 <ahf> ok, cool
23:43:26 <catalyst> so the two clang-format config choices that have a nontrivial influence on minimum required clang-format version are SpaceAfterLogicalNot and StatementMacros, and the current branch already dropped those
23:44:10 <ahf> that is good
23:44:22 <catalyst> there's still the problem that i think we require a minimum of clang-format 6.0 (maybe lower, but definitely newer than 3.8), so we should have a way to parameterize the executable name
23:44:25 <ahf> i just checked and my dev env has clang-format 8
23:44:57 <nickm> catalyst: but that doesn't block #32921, does it?
23:45:04 <catalyst> though if we're not requiring people to run it yet, we can paramaterize it with a different ticket
23:46:36 <ahf> i don't think the version blocks #32921?
23:46:40 <nickm> If we think there's a good chance we'll want to completely discard clang-format, then we should back off from #32921.  But if it looks like we're probably going to do that and not eg uncrustify or emacs
23:47:14 <nickm> then IMO #32921 is not wasted work
23:47:18 <ahf> yes, that is how i see it too. to me it's clang-format with some iterations on finding out something nice OR we wait to a later point in time to do this again
23:47:37 <nickm> the iterations should be done in formatting and tools though, not in the codebase
23:47:44 <ahf> yes
23:47:51 <nickm> we shouldn't do an intrusive reformat the whole codebase more than once
23:47:56 <catalyst> ahf: yeah i think we can say the versioned clang-format executable name doesn't block #32921
23:48:19 <ahf> agreed (to both)
23:48:36 <ahf> 10 min. left - what is the next step then just so folks who are not here also gets the next step
23:48:38 <catalyst> i would like a prominent comment in .clang-format that it's not final and not to push code where it's been run until separately agreed
23:48:40 <nickm> teor: I like your idea of adding some kind of autostyle thing to the pre-push hook for master. Would you be interested in helping me figure tha tout?
23:48:48 <nickm> *that out
23:48:55 <ahf> catalyst: good idea
23:48:55 <nickm> catalyst: can do
23:50:31 <nickm> HOw is this disclaimer:
23:50:37 <nickm> # DO NOT COMMIT OR MERGE CODE THAT IS RUN THROUGH THIS TOOL YET.
23:50:38 <nickm> #
23:50:38 <nickm> # WE ARE STILL DISCUSSING OUR DESIRED STYLE AND ITERATING ON IT.
23:50:50 <ahf> add a date plz
23:51:04 <ahf> so people can see this was added in february 2020
23:51:08 <catalyst> nickm: sounds good
23:51:15 <ahf> other than that sounds good IMO
23:52:01 <catalyst> also i think we might be close to converging on actual style choices already anyway?
23:52:11 <teor> nickm: yes, I think we could do an autostyle in pre-commit or pre-push
23:52:21 <nickm> so next steps are to finish review + sign off on #32921, then propose and discuss tooling and style changes?
23:52:39 <ahf> catalyst: i think so too
23:52:51 <ahf> with the space before logical not out was a big one folks disagreed about i think
23:53:12 <ahf> nickm: +1, with the hooks in mind as tooling?
23:54:19 <catalyst> yes, if tooling includes making it easier to run the minimum required clang-format where the default executable isn't the minimum (which doesn't seem too hard)
23:54:36 <nickm> right
23:54:50 <ahf> yep
23:54:53 <nickm> I think we will have even more necessary tooling that we don't know is necessary yet
23:55:01 <ahf> yes
23:55:13 <nickm> I think we should be open to realizing our approach needs more to work
23:55:38 <nickm> I don't want to stampede towards a solution; I just want to get to a point where we can experiment with one more cormfortably
23:56:13 <nickm> Is anybody -1 on teor's idea of pretending that our current "make autostyle" is a mandatory code transformation tool, and getting our hooks etc to enforce it as painlessly as possible?
23:56:18 <ahf> yeah, agreed
23:56:25 <nickm> (I'm +1 on it)
23:56:36 <ahf> sounds good with make autostyle
23:56:46 <ahf> (sounds like another GNU autotool 8)
23:57:07 <dgoulet> +1
23:57:13 <catalyst> i'm not sure what all autostyle does already? but it doesn't seem too intrusive at the moment
23:57:38 <teor> It mainly fixes header paths and macro end comments
23:57:52 <teor> At least, those are the things we tend to miss when we manually code
23:57:57 <nickm> i just ran it and produced d0c3350218765d43c538df4dd1d548fa9a7c430a
23:58:20 <nickm> i think it  also adjusts our copyright dates once a year
23:59:07 <ahf> cool
23:59:11 <ahf> i think we have s55 meeting now
23:59:17 <ahf> so i have to close this off
23:59:27 <ahf> thanks all for joining and good that we found out some next steps!
23:59:39 <ahf> #endmeeting