927707 - vcs-sync: combined mapfile support, improved configs, project-branches force_push

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Comment 1

[Aki Sasaki (not active)]

Attachment: combine_mapfile.diff

This patch:

* removes the standalone test_push repos.  Since our answer for bug 847643 is going to be "nuke those repos" rather than "populate those repos in an additional push from the same process", these aren't needed anymore,

* removes the "vcs": "git" definition per non-test_push target.  The remote_targets config already specifies the vcs type, so specifying it again in the per-repo targets list-of-dicts was ugly,

* updates the *_share_base comment,

* makes the gecko-git conversion process spammy (so I see empty messages in my email),

* adds a combined_mapfile for l10n,

* adds a combined_mapfile for the project-branches.  I debated whether to do this from the gecko-dev process or the project-branches process.  My reasoning here is I'd rather the project-branches process die from people.m.o being unavailable than gecko-dev, but either way the combined_mapfile is going to get stale until it works again.

* adds a combine-mapfile action, and makes the mapfile paths all relative to abs_upload_dir so the sort commandline isn't huge and the softlink doesn't go to an absolute path,

* pulls some logic out of the |if git| block in push() so we can have a common set of variables in hg or git, test_push or remote_target.

I've tested this with both l10n and project-branches.

Comment 2

[Hal Wine [:hwine] use NI!]

Comment on attachment 818197 [details] [diff] [review]
combine_mapfile.diff

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

looks good -- One change needed -- comment marked "BUG". 

r+ assuming that fixed.

::: scripts/vcs-sync/vcs_sync.py
@@ +610,1 @@
>          """ Ported from repo-sync-tools/combine_mapfiles

"Adapted from" would be more accurate. The original had to deal with asynchronous reading of mapfiles, leading to complexity in file system operations. 

Since this system uses a synchronous push of the mapfile that complexity can be voided. (Likely only an issue for me, who got confused by knowing the original.)

@@ +628,3 @@
>              for f in existing_mapfiles:
> +                f_path = os.path.join(cwd, f)
> +                if time.ctime(os.path.getmtime(f_path)) > combined_timestamp:

BUG: I don't believe you want time.ctime() here -- that will produce a string that would compare differently from the numeric seconds.

@@ +637,2 @@
>          output = self.get_output_from_command(
> +            ['sort', '--unique', '-t', ' ',

Just curious -- why switch to '-t' from '--field-separator' -- everything else is long form. (I will note the old long form was misspelled -- must have been written on talk-like-a-pirate-day ;)

Comment 3

[Aki Sasaki (not active)]

Comment on attachment 818197 [details] [diff] [review]
combine_mapfile.diff

http://hg.mozilla.org/build/mozharness/rev/3a2d7e2f5074

(In reply to Hal Wine [:hwine] (use needinfo) from comment #2)
> ::: scripts/vcs-sync/vcs_sync.py
> @@ +610,1 @@
> >          """ Ported from repo-sync-tools/combine_mapfiles
> 
> "Adapted from" would be more accurate.

Fixed.

> @@ +628,3 @@
> >              for f in existing_mapfiles:
> > +                f_path = os.path.join(cwd, f)
> > +                if time.ctime(os.path.getmtime(f_path)) > combined_timestamp:
> 
> BUG: I don't believe you want time.ctime() here -- that will produce a
> string that would compare differently from the numeric seconds.

Fixed, good catch!

> @@ +637,2 @@
> >          output = self.get_output_from_command(
> > +            ['sort', '--unique', '-t', ' ',
> 
> Just curious -- why switch to '-t' from '--field-separator' -- everything
> else is long form. (I will note the old long form was misspelled -- must
> have been written on talk-like-a-pirate-day ;)

The list-style command didn't like having the option="value" all in one quoted argument, and I wasn't sure if the option required the = or not.

Comment 4

[Aki Sasaki (not active)]

Transplanted to production.