1102907 - Add a script for syncing local changes to the upstream repository.

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect)

Description

[James Graham [:jgraham]]

When doing a sync with upstream we want to first push the patches we have taken locally to upstream. Something like the following:

1) List all commits in testing/web-platform/tests since the last sync
2) Generate a patch from each commit and apply it to the upstream repo on a branch from the previous sync commit.
3) Rebase the newly created branch on master
4) For each rebased commit, in order, create a PR against the upstream and merge it.
5) Update the local repo to the new upstream master.

Note: this doesn't work if we don't want to sync to master. In that case we should do steps 1-3 against the revision we are syncing to and then just use the local rebased version as the new m-c version.

There are also a number of things that can go wrong that might require human attention. In particular, step 3 can generate merge errors which will need to be fixed by hand.

Comment 1

[James Graham [:jgraham]]

Another problem is how to deal with backouts on the mozilla side; upstreaming each of these as-is is somewhat ugly.

Comment 2

[James Graham [:jgraham]]

Attachment: wpt-upstream.diff

https://github.com/jgraham/gecko-dev/commit/3b0e88f83d9c50eed64bae2130dce19be95963a8

First cut at this, so go with the corresponding changes in wptrunner [1]

[1] https://critic.hoppipolla.co.uk/r/3315

Comment 3

[James Graham [:jgraham]]

Attachment: MozReview Request: bz://1102907/jgraham

Comment 4

[James Graham [:jgraham]]

/r/1585 - Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.

Pull down this commit:

hg pull review -r 3d02e171f665f976b5001bd54296503017f8f115

Comment 5

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/1583/#review973

::: testing/web-platform/wptrunner.ini
(Diff revision 1)
> -remote_url = https://github.com/w3c/web-platform-tests.git
> +remote_url = https://github.com/jgraham/web-platform-tests.git

Need to change this back for release; this is just more convenient for testing.

Comment 6

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/1583/#review975

::: testing/web-platform/update/tree.py
(Diff revision 1)
> +    # TODO: The extra methods for upstreaming patches from a

I plan to add hg support in future work; this isn't something that most people are going to run so I don't think that it's important to make it work in all vcs setups right now.

Comment 7

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/1583/#review1037

::: testing/web-platform/mach_commands.py
(Diff revision 1)
> +        self.virtualenv_manager.install_pip_package('requests')

I wish this was in-tree. Maybe one day someone will add it.

::: testing/web-platform/update/fetchlogs.py
(Diff revision 1)
> +    print job_data

Leftover debug statement?

::: testing/web-platform/update/fetchlogs.py
(Diff revision 1)
> +                if item["value"] == "wpt_structured_full.log":

We should probably have a standard file name/pattern for structured logs to make finding them in blobber easier. But that's not related to this patch.

::: testing/web-platform/update/fetchlogs.py
(Diff revision 1)
> +                    if job_type_name.startswith("W3C Web Platform") or job_type_name == "unknown":

Won't "unknown" catch other kinds of jobs too?

::: testing/web-platform/update/fetchlogs.py
(Diff revision 1)
> +    with open(name, "wb") as f:
> +        resp = requests.get(url)
> +        f.write(resp.text.encode(resp.encoding))

I recently learned the recommended way of saving to a file with requests is:

    for chunk in r.iter_content(1024):
        f.write(chunk)

::: testing/web-platform/update/github.py
(Diff revision 1)
> +        if 200 <= resp.status_code < 300:
> +            return resp.json()
> +        else:
> +            raise GitHubError(resp.status_code, resp.json())

Instead of status code checking you could call `resp.raise_for_status()`.

::: testing/web-platform/update/tree.py
(Diff revision 1)
> +    _bug_re = re.compile("^Bug (\d+)[^\w]*(?:Part \d+[^\w]*)?(.*?)\s*(?:r=(\w*))?$",
> +                         re.IGNORECASE)
> +
> +    _backout_re = re.compile("^(?:Back(?:ing|ed)\s+out)|Backout|(?:Revert|(?:ed|ing))",
> +                             re.IGNORECASE)
> +    _backout_sha1_re = re.compile("(?:\s|\:)(0-9a-f){12}")

Are these the official regexes used by the hg repo hooks? If so, please add a link to them as a comment.

::: testing/web-platform/update/update.py
(Diff revision 1)
> +    """Load gecko tree and tree to sync with upstream"""

Some kind of typo here

::: testing/web-platform/update/upstream.py
(Diff revision 1)
> +            os.mkdir(state.sync["path"])

This will fail if an intermediate directory needs to be made. Should this be os.makedirs() instead?

Comment 8

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> I recently learned the recommended way of saving to a file with requests is:
> 
>     for chunk in r.iter_content(1024):
>         f.write(chunk)

Forgot to mention this also requires `stream=True` in the call to requests.get().

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8538414 [details]
MozReview Request: bz://1102907/jgraham

I'm never really sure what to do about the r? in bugzilla with the reviewboard/issue based workflow. Un-setting to clear my queue for now.

Comment 10

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/1583/#review1379

> Leftover debug statement?

Kind of? At the moment this script is used interactively and this is quite helpful to ensure it's doing the right thing (e.g. if all the job_data is [] then something went wrong with the log processing).

> Won't "unknown" catch other kinds of jobs too?

Yeah, legacy from before these jobs had consistent names (this file was only moved in this patch not actually added, but it still makes sense to improve it :)

> Instead of status code checking you could call `resp.raise_for_status()`.

I'm not sure that's better; it just means I have to catch the exception and rewrite it to be a GitHubError, and also is less strict (e.g. if I get a 100 or a 303 I don't handle it, so raing an exception is better than just failing in some other way).

> Some kind of typo here

Actually no, but it's rather unclear (you should read it as "Load (gecko tree) and (tree to sync with upstream)"). I've tried to clarify.

Comment 11

[James Graham [:jgraham]]

/r/1585 - Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.
/r/2201 - fixup! Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.
/r/2207 - fixup! fixup! Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.

Pull down these commits:

hg pull review -r d5c673b916c5ad12616d4b9a8660f0871b5a1861

Comment 12

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/1583/#review1465

::: testing/web-platform/update/tree.py
(Diff revisions 1 - 2)
> +    # slightly different becase we need to parse out specific parts of the message rather

nit: because

Comment 13

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/1583/#review1467

::: testing/web-platform/update/fetchlogs.py
(Diff revisions 1 - 2)
>          resp = requests.get(url)
> -        f.write(resp.text.encode(resp.encoding))
> +        for chunk in resp.iter_content(1024):
> +            f.write(chunk)

Hang on, I think the `resp.iter_content()` method only works if you pass `stream=True` into `requests.get()`

Comment 14

[James Graham [:jgraham]]

> Hang on, I think the `resp.iter_content()` method only works if you pass
> `stream=True` into `requests.get()`

OK, I added that.

Comment 15

[James Graham [:jgraham]]

https://tbpl.mozilla.org/?tree=Try&rev=9f41ea8e6e52

Comment 16

[James Graham [:jgraham]]

https://tbpl.mozilla.org/?tree=Try&rev=5304ea420442

Comment 17

[James Graham [:jgraham]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64e1d633f7de
https://hg.mozilla.org/integration/mozilla-inbound/rev/c38f53efdcc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/b57000fb4ca3
https://hg.mozilla.org/integration/mozilla-inbound/rev/334f3c880340

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/64e1d633f7de
https://hg.mozilla.org/mozilla-central/rev/c38f53efdcc9
https://hg.mozilla.org/mozilla-central/rev/b57000fb4ca3
https://hg.mozilla.org/mozilla-central/rev/334f3c880340

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8530398 [details] [diff] [review]
wpt-upstream.diff

Review of attachment 8530398 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't really have any useful feedback to give for this.

Comment 22

[James Graham [:jgraham]]

Attachment: MozReview Request: fixup! Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.

Comment 23

[James Graham [:jgraham]]

Attachment: MozReview Request: Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.

Comment 24

[James Graham [:jgraham]]

Attachment: MozReview Request: fixup! fixup! Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.