"Dangerous to submit" dialog triggered in chromium sub-projects |
|||||
Issue descriptionAffected Version: 2.13.3-1801-g906c470 What steps will reproduce the problem? 1. Click submit for a Crashpad CL while Chromium tree is closed 2. Get [The Chromium tree is closed, submitting this change is dangerous. Submit anwyay?] What is the expected output? No warning for projects that aren't Chromium (and maybe aren't autorolled?)
,
Dec 7 2016
,
Dec 7 2016
,
Dec 13 2016
Can you point to a CL where this message popped up? None of the chumpdetector configs match the 'crashpad/crashpad' project name: https://chromium.googlesource.com/All-Projects/+/refs/meta/config/chumpdetector.config And I don't see the tree status box showing up when I click through to any of these reviews: https://chromium-review.googlesource.com/q/project:crashpad%252Fcrashpad
,
Dec 13 2016
Hmm, I forget which CL it was on now, shoot. ... Aha, maybe mini_chromium? https://chromium-review.googlesource.com/c/417285/ I guess it seems more reasonable that it wouldn't be obvious based on that name now. mini_chromium is a subproject of Crashpad though.
,
Dec 13 2016
Ah yes, that would make sense. The chumpdetector regex definitely matches for all "chromium/*" repos, and makes them subject to the chromium tree status. I was previously unaware of the existence of mini_chromium. It hasn't been officially migrated to PolyGerrit like the other crashpad repos. Its ACLs don't inherit from crashpad/. And if it is a crashpad project, why is it in the chromium/ namespace? Personally, I think that the most appropriate way to resolve this would be to move the repo to crashpad/mini_chromium, which would resolve all of the above questions in one fell swoop. That may be easier or harder depending on where (if at all) it shows up in DEPS files of other projects.
,
Dec 13 2016
Uh, ok. I don't know the answer to any of those questions. Maybe Mark does. Or maybe I can just ignore the problem and click through the dialog.
,
Dec 13 2016
mini_chromium is supposed to be usable by more than just Crashpad. We’ve had several would-be consumers knock on our door and we offer them our code and friendship. It hasn’t come to fruition for anyone else for political reasons, which is unfortunate. But the hope is still there. It’s not really a Crashpad project, but the Crashpad team does the maintenance for mini_chromium. mini_chromium shouldn’t be subject to the chromium-status tree closures. mini_chromium’s Gerrit ACL is crashpad-owners/crashpad-committers.
,
Dec 13 2016
Responses inline > mini_chromium is supposed to be usable by more than just Crashpad. We’ve had several would-be consumers knock on our door and we offer them our code and friendship. It hasn’t come to fruition for anyone else for political reasons, which is unfortunate. But the hope is still there. Cool. To me, that doesn't affect where it should live, since it's all on chromium.googlesource.com anyway. But I understand why you'd want to have it in the bigger namespace. > It’s not really a Crashpad project, but the Crashpad team does the maintenance for mini_chromium. > mini_chromium shouldn’t be subject to the chromium-status tree closures. Ok. > mini_chromium’s Gerrit ACL is crashpad-owners/crashpad-committers. Ish. It's gerrit acl names those groups, but its gerrit acl isn't correctly set up for full review the way that crashpad/crashpad is. I will make its gerrit acl inherit from crashpad/, even though it is not in that namespace. So my two tasks are: * Update the ACLs to match crashpad * Figure out how to turn chumpdetector *off* for just a single project. It's going to be ugly regexes (especially if someone else in the chromium/ namespace wants the same thing later...), and I will probably cry a little.
,
Dec 20 2016
So this is happening in two parts: * I've updated mini_chromium to inherit ACLs from crashpad: https://chromium.googlesource.com/chromium/mini_chromium/+/ca3939e63ad9fe3f4e6fb94798f54196b0babff1 * I'm changing the way the chumpdetector plugin works (https://critique.corp.google.com/#review/142497363) so that it is configured on a per-project basis, and inherits configuration the same as ACLs. Since crashpad doesn't configure a chumpdetector, neither will mini_chromium. Once that plugin lands and is deployed, I'll make a whitespace change to verify.
,
Jan 5 2017
Update: CLs have landed but not been deployed. Probably have to wait a week for latest version of chumpdetector to be rolled out.
,
Jan 10 2017
The chumpdetector is now disabled on canary, and will be disabled on prod when the current rollout is complete. You can see it (not) in action here: https://canary-chromium-review.googlesource.com/c/424847/ https://canary-chromium-review.googlesource.com/c/414408/ The configuration necessary to disable it is shown here: https://chromium.googlesource.com/crashpad/+/1c164945a95efdb272731f3398ef705c7820a9b4/chumpdetector.config That configuration is picked up by other crashpad projects (namely crashpad/crashpad, and chromium/mini_chromium) via acl inheritance. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by andyb...@chromium.org
, Dec 7 2016