Closed Bug 1102907 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla38

People

(Reporter: jgraham, Assigned: jgraham)

References

(Depends on 1 open bug, )

Details

Attachments

(4 files, 1 obsolete file)

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.
Another problem is how to deal with backouts on the mozilla side; upstreaming each of these as-is is somewhat ugly.
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
Attachment #8530398 - Flags: feedback?(ato)
Attachment #8530398 - Flags: feedback?(Ms2ger)
Attachment #8538414 - Flags: review?(ahalberstadt)
/r/1585 - Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.

Pull down this commit:

hg pull review -r 3d02e171f665f976b5001bd54296503017f8f115
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.
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.
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?
(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 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.
Attachment #8538414 - Flags: review?(ahalberstadt)
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.
Attachment #8538414 - Flags: review?(ahalberstadt)
/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
Attachment #8538414 - Flags: review?(ahalberstadt) → review+
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
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()`
> Hang on, I think the `resp.iter_content()` method only works if you pass
> `stream=True` into `requests.get()`

OK, I added that.
Attachment #8530398 - Flags: feedback?(Ms2ger) → feedback+
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.
Attachment #8530398 - Flags: feedback?(ato)
Attachment #8538414 - Attachment is obsolete: true
Attachment #8618670 - Flags: review+
Attachment #8618671 - Flags: review+
Attachment #8618672 - Flags: review+
Depends on: 1216377
You need to log in before you can comment on or make changes to this bug.