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

RESOLVED FIXED in mozilla38

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jgraham, Assigned: jgraham)

Tracking

(Depends on: 1 bug)

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Another problem is how to deal with backouts on the mozilla side; upstreaming each of these as-is is somewhat ugly.
(Assignee)

Comment 2

4 years ago
Created attachment 8530398 [details] [diff] [review]
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
Attachment #8530398 - Flags: feedback?(ato)
Attachment #8530398 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 3

4 years ago
Created attachment 8538414 [details]
MozReview Request: bz://1102907/jgraham
Attachment #8538414 - Flags: review?(ahalberstadt)
(Assignee)

Comment 4

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

Pull down this commit:

hg pull review -r 3d02e171f665f976b5001bd54296503017f8f115
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
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)
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8538414 - Flags: review?(ahalberstadt)
(Assignee)

Comment 11

4 years ago
/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()`
(Assignee)

Comment 14

4 years ago
> 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)
Duplicate of this bug: 1064625
(Assignee)

Comment 21

4 years ago
Comment on attachment 8538414 [details]
MozReview Request: bz://1102907/jgraham
Attachment #8538414 - Attachment is obsolete: true
Attachment #8618670 - Flags: review+
Attachment #8618671 - Flags: review+
Attachment #8618672 - Flags: review+
(Assignee)

Comment 22

4 years ago
Created attachment 8618670 [details]
MozReview Request: fixup! Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.
(Assignee)

Comment 23

4 years ago
Created attachment 8618671 [details]
MozReview Request: Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.
(Assignee)

Comment 24

4 years ago
Created attachment 8618672 [details]
MozReview Request: fixup! fixup! Bug 1102907 - Script for allowing web-platform-tests changes to be upstreamed.

Updated

3 years ago
Depends on: 1216377
You need to log in before you can comment on or make changes to this bug.