chrome-internal autolinks CQ-DEPEND=<Change-Id> to chrome-internal instead of chromium |
|||||||||||||
Issue descriptionCL:*708528 is auto-generated and has the following tags: BUG= chromium:901057 TEST=None CQ-DEPEND=I764f703972766874c5fd5e998b519b9748ebea2a where the dependency is supposed to be in the public gerrit. The UI does not recognize this and searches for the Change-Id in the private gerrit. The same is true for CL:1313149, where the opposite is observed.
,
Nov 8
It may be possible that this can be achieved via configuration on chromium's end.
,
Nov 8
,
Nov 8
,
Nov 9
Jason, here a CL on one gerrit host uses CQ-DEPEND to reference a CL on a different gerrit host. Whatever interprets CQ-DEPEND (I assume cbuildbot), does it assume that commit ids do not collide on different hosts?
,
Nov 9
i am asking because without the host, Gerrit would have hard time guessing it. Note that bug links do have optional Monorail project prefix, e.g. "chromeos:903483" (with default "chromium")
,
Nov 9
+Gardener CBuildBot and Gerrit assume that a CL:*NUMBER is chrome-internal and CL:NUMBER (without the star) is chromium. Example: https://chrome-internal-review.googlesource.com/c/chromeos/overlays/overlay-cheza-private/+/711210 I've never seen someone use the Change-Id in the CQ-DEPEND syntax. I'm surprised that works, really. groeck@ can you switch the CL autogeneration to use the Gerrit change number instead of the autogenerated Change-Id? I believe that will fix it. I don't know what autogenerated these CL's. If not you, please assign to current Gardener, sammc@.
,
Nov 9
Sorry, I have no idea where those scripts are located. The author of the CLs mentioned in the description might know.
,
Nov 9
,
Nov 12
Based on https://chromium.googlesource.com/chromiumos/chromite/+/7dbd8f0710161ed61e98f4af3660430a6d8a48fe/lib/patch.py#509 and https://chromium.googlesource.com/chromiumos/chromite/+/7dbd8f0710161ed61e98f4af3660430a6d8a48fe/lib/patch.py#735, Change-Id looks valid for CQ-DEPEND. The part of the uprev script responsible for these is https://chromium.googlesource.com/chromiumos/chromite/+/7dbd8f0710161ed61e98f4af3660430a6d8a48fe/cli/cros/cros_uprevchrome.py#313. Over to next week's gardeners to decide what to do with this.
,
Nov 12
Hi Sam, the incompatibility is in Gerrit, not Chromite. Gerrit doesn't build cros-instance indexes on Change-ID's but it does for Gerrit change numbers.
,
Nov 14
+jdufault@ who is the owner of http://go/cgcl/1313149. Do you know where to find PFQ manual uprev script (specifically the CQ-DEPEND for commit message)?
,
Nov 14
Not sure - I followed https://yaqs.googleplex.com/eng/q/5592011371970560
,
Nov 15
+dgarrett@, uprevchrome currently use change Id for CQ-DEPEND in commit message [1] [2], we need to change it to CL number instead. Is there a way to get CL number? [1] https://cs.corp.google.com/chromeos_public/chromite/cli/cros/cros_uprevchrome.py?type=cs&q=p:chromeos_+CommitMessage&g=0&l=314 [2] https://cs.corp.google.com/chromeos_public/chromite/cli/cros/cros_uprevchrome.py?type=cs&q=p:chromeos_+CommitMessage&g=0&l=324
,
Nov 15
If the CL has been uploaded to Gerrit before that point, it's possible to look up the change number. Our existing Gerrit library is here, it might not have the exact API you want, but should be extendable in a relatively straight forward way. http://cs/chromeos_public/chromite/lib/gerrit.py The backing API itself is defined here: https://gerrit-review.googlesource.com/Documentation/rest-api.html
,
Nov 19
Over to this week's gardener.
,
Nov 26
Passing to the next gardener
,
Nov 27
This bug has been passed on between four gardeners, if I'm counting correctly. That sorta suggests to me that it's not important enough to take the gardener's time. Nevertheless, taking a look. :-/
I think that nxia@ is cli/cros/cros_uprevchrome.py's author, but she's no longer on the team.
The script contains the following:
311 priv_commit_body = self.ParseGitLog(priv_overlay)
312 # Add CQ-DEPEND
313 commit_message = self.CommitMessage(
314 priv_commit_body, pfq_build, build_number, pub_cid)
315
316 # Update the commit message and reset author
317 priv_cid = git.Commit(priv_overlay, commit_message, amend=True,
318 reset_author=True)
319 if not priv_cid:
320 raise Exception("Don't know the commit ID of the private overlay CL.")
321
322 # Add CQ-DEPEND
323 commit_message = self.CommitMessage(
324 pub_commit_body, pfq_build, build_number, '*' + priv_cid, pub_cid)
325
326 git.Commit(pub_overlay, commit_message, amend=True)
327
328 overlays = [(pub_overlay, PUB_OVERLAY_URL),
329 (priv_overlay, PRIV_OVERLAY_URL)]
330
331 for (_overlay, _overlay_url) in overlays:
332 logging.info('git pull --rebase %s %s', remote, MASTER_BRANCH)
333 git.RunGit(_overlay,
334 ['pull', '--rebase', remote, MASTER_BRANCH])
335
336 logging.info('Upload CLs to Gerrit.')
337 git.UploadCL(_overlay, _overlay_url, MASTER_BRANCH,
338 skip=self.options.dry_run, draft=self.options.draft,
339 debug_level=logging.NOTICE, reviewers=reviewers)
(Aside: many functions' return values are undocumented in lib/git.py.)
I think that we'd need to upload the changes to Gerrit once, parse the change numbers from the output, and then amend the local commits to have the right CQ-DEPEND lines and reupload.
Taking a stab at this, although not enjoying it at all since this is a several-hundred line Python script with inadequate tests that takes more than a minute to run every time I want to try out my changes.
,
Nov 27
,
Nov 27
so the complaint is just that the autolinkification on the internal GoB points to the internal GoB instead of the public GoB when using CQ-DEPEND= lines ? the syntax used (from the perspective of cbuildbot) is correct. as you noted, the reason we use the full Change-Id syntax is so that we don't have to upload, get a CL #, edit, then re-upload. the uprev script is WAI currently. the gerrit settings suggest that it's already supposed to autolink to the public GoB instance, but maybe gerrit has an internal check that overrides that. https://cs.corp.google.com/piper///depot/google3/devtools/gerritcodereview/hosted_sites/chrome_internal/etc/gerrit.config?q=CL:+file:%5E//depot/google3/devtools/gerritcodereview/+package:%5Epiper$&l=86
,
Nov 27
Well, I just uploaded a change to switch to the CL:123 format. If you want to convince someone to fix Gerrit to link asterisk-prefixed Change-Ids correctly, that's cool too. :-P
In the config file that you linked to, there's this pattern:
CL:(I[0-9a-fA-F]{8,40}|[0-9]{1,8})
That seems off -- it looks like it's trying to match "CL:<Change-Id>" in addition to "CL:<change_num>". As far as I'm aware, at least in CQ-DEPEND, the "CL:" prefix is only used with change nums, and Change-Ids are supposed to be bare.
,
Nov 27
> As far as I'm aware, at least in CQ-DEPEND, the "CL:" prefix is only used with change nums, and Change-Ids are supposed to be bare. fairly certain that has never been the case. chromite/lib/patch.py:GetPaladinDeps is the code that rips out CQ-DEPEND= lines and tokenizes the content, and chromite/lib/patch.py:ParsePatchDep is the token parser. the "CL:" is always stripped off from every token and can be considered optional. whether the community has settled on a specific style is a sep question. sounds like the trivial patch is to change the uprev tool to use CL:*<Change-Id> syntax ?
,
Nov 28
That is trivial and indeed works. Thanks! Uploaded https://crrev.com/c/1351648.
,
Nov 30
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/chromite/+/0de9e8166ee4a9fbf483b9d7993b5d3ee8fbcd1f commit 0de9e8166ee4a9fbf483b9d7993b5d3ee8fbcd1f Author: Daniel Erat <derat@chromium.org> Date: Fri Nov 30 00:09:10 2018 cros_uprevchrome: Fix cross-site CQ-DEPEND links. Prefix CQ-DEPEND links in commits generated by "cros uprevchrome" with "CL:" so that references between the public and private Gerrit instances will be interpreted correctly. BUG= chromium:903483 TEST=manual: ran the script and checked changes Change-Id: I92c6e34e9eed3ea5e143e993920acd8da138548b Reviewed-on: https://chromium-review.googlesource.com/1351648 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Don Garrett <dgarrett@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/0de9e8166ee4a9fbf483b9d7993b5d3ee8fbcd1f/cli/cros/cros_uprevchrome.py
,
Nov 30
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by groeck@chromium.org
, Nov 1