consider using "topic" to chain CLs like CQ-DEPEND when submitting |
||||||||
Issue descriptionAndroid uses Gerrit's topic feature to set up relationships across CLs & different git repos. this way they can submit all CLs simultaneously in a single topic w/out having to annotate the commit message with something like CQ-DEPEND. Gerrit has to be configured with this behavior: https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#change.submitWholeTopic i think that's a Gerrit-instance-wide setting, so we'd need to coordinate all Chromium users. after that, all Submit operations (either through the API or the web interface) would use the topic. https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submit-change https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#submitted-together i don't think this would be too much of a disruption for existing users as topics aren't heavily used, and would help harmonize Gerrit flows between these large projects.
,
Jun 26 2018
it's a feature in Gerrit that we'd turn on. then users, instead of writing manual CQ-DEPEND lines, would set the topic field in their CLs. our CQ bots would need updating to chain this metadata together like it does with CQ-DEPEND today for testing, and then the submit code paths would need a bit of tweaking to be aware of topic submissions (so it wouldn't have to walk all the CLs by hand).
,
Oct 1
This would need to take a fair amount of thinking before we'd want to enable it for Chromium (src.git and its DEPS), because while this feature might be a relatively desirable thing, it's not immediately obvious to me what this implications would be on a Chromium dev's workflow. I think -- after thinking about it for ~30 seconds -- that as long as the chromium CQ had done a dryrun of the newest (leaf) CL in the topic, it would probably be safe to commit everything in the CL at once, because I think that's essentially the same configuration that the CQ would've applied and tested. @tandrii - does that sound right?
,
Oct 1
,
Oct 2
I don't have time to give extended reply, which this deserves (and I also want topics), but basic answer is that we must not yet enable submitWholeTopic host-wide. Given Pri3, i guess CrOS doesn't badly want it, so we have time to discuss this. Dirk: note that there is diff between CLs stack and topic: * CLs in a stack target the same ref&repo. Submitting the whole CL stack is already possible today! * Topics can span multiple refs and repos (hence global config). CLs stack are already supported by Chromium CQ (CQ used by chromium/src, v8/v8 and other chromium-like repos). Submitting CLs stack in CQ is disallowed by default, but can be enabled with cq.cfg and is actively used by a few projects, but this has bisectability implications for chromium/src with today's submit strategy.
,
Oct 18
,
Oct 18
,
Oct 19
hmm, missed the updates on the bug i don't think the Chromium side needs to fully support this as a blocker to enabling it. the Chromium CQ could enforce "don't use topics on Chromium CLs" by refusing to merge any that have the topic field set. that'd train users pretty quickly into not setting the field until the Chromium CQ is ready. that would also allow CrOS to move forward with the feature independently.
,
Oct 19
Teaching browser(chromium) CQ to avoid topics is necessary, but not sufficient. A submit action can be started on any CL of the topic, potentially on a project without CQ. This, each repo which doesn't want topics will need to disallow submits by committers, effectivelyandating use of CQ. This is probably a good thing, but it requires communication, user education, and finally changing actual ACLs. A weaker, but potentially sufficient mitigation might be disabling submit action in UI in chromium Gerrit plugin if topic is set, unless repo opts in for use of topics. It's easier to do, requires no ACL changes or user education.
,
Oct 19
for repos not using a CQ, it really sounds like a PSA is sufficient to call it a day. people will figure it out soon enough.
,
Oct 19
wrt priority, from CrOS's side, it's not super important because we already have this CQ-DEPEND framework that mostly works well enough most of the time. it might be better as a Pri-2 as it'd make our CI more reliable when it comes to submitting CLs.
,
Oct 19
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/chromite/+/b14af659553ae9b2ccc5befcce39eea62a71a7a0 commit b14af659553ae9b2ccc5befcce39eea62a71a7a0 Author: Mike Frysinger <vapier@chromium.org> Date: Fri Oct 19 22:07:51 2018 cbuildbot: reject CLs using topics for now Since Gerrit has specific meanings for the Topic field (which we have disabled currently), reject CLs trying to use it until it works. They can use hashtags to organize/group their own CLs in the meantime. BUG=chromium:852823 TEST=precq passes Change-Id: I9cd32081e898fd3d1ab9f2ee17e6c6a8d60d64f7 Reviewed-on: https://chromium-review.googlesource.com/1290649 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Lann Martin <lannm@chromium.org> Reviewed-by: Don Garrett <dgarrett@chromium.org> [modify] https://crrev.com/b14af659553ae9b2ccc5befcce39eea62a71a7a0/cbuildbot/validation_pool.py [modify] https://crrev.com/b14af659553ae9b2ccc5befcce39eea62a71a7a0/lib/patch_unittest.py [modify] https://crrev.com/b14af659553ae9b2ccc5befcce39eea62a71a7a0/lib/patch.py
,
Oct 19
I've seen CQ-Depend used before as a signal when bisecting the kernel (to identify which range of CLs to skip, since they're likely to fail by themselves), it would be much harder to use the topic (since they're not visible in the git log) to make such decisions. While this topics feature is kind of neat, I don't think we should completely replace CQ-Depend with it (unless it adds similar tags in the commit message).
,
Oct 20
people can still use CQ-DEPEND in their commit messages if they want docs. but to be clear, CQ-DEPEND is an artificial construct with zero guarantees of atomicity or transaction behavior ... that means if you have a lot of CL's in a CQ-DEPEND loop and Gerrit is being slow to submit, it could take minutes before they're all merged, assuming Gerrit doesn't reject/flake some of them in the middle and leave the repos in an inconsistent state (we've seen both problems in the past). Gerrit topics on the other hand guarantee that, across multiple repos in a single GoB instance, CLs with the same topic will be simultaneously submitted altogether or none will be submitted at all. which is what we need. so, short of any other technical solution, we need & will recommend people use topics when possible.
,
Oct 26
Mike, I have been using "topic" just to make it easier to find a collection of CLs that all fix the same bug or are needed to implement a feature. e.g.: https://chromium-review.googlesource.com/q/topic:%22atlas_1Mhz_I2C%22+(status:open%20OR%20status:merged) Right now I can't even run a trybot on those due to this recent change. Can you provide documentation or an example of how to properly use hashtags for the same purpose? Can I use hashtags with gerrit command line? e.g. what is the replacement for this: gerrit search 'is:open branch:chromeos-4.4 topic:atlas_1Mhz_I2C' I have a list of CLs to operate on and will clear the topic for those. But I won't be able to build another list with gerrit command until I understand the alternatives. TIA!
,
Oct 26
the `gerrit --help` output links to the Gerrit search syntax: https://gerrit-review.googlesource.com/Documentation/user-search.html which documents the hashtag query syntax: https://gerrit-review.googlesource.com/Documentation/user-search.html#hashtag `gerrit --help` also shows the "sethashtags" helper command if you want to modify the set of hashtags on CLs. there's also a "topic" helper command. so with those two, you should be able to look up all the CLs using a specific topic, then set the hashtags on those CLs to whatever you want, and then clear the topic on all the CLs.
,
Oct 26
gerrit accepts hashtags on push https://gerrit-review.googlesource.com/Documentation/intro-user.html#hashtags Chromium's `git cl upload` derives hashtags from the commit subject https://cs.chromium.org/chromium/tools/depot_tools/git_cl.py?q=git_cl.py&sq=package:chromium&g=0&rcl=2b71832f6d8dc74119589992836cf95aeb8a9842&l=3450 and from a flag https://cs.chromium.org/chromium/tools/depot_tools/git_cl.py?q=git_cl.py&sq=package:chromium&g=0&rcl=2b71832f6d8dc74119589992836cf95aeb8a9842&l=4896
,
Nov 5
How do I make "repo upload" (I believe it's the typical way for uploading Chrome OS CLs) use hashtags? It has a convenient -t switch which uses current branch name as the topic, but it doesn't seem to have anything similar for hashtags.
,
Nov 5
the latest repo release does not offer a short option for setting hashtags
,
Nov 6
hashtags also dont seem to work from the GUI. that is bad because now one needs to patch in the CL locally and reupload to set the hashtag?
,
Nov 6
Clarification on #c20: it does work for self-uploaded CLs, not for partner's CLs that you are reviewing. I was trying to add a hashtag to https://chromium-review.googlesource.com/c/chromiumos/third_party/linux-firmware/+/1314389/ and failed.
,
Nov 12
if you have issues using the hashtag field, please file a new bug, or start a new e-mail thread. i don't think we need to get into hashtag usability issues here ... this is just about using topics like they're (currently) intended to be used in Gerrit.
,
Nov 13
Let me circle back here: given the desire to share parts of CQ between future CrOS CQ and existing browser CQ, we can raise priority here. If we can drop CQ-depend completely, sharing CQ parts will be easier. At the same time, if we teach browser CQ to recognize topics, we can reject CLs with topics for chromium/src and other repos by default. Then PSA will indeed suffice before enabling submit-by-topic on Chromium host. Btw, I dunno why bug was assigned to me, but I guess now I have good reason to own this.
,
Nov 13
I believe that we should pursue topics but CQ-DEPEND is often used for cros-host dependencies so we won't be able to get rid of it.
,
Nov 13
right, CQ-DEPEND is still necessary when commits span chrome-internal & chromium (manifest & ebuild changes come to mind), but i think that's far less common.
,
Nov 13
We will need to add CQ-DEPEND support to the LUCI CQ Daemon. Sounds like it will forever remain a hack though (non-transactional/atomic submits). Any way to make it less hacky while we are at this? We should also consider either disabling CQ-DEPEND for CLs in the same project, or translating those into Topics. Might get tricky if later a cross-project depends is added though.
,
Nov 13
i'm fine with encouraging people to stop using CQ-DEPEND for same project/GoB host, but i don't think we need to be proactive about it on the coding side if it's a pain to cover wrt atomic/transactions, that's how it's always been and i think people get it, so that's OK |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by la...@chromium.org
, Jun 14 2018