On Wed, Nov 16, 2016 at 10:05 PM, Gregory Farnum <gfarnum@xxxxxxxxxx> wrote: > Recently, Patrick noticed an issue with the MDS TrackedOp > implementation where we weren't actually dumping out the event lists > anywhere. It turned out to be as a result of an incorrect cleanup, and > he fixed it by just reverting the commit in > https://github.com/ceph/ceph/pull/11985. > > But we also wanted to clean up the spurious "now" parameter which goes > on used, and John did that in https://github.com/ceph/ceph/pull/12007. > It was reviewed by Patrick, Kefu, and myself and got merged with green > checkboxes (ie, Github and Jenkins agreed it was good). > > Then Patrick noticed we'd missed the Monitor's use of TrackedOp code > and it was spawning warnings because the relevant _dump() functions no > longer had matching parameter lists > (https://github.com/ceph/ceph/pull/12029). This concerned me since > we'd gotten a pass from Jenkins. > > It turns out, Jenkins assumes the build is good if the build script > returns 0 — it doesn't know anything about the actual text output and > doesn't scan for warnings. > I'm not sure how hard it would be to change things from that end. From > the *compilation* end, Casey suggests the "nicest" thing would be to > set the -Werr flag, which turns all warnings into errors. Since we > like warning-free code, that's definitely appealing to me. > Unfortunately, we've got a problem: boost is notorious for build > warnings. At the moment, I'm only seeing one (repeated many times > over) resulting from use of the "remove_whitespace" iterator > functionality in RGW. Does anybody know a good way to handle that? > > We also have another (seemingly real) warning: the BlueStore > BitAllocator says there are missing return statements in > BmapEntry::operator new [](size_t). I didn't look at the code; maybe > it's failing to detect return statements in an if-else block. But that > should be fixed up as well. > > So, I guess I'll just throw out a few statements and see how people > react to them: > > 1) The Ceph source should build warning-free. If we have warnings from > external code we still trust, we need a way to suppress *only* those > warnings so that issues we introduce can easily be detected. > > 2) Patches that generate new warnings should not be merged. > > 3) We should turn on -Werr just as soon as the existing BlueStore > warning is fixed and the boost warnings can be suppressed. That should > happen quickly. > > 4) If we do that, I guess the existing Jenkins is okay? Yes to all points. John > -Greg > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
