Find manifest for download by tags instead of commits #14025
Conversation
| return None | ||
|
|
||
|
|
||
| def download_manifest(manifest_path, tags_func, url_func, force=False): |
There was a problem hiding this comment.
The abstraction here becomes kind of pointless now, as using tags instead of commits makes this very likely GitHub specific. Just collapse the indirection?
There was a problem hiding this comment.
Well the right indicrection would probably be a single function that did the equivalent of f = lambda x:url_func(tags_func()), although in practice we built the gecko equivalent of this at a higher level (perhaps incorrectly) so I don't mind. In any case, followup.
|
|
||
| def download_manifest(manifest_path, commits_func, url_func, force=False): | ||
| tags = [] | ||
| for line in git("log", "--format=%D", "--max-count=%s" % max_count).split("\n"): |
There was a problem hiding this comment.
git describe --tags --match 'merge_pr_*' does basically this (this is supported back to ancient times)
There was a problem hiding this comment.
or maybe that doesn't work if we need more than one? hmm
There was a problem hiding this comment.
It won't get just the tags reachable from HEAD, or even the tags in tree order.
There was a problem hiding this comment.
We could call it again using old_tag^1 of course. Possibly that would look nicer?
There was a problem hiding this comment.
git describe --tags --match 'merge_pr_*' just tries to describe HEAD, man page says "Commit-ish object names to describe. Defaults to HEAD if omitted." Please provide exact command if you think there's a way :)
If we did want to call git describe many times to get from merge_pr_14054 (a merge commit) to merge_pr_14137 (the previous tag) then ^1 is the right thing unless an admin disables the master branch protection and pushes a merge commit to master with old master as the non-first parent and that push closes a PR such that it becomes tagged with merge_pr_*. And then all that would happen is that maybe an older version of the manifest were found. The only improvement would be git describe --tags all the parents and then pick the most recent tag. But then you're better off using git log to walk commits in date order as I've already done.
There was a problem hiding this comment.
@foolip according to the man page I have here:
The command finds the most recent tag that is reachable from a commit. If the tag
points to the commit, then only the tag is shown. Otherwise, it suffixes the tag
name with the number of additional commits on top of the tagged object and the
abbreviated object name of the most recent commit.
On wptrunner_tlbc for example it currently gives merge_pr_13862-3-g2ed77c929e. Adding --abbrev=0 gets just the tag: merge_pr_13862.
See also the Search Strategy.
There was a problem hiding this comment.
We want to find many tags though, not just the most recent, since the most recent tag might not have the manifest uploaded yet, and also occasionally manifest upload fails in the deploy step.
There was a problem hiding this comment.
I think what's here is fine.
There was a problem hiding this comment.
Right, indeed, which is why this doesn't work.
|
|
||
| def download_manifest(manifest_path, commits_func, url_func, force=False): | ||
| tags = [] | ||
| for line in git("log", "--format=%D", "--max-count=%s" % max_count).split("\n"): |
There was a problem hiding this comment.
I think what's here is fine.
| return None | ||
|
|
||
|
|
||
| def download_manifest(manifest_path, tags_func, url_func, force=False): |
There was a problem hiding this comment.
Well the right indicrection would probably be a single function that did the equivalent of f = lambda x:url_func(tags_func()), although in practice we built the gecko equivalent of this at a higher level (perhaps incorrectly) so I don't mind. In any case, followup.
This makes it possible to use `./wpt manifest` on a checkout that is more than 50 commits behind master. The downside is that it will require more network requests if the most recent tag/tags don't have releases uploaded. Add more warnings to make it clearer why it goes wrong when it does.
foolip
force-pushed
the
foolip/manifest-download-by-tag
branch
from
63cacb0
to
948619e
Compare
This makes it possible to use
./wpt manifeston a checkout that ismore than 50 commits behind master. The downside is that it will require
more network requests if the most recent tag/tags don't have releases
uploaded.
Add more warnings to make it clearer why it goes wrong when it does.