update relman scripts based on notes from first usage

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
3 years ago
20 days ago

People

(Reporter: hwine, Assigned: hwine)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Much was learned about these scripts during the first usage. They work, but aren't always clear as to "action needed".

This bug is to annotate the scripts (beta2release.sh, beta2release_l10n.sh, merge_helper.py) based on the notes collected during the Mar 14 merge. The end result is a better set of scripts for the next merge day "as is"
(Assignee)

Updated

3 years ago
Blocks: 988613

Updated

3 years ago
Blocks: 984214
(Assignee)

Comment 1

3 years ago
Created attachment 8401588 [details] [diff] [review]
py27.diff

tweak to prevent running if python 2.7 isn't used.
Attachment #8401588 - Flags: review?(aki)
(Assignee)

Comment 2

3 years ago
Created attachment 8401589 [details] [diff] [review]
doc.diff

Changes based on notes from usage on ff 28
- clarify what input is expected at places
- remove prompts with no action
- provide more context to prompts

No functional changes.
Attachment #8401589 - Flags: review?(aki)

Updated

3 years ago
Attachment #8401588 - Flags: review?(aki) → review+

Updated

3 years ago
Attachment #8401589 - Flags: review?(aki) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8401597 [details] [diff] [review]
doc_l10n.diff

Based on notes from ff 28
- removed diff that wasn't reviewed at all
- updated comments for context of some anecdotal issues from the past
Attachment #8401597 - Flags: review?(aki)

Updated

3 years ago
Attachment #8401597 - Flags: review?(aki) → review+
(Assignee)

Comment 4

3 years ago
Comment on attachment 8401588 [details] [diff] [review]
py27.diff

https://hg.mozilla.org/build/braindump/rev/2825976a562b
Attachment #8401588 - Flags: checked-in+
(Assignee)

Comment 5

3 years ago
Comment on attachment 8401589 [details] [diff] [review]
doc.diff

https://hg.mozilla.org/build/braindump/rev/9c268cdab9bd
Attachment #8401589 - Flags: checked-in+
(Assignee)

Comment 6

3 years ago
Comment on attachment 8401597 [details] [diff] [review]
doc_l10n.diff

https://hg.mozilla.org/build/braindump/rev/c21f6a952054
Attachment #8401597 - Flags: checked-in+
Created attachment 8407977 [details] [diff] [review]
retry-tools-4.diff

this is kind of a rewrite of beta2release.py:

* since I used a lot of helper functions from the tools repo, I moved the script to tools/release
* removed python 2.7 check since argparse requires it implicitly
* Removed unused tag step http://hg.mozilla.org/build/braindump/file/f171ef1f3bd3/releases-related/beta2release.py#l140. The tag wasn't accessible by hg up until recently we moved the tagging step. Less tags FTW.
* It works on my laptop!
Attachment #8407977 - Flags: review?(hwine)
Attachment #8407977 - Flags: review?(aki)

Comment 8

3 years ago
I'll give this a try.  I need to prep for next week anyway.
Created attachment 8408542 [details] [diff] [review]
retry-tools-5.diff

+ merge_helper.py
* moved tagging after merge_via_debugsetparents, so the tags are accessible vi hg command line. This change prevents them to be pulled. For example FIREFOX_BETA_30_BASE is accessible from the mozilla-aurora repo only until the next uplift.
* dropped android xul patching
* it seems working on my laptop
Attachment #8407977 - Attachment is obsolete: true
Attachment #8407977 - Flags: review?(hwine)
Attachment #8407977 - Flags: review?(aki)
Attachment #8408542 - Flags: review?(hwine)
Attachment #8408542 - Flags: review?(aki)

Comment 10

3 years ago
Comment on attachment 8408542 [details] [diff] [review]
retry-tools-5.diff

Stamping since you've tested and we're still smoothing rough edges.
Let me know if you want something more in-depth.
Attachment #8408542 - Flags: review?(aki) → review+
(Assignee)

Comment 11

3 years ago
Comment on attachment 8408542 [details] [diff] [review]
retry-tools-5.diff

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

lgtm -- most of my comments are about making it clearer what is going to happen when the operator is prompted by the script. That was the bulk of my confusion using the original scripts last time.

There are also a couple of consistency issues between the 2 scripts.

When you commit these, please delete the versions in braindump. Thanks!

::: lib/python/util/hg.py
@@ +627,5 @@
> +def merge_via_debugsetparents(dest, old_head, new_head, msg, user=None):
> +    """Merge 2 heads avoiding non-fastforward commits"""
> +    cmd = ['hg', 'debugsetparents', new_head, old_head]
> +    run_cmd(cmd, cwd=dest)
> +    commit(dest, msg=msg, user=user)

lgtm assuming run_cmd & commit raise exceptions on error

::: release/beta2release.py
@@ +39,5 @@
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument("--from-dir", default="mozilla-beta",
> +                        help="Working directory of repo to be merged from")
> +    parser.add_argument("--from-repo",
> +                        default="https://hg.mozilla.org/releases/mozilla-beta",

should the default be an ssh url to simplify pushing?

If not, should we "suggest" via a print to console the commit command?

I don't have a preference really -- I'm the odd one with merges (only level 2 access)

@@ +84,5 @@
> +    tag(from_dir, tags=[release_base_tag], rev=beta_rev, user=hg_user,
> +        msg="Added %s tag for changeset %s. DONTBUILD CLOSED TREE a=release" %
> +        (release_base_tag, beta_rev))
> +    new_beta_rev = get_revision(from_dir)
> +    raw_input("Push mozilla-beta and hit Return")

This felt brittle to me in the original script -- too easy to accidentally hit return. While no harm is done by an early return, if the operator forgets to push, that could be an issue.

For the future, maybe have this be a separate mh action so it can easily be re-run?

@@ +121,5 @@
> +    run_cmd(["hg", "diff"], cwd=to_dir)
> +    raw_input("If the diff looks good hit return to commit those changes")
> +    commit(to_dir, user=hg_user,
> +           msg="Update configs. CLOSED TREE a=release")
> +    log.warn("Go ahead and push mozilla-release changes.")

While brittle, it'd be nice to have a pause here, then have the script confirm that both repos have been pushed (i.e. 'hg out' doesn't report anything)

::: release/merge_helper.py
@@ +68,5 @@
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument(
> +        "--hg-user",
> +        default="Mozilla Release Engineering <release+merge@mozilla.com>",
> +        help="Mercurial username to be passed to hg -u")

We don't do it in the m-b => m-r case, so one of the 2 should change to be consistent.

I could go either way -- once it's fully automated, the role account is appropriate, before that (e.g. now)... meh

@@ +105,5 @@
> +    raw_input("if the diff looks good hit return to continue to commit")
> +    run_cmd(["hg", "diff"], cwd=mc_dir)
> +    raw_input("If the diff looks good hit return to commit those changes")
> +    commit(mc_dir, user=hg_user,
> +           msg="Version bump. CLOSED TREE a=release")

add DONTBUILD?

@@ +123,5 @@
> +    tag(ma_dir, tags=[ma_tag, ma_end_tag], rev=ma_revision, user=hg_user,
> +        msg="Added %s %s tags for changeset %s. DONTBUILD CLOSED TREE a=release" %
> +        (ma_tag, ma_end_tag,  ma_revision))
> +    bump_version(ma_dir, next_ma_version, next_ma_version, "a1", "a2")
> +    raw_input("if the diff looks good hit return to continue to commit")

no diff yet displayed -- will be confusing to operator. Either delete or tweak message that a diff _will_ come when they press enter.

@@ +126,5 @@
> +    bump_version(ma_dir, next_ma_version, next_ma_version, "a1", "a2")
> +    raw_input("if the diff looks good hit return to continue to commit")
> +    run_cmd(["hg", "diff"], cwd=ma_dir)
> +    raw_input("If the diff looks good hit return to commit those changes")
> +    commit(ma_dir, user=hg_user, msg="Version bump. CLOSED TREE a=release")

add DONTBUILD

@@ +156,5 @@
> +    raw_input("if the diff looks good hit return to continue to commit")
> +    run_cmd(["hg", "diff"], cwd=ma_dir)
> +    raw_input("If the diff looks good hit return to commit those changes")
> +    commit(ma_dir, user=hg_user,
> +           msg="Update configs. CLOSED TREE a=release")

add DONTBUILD

@@ +171,5 @@
> +    tag(mb_dir, tags=[mb_tag], rev=mb_revision, user=hg_user,
> +        msg="Added %s tag for changeset %s. DONTBUILD CLOSED TREE a=release" %
> +        (mb_tag, mb_revision))
> +    bump_version(mb_dir, next_mb_version, next_mb_version, "a2", "")
> +    raw_input("if the diff looks good hit return to continue to commit")

dito above about operator confusion

@@ +189,5 @@
> +        for f in branding_files:
> +            replace(path.join(mb_dir, d, f),
> +                    "ac_add_options --with-branding=mobile/android/branding/aurora",
> +                    "ac_add_options --with-branding=mobile/android/branding/beta")
> +    raw_input("if the diff looks good hit return to continue to commit")

ditto above about operator confusion

@@ +196,5 @@
> +    commit(mb_dir, user=hg_user,
> +           msg="Update configs. CLOSED TREE a=release")
> +
> +if __name__ == "__main__":
> +    main()

Unlike the m-b => m-r script, we don't have any reminders to push m-a & m-b. Should be added somewhere, or a check here for no outgoing on all 3 repos.
Attachment #8408542 - Flags: review?(hwine) → review+
Comment on attachment 8408542 [details] [diff] [review]
retry-tools-5.diff

https://hg.mozilla.org/build/tools/rev/27073c214410

(In reply to Hal Wine [:hwine] (use needinfo) from comment #11)
> When you commit these, please delete the versions in braindump. Thanks!

https://hg.mozilla.org/build/braindump/rev/e87016133dc2
 
> should the default be an ssh url to simplify pushing?

Done,
 
> > +    raw_input("Push mozilla-beta and hit Return")
> 
> This felt brittle to me in the original script -- too easy to accidentally
> hit return. While no harm is done by an early return, if the operator
> forgets to push, that could be an issue.
> 
> For the future, maybe have this be a separate mh action so it can easily be
> re-run?

I'd rather kill all pauses and make the script unattended. Next time, probably.

> > +    log.warn("Go ahead and push mozilla-release changes.")
> 
> While brittle, it'd be nice to have a pause here, then have the script
> confirm that both repos have been pushed (i.e. 'hg out' doesn't report
> anything)

replaced with raw_input() for now.

> > +        default="Mozilla Release Engineering <release+merge@mozilla.com>",
> We don't do it in the m-b => m-r case, so one of the 2 should change to be
> consistent.

Replaced with "ffxbld <release@mozilla.com"

> > +    commit(mc_dir, user=hg_user,
> > +           msg="Version bump. CLOSED TREE a=release")
> 
> add DONTBUILD?

I explicitly removed DONTBUILD from commits changing configs/versions because we want the to be built using the uplifted branch configs.

> > +    raw_input("if the diff looks good hit return to continue to commit")
> 
> no diff yet displayed -- will be confusing to operator. Either delete or
> tweak message that a diff _will_ come when they press enter.

Fixed by changing the message

> Unlike the m-b => m-r script, we don't have any reminders to push m-a & m-b.
> Should be added somewhere, or a check here for no outgoing on all 3 repos.

Done
Attachment #8408542 - Flags: checked-in+
Created attachment 8409709 [details] [diff] [review]
retry-tools.diff

Pass locales to be removed. I made the argument mandatory for now.
Attachment #8409709 - Flags: review?(hwine)
(Assignee)

Comment 14

3 years ago
Comment on attachment 8409709 [details] [diff] [review]
retry-tools.diff

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

I don't think this does the right thing in general. If we're just concerned about the usual suspects, then we should make those the default for the option.

::: release/beta2release.py
@@ +52,5 @@
> +        lines = f.readlines()
> +    with open(tmp_file_path, "w") as out:
> +        for line in lines:
> +            locale = line.split()[0]
> +            if locale not in locales:

I don't believe this will work right if locales is a comma separated string (which I think it is right now). The issue is substring matches -- e.g. if I'm trying to remove 'en-US', this code matches both 'en' and 'en-US' locales.

@@ +56,5 @@
> +            if locale not in locales:
> +                out.write(line)
> +            else:
> +                log.warn("Removied locale: %s", locale)
> +    shutil.move(tmp_file_path, file_name)

We're missing an opportunity here to catch bad input. If there is no such locale, I think we should error out.

@@ +76,5 @@
>      parser.add_argument("--hg-user", default="ffxbld <release@mozilla.com>",
>                          help="Mercurial username to be passed to hg -u")
> +    parser.add_argument("--remove-locale", dest="remove_locales", action="append",
> +                        required=True,
> +                        help="Locales to be removed from release shipped-locales")

What's the format to supply the locales?

One way to show that might be to include the usual suspects as the defaults for right now.

@@ +128,5 @@
>                  "ac_add_options --with-branding=mobile/android/branding/beta",
>                  "ac_add_options --with-branding=mobile/android/branding/official")
>  
> +    if args.remove_locales:
> +        log.info("Removing locales: %s", args.remove_locales)

I think you can fix the "using string" issue here by splitting the string into a list of locales.
Attachment #8409709 - Flags: review?(hwine) → review-
(Assignee)

Comment 15

3 years ago
Comment on attachment 8409709 [details] [diff] [review]
retry-tools.diff

r+ -- I missed the 'action="append"' in the option definition -- that forces the list context I was asking for. :/
Attachment #8409709 - Flags: review- → review+
Comment on attachment 8409709 [details] [diff] [review]
retry-tools.diff

https://hg.mozilla.org/build/tools/rev/6bc50972c7c0
Attachment #8409709 - Flags: checked-in+
Created attachment 8410281 [details] [diff] [review]
merge_helper-tools.diff

Added ba=release to avoid IID push errors.
Attachment #8410281 - Flags: review?(hwine)
(Assignee)

Comment 18

3 years ago
Comment on attachment 8410281 [details] [diff] [review]
merge_helper-tools.diff

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

lgtm, assuming that is all the commits in the new script
Attachment #8410281 - Flags: review?(hwine) → review+
Comment on attachment 8410281 [details] [diff] [review]
merge_helper-tools.diff

https://hg.mozilla.org/build/tools/rev/aae23b8c7ee3
Attachment #8410281 - Flags: checked-in+
Created attachment 8415442 [details] [diff] [review]
revert_locales.diff
Attachment #8415442 - Flags: review?(hwine)
(Assignee)

Comment 21

3 years ago
Comment on attachment 8415442 [details] [diff] [review]
revert_locales.diff

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

lgtm
Attachment #8415442 - Flags: review?(hwine) → review+
Comment on attachment 8415442 [details] [diff] [review]
revert_locales.diff

https://hg.mozilla.org/build/tools/rev/6ae2aa728cae
Attachment #8415442 - Flags: checked-in+

Comment 23

3 years ago
I'm going to mark this resolved.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Component: Tools → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.