New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 903483 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug


Previous locations:
gerrit:9968


Sign in to add a comment

chrome-internal autolinks CQ-DEPEND=<Change-Id> to chrome-internal instead of chromium

Project Member Reported by groeck@chromium.org, Nov 1

Issue description

CL:*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.

 
Here are the tags from CL:1313149:

BUG= chromium:901057 
TEST=None
CQ-DEPEND=*Iba309fe25c0c732a7ffb984022d30cccaf1e6dc1

Change-Id: I764f703972766874c5fd5e998b519b9748ebea2a
It may be possible that this can be achieved via configuration on chromium's end.
Project: chromium
Moved issue gerrit:9968 to now be  issue chromium:903483 .
Components: Infra>Codereview>Gerrit
Cc: jclinton@chromium.org no...@chromium.org
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?
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")
Cc: sa...@chromium.org
Labels: OS-Chrome Pri-3 Type-Bug
Owner: groeck@chromium.org
Status: Assigned (was: New)
Summary: PFQ manual uprev script incorrectly using Change-Id in CQ-DEPEND (was: UI does not correctly recognize CQ-DEPEND entries specifying a Change-Id)
+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@.

Sorry, I have no idea where those scripts are located. The author of the CLs mentioned in the description might know.

Owner: sa...@chromium.org
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.
Cc: jdufault@chromium.org
+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)?
Cc: dgarr...@chromium.org
+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
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
Owner: agawronska@chromium.org
Over to this week's gardener.
Owner: derat@chromium.org
Passing to the next gardener
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.
Status: Started (was: Assigned)
Cc: vapier@chromium.org
Summary: chrome-internal autolinks CQ-DEPEND=<Change-Id> to chrome-internal instead of chromium (was: PFQ manual uprev script incorrectly using Change-Id in CQ-DEPEND)
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
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.
> 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 ?
That is trivial and indeed works. Thanks!

Uploaded https://crrev.com/c/1351648.
Project Member

Comment 24 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment