New issue
Advanced search Search tips

Issue 671878 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----


Previous locations:
gerrit:5076


Sign in to add a comment

"Dangerous to submit" dialog triggered in chromium sub-projects

Project Member Reported by scottmg@chromium.org, Dec 6 2016

Issue description

Affected 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?)
 
Labels: -Restrict-View-Google
Project: chromium
Moved issue gerrit:5076 to now be  issue chromium:671878 .
Components: Infra>Codereview>Gerrit
Labels: Milestone-Fishfood Proj-Gerrit-Migration Pri-2
Owner: aga...@chromium.org
Status: Assigned (was: New)

Comment 4 by aga...@chromium.org, 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
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.

Comment 6 by aga...@chromium.org, 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.
Cc: mark@chromium.org
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.

Comment 8 by mark@chromium.org, 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.

Comment 9 by aga...@chromium.org, 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.
Status: Started (was: Assigned)
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.
Update: CLs have landed but not been deployed. Probably have to wait a week for latest version of chumpdetector to be rolled out.
Status: Fixed (was: Started)
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