Closed Bug 1410737 Opened 7 years ago Closed 7 years ago

Add support for displaying the repository and version checked out in a way that will appear on treeherder.

Categories

(Developer Services :: Mercurial: robustcheckout, enhancement)

Product:
Developer Services
Component:
Mercurial: robustcheckout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: tomprince, Assigned: tomprince)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 1410737: Log repositories checked out.
59 bytes, text/x-review-board-request
gps
: review+
Details 
      No description provided.
comm-central currently has code in `client.py` to display the repository and revision of mozilla-central that was checked out in the treeherder summary. We are moving to using taskcluster, which calls robustcheckout, either directly (on windows/macos) or via run-task. I'd like to add an option to display the same information from robustcheckout.
Comment on attachment 8920899 [details]
Bug 1410737: Log repositories checked out.

https://reviewboard.mozilla.org/r/191850/#review197012

I realize that this is a vendored copy, but it was easier to prototype this way. I'll submit it a patch to the upstream version if this approach is acceptable.

::: testing/mozharness/external_tools/robustcheckout.py:613
(Diff revision 1)
> +    repo_name = url.split('/')[-1]
> +    ui.write(b"TinderboxPrint:<a href=%(source_repo)s/rev/%(revision)s "
> +             b"title='Built from %(repo_name)s revision %(revision)s'>%(revision)s</a>\n" %
> +             {b"revision": checkoutrevision,
> +              b"source_repo": url,
> +              b"repo_name": repo_name})

I considered using Mercurial's templating functionality, instead of hard-coding this. But it didn't seem documneted in a way that would make it easy to do.
Comment on attachment 8920899 [details]
Bug 1410737: Log repositories checked out.

https://reviewboard.mozilla.org/r/191852/#review197306

I chatted with some people in #treeherder ~1 hour ago and nobody is a big fan of `Tinderboxprint:`. They want to remove it. That being said, there is no good replacement for it. This chat let me to believe that there is questionable value in this commit.

Also, `robustcheckout` is canonically developed in the version-control-tools repo: https://hg.mozilla.org/hgcustom/version-control-tools/file/@/hgext/robustcheckout.

But if we must take this commit, I'd prefer to perform the `Tinderboxprint:` from `run-task` (not involving `hg robustcheckout`) so we can avoid complexity.
Attachment #8920899 - Flags: review?(gps) → review-
Comment on attachment 8920899 [details]
Bug 1410737: Log repositories checked out.

https://reviewboard.mozilla.org/r/191852/#review197306

> But if we must take this commit, I'd prefer to perform the Tinderboxprint: from run-task (not involving hg robustcheckout) so we can avoid complexity.

I had originally done that, but I realized that windows [doesn't use run-task](https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/job/mozharness.py#254) but does use robustcheckout. I didn't want to figure out how to resolve the commit in batch; but on further reflection, by the time that runs, `GECKO_HEAD_*` are fully resolved, so I can just put the `TinderboxPrint` in a [comment command](https://hg.mozilla.org/try/rev/d6f222f6e7803362ccba50a218d487615117c869).
> https://mozilla.logbot.info/treeherder/20171023#c13735353-c13735458

I'd be happy to use something other than TinderboxPrint, particularly if it was structured data. I'd be happy to use whatever, and perhaps even help implementing it. Regarding whether the information here is useful, for comm-central builds, I often want to see the mozilla-central version associated to a build. I implemented this after looking for the information in a c-c taskcluster build and realizing that it wasn't there.
Flags: needinfo?(emorley)
See Also: → 1342296
Hi!

So bug 1342296 is something that I'd love to see resolved, though it involves a lot of moving pieces, which is why it hasn't been tackled yet. 

The existing TinderboxPrint usages (or their Pulse schema equivalents) fall into a few categories:
* junk characters / mistakes
* duplicates of data already tracked in other parts of Treeherder (eg performance stats that are already submitted to perfherder)
* things that only 1-2 people use, or are used so infrequently that people should just be looking at them in the log instead
* things that are available in other systems, so might be better off linked to instead of being stored in Treeherder (eg Taskcluster has an artifacts viewer which already lists all uploaded files, so Treeherder probably doesn't need to store that same list)
* valid use-cases, that Treeherder should be storing (but ideally via a structured data format rather than TinderboxPrints)

As for this bug/PR, I have no objections to people still using TinderboxPrints until we make more progress on bug 1342296, however do bear in mind that we may not support this use-case, given it's a workaround for lack of revision pinning.

ie: the way B2G resolved the problem of tracking two repositories was to use a pinned revision based approach, and a bot that pushed to the repo to increment the revision. This solves many many problems with the unpinned approach (think bisection, backouts, sheriffing, ...).

I would really recommend that comm-central look into that approach, rather than continue with the current process.
Flags: needinfo?(emorley)
Attachment #8922437 - Attachment is obsolete: true
Attachment #8922437 - Flags: review?(gps)
Comment on attachment 8920899 [details]
Bug 1410737: Log repositories checked out.

https://reviewboard.mozilla.org/r/191852/#review205068

I suppose this logging is fine. But I have a nit with the Python code.

::: taskcluster/docker/recipes/run-task:221
(Diff revision 3)
> +    print_line(b'vcs', b"TinderboxPrint:<a href=%(source_repo)s/rev/%(revision)s "
> +                       b"title='Built from %(repo_name)s revision %(revision)s'>%(revision)s</a>" %
> +               {b"revision": revision,
> +                b"source_repo": source_repo,
> +                b"repo_name": repo_name})

I've never seen `'%(var)' % dict` actually used. In fact, I didn't realize this was even valid.

Please switch to using `.format()`, which is the way variable interpolation is commonly done in Python 2.7.
Attachment #8920899 - Flags: review?(gps) → review-
Comment on attachment 8920899 [details]
Bug 1410737: Log repositories checked out.

https://reviewboard.mozilla.org/r/191852/#review205068

> I've never seen `'%(var)' % dict` actually used. In fact, I didn't realize this was even valid.
> 
> Please switch to using `.format()`, which is the way variable interpolation is commonly done in Python 2.7.

Done. It looked like `run-task` was using `%` in preference to `.format`, so I originally just kept the same style.
Comment on attachment 8920899 [details]
Bug 1410737: Log repositories checked out.

https://reviewboard.mozilla.org/r/191852/#review205088

Well, it uses `%` formatting. It just doesn't use the `%(var)s` form.
Attachment #8920899 - Flags: review?(gps) → review+
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/b91ed2019729
Log repositories checked out. r=gps
Attachment #8920899 - Attachment is obsolete: true
Attachment #8928826 - Flags: review+
Attachment #8928826 - Flags: review+ → review?(mozilla)
Attachment #8928826 - Flags: review?(mozilla) → review+
Attachment #8928826 - Attachment is obsolete: true
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b43757a2b80
Backed out changeset b91ed2019729 on request. r=backout on a CLOSED TREE
Comment on attachment 8928838 [details]
Bug 1410737: Log repositories checked out.

https://reviewboard.mozilla.org/r/200120/#review205566
Attachment #8928838 - Flags: review?(gps) → review+
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/184679df8435
Log repositories checked out. r=gps
https://hg.mozilla.org/mozilla-central/rev/184679df8435
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: