1109346 - gecko: Emulator mozharness scripts should be able to use tc-vcs

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P1)

Description

[James Lal [:lightsofapollo]]

tc-vcs makes it pretty easy to wrap vcs of choice (this already works with caching for all of our gecko build jobs except for emulator/phone) so we should also make it work for phones/emulators then supporting use cases like "git try" and "user repos" will work easily (both git/hg variants)

Comment 1

[Wander Lairson Costa]

Attachment: MozReview Request: bz://1109346/wcosta

Comment 2

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r c54a5679edf6710f1169d53f3b3908d57e097d41

Comment 3

[Wander Lairson Costa]

Attachment: MozReview Request: bz://1109346/wcosta

Comment 4

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=catlee

Pull down this commit:

hg pull review -r 24d8c5a7c745b01025a0e135375f773727567847

Comment 5

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=catlee

Pull down this commit:

hg pull review -r ea513cf1433654a9e1b5ab49d66834debc2a8563

Comment 6

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=catlee

Pull down this commit:

hg pull review -r e9dd43e05a8b4b91b97ee980099f4f5440e98fbf

Comment 7

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=catlee

Pull down this commit:

hg pull review -r e9dd43e05a8b4b91b97ee980099f4f5440e98fbf

Comment 8

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r a2917ffa1fa01dff2ae7edf6d069b87279aad319

Comment 9

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71

Comment 10

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71

Comment 11

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71

Comment 12

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71

Comment 13

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71

Comment 14

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71

Comment 15

[James Lal [:lightsofapollo]]

https://reviewboard.mozilla.org/r/1533/#review1075

::: testing/docker/phone-builder/bin/validate_task.py
(Diff revision 4)
> -    if 'REPOSITORY' not in payload['env']:
> +    if 'GECKO_HEAD_REPOSITORY' not in payload['env']:

We should check both base and HEAD I think

Comment 16

[James Lal [:lightsofapollo]]

Comment on attachment 8537810 [details]
MozReview Request: bz://1109346/wcosta

one comment to address otherwise lgtm

Comment 17

[James Lal [:lightsofapollo]]

Comment on attachment 8538154 [details]
MozReview Request: bz://1109346/wcosta

tc-vcs bits lgtm

Comment 18

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r ce9ce960cda87f2417e1c087ffd5d626b9c5b9e1

Comment 19

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 1b003db06c5196cf75eb5446d1c072e148aa9faa

Comment 20

[Wander Lairson Costa]

/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 1b003db06c5196cf75eb5446d1c072e148aa9faa

Comment 21

[Jordan Lund (:jlund)]

https://reviewboard.mozilla.org/r/1553/#review1111

looks really good to me :D

this is great to see more vcs tools getting snapped into VCSMixin. I have some nits/comments in the review and so before I r+, I'd like to address those and also see this patch being actually used with log output.

as for the tc-vcs cmd iteslf and valid args, I'll trust that jlal r+ was verified that.

::: mozharness/base/vcs/tcvcs.py
(Diff revision 5)
> +        self.tc_vcs = self.query_exe('tc-vcs', return_type='list')

looks like you never add this to self.config['exes'], so I'm assuming you will always have 'tc-vcs' in your path?

::: mozharness/base/vcs/tcvcs.py
(Diff revision 5)
> +            if retval != 0:

IIUC you can s/if retval != 0:/if retval:/ everywhere in this file. or, if you want, simply, `if self.run_command(cmd):`

::: mozharness/base/vcs/tcvcs.py
(Diff revision 5)
> +        parser.got_revision

unlike ruby, I don't think python will return the last statement in a function. IIRC, you must explicitly use `return X`

::: mozharness/base/vcs/tcvcs.py
(Diff revision 5)
> +        retval = self.run_command(cmd, output_parser=parser)

IIUC you are only expecting the revision as output from this command. instead of adding a custom OutputParser that assigns self.got_revision to the latest line of output (in this case, we only expect one line so it works), you can also use this[1].

e.g.
return self.get_output_from_command(cmd)

[1] http://mxr.mozilla.org/build/source/mozharness/mozharness/base/script.py#780

::: scripts/b2g_build.py
(Diff revision 5)
> +            "dest": "base_repo",

I'm confused why we need --repo[1] and --base-repo

in TcVCS, you have:
base_repo = c.get('base_repo', c['repo'])

are you expecting c['base_repo'] to be different than c['repo'] if c['base_repo'] exists?


[1] http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildb2gbase.py#48

Comment 22

[Wander Lairson Costa]

Attachment: Mozharness build log

:jlund,

Here is piece of log related to the patch

18:23:49     INFO - retry: Calling <bound method B2GBuild._get_revision of <__main__.B2GBuild object at 0x7f0d8f82ea90>> with args: (<mozharness.base.vcs.tcvcs.TcVCS object at 0x7f0d8f6e1f10>, '/home/wander/work/build/gecko'), kwargs: {}, attempt #1
18:23:49     INFO - Running command: ['tc-vcs', 'checkout-revision', '/home/wander/work/build/gecko', 'https://hg.mozilla.org/try', 'master', 'c355549c9381']
18:23:49     INFO - Copy/paste: tc-vcs checkout-revision /home/wander/work/build/gecko https://hg.mozilla.org/try master c355549c9381
18:23:49     INFO -  [tc-vcs] run start : hg pull -r c355549c9381 https://hg.mozilla.org/try
18:23:53     INFO -  trazendo revisões de https://hg.mozilla.org/try
18:23:53     INFO -  nenhuma alteração encontrada
18:23:53     INFO -  [tc-vcs] run end : hg pull -r c355549c9381 https://hg.mozilla.org/try (0) in 4468 ms
18:23:53     INFO -  [tc-vcs] run start : hg update -C c355549c9381
18:23:54     INFO -  0 arquivos atualizados, 0 arquivos mesclados, 0 arquivos removidos, 0 arquivos não resolvidos
18:23:54     INFO -  [tc-vcs] run end : hg update -C c355549c9381 (0) in 959 ms
18:23:54     INFO - Return code: 0

Comment 23

[Wander Lairson Costa]

https://reviewboard.mozilla.org/r/1553/#review1113

> looks like you never add this to self.config['exes'], so I'm assuming you will always have 'tc-vcs' in your path?

That's correct, at least in current situation, do you think would be better to add it to config['exes']?

> IIUC you can s/if retval != 0:/if retval:/ everywhere in this file. or, if you want, simply, `if self.run_command(cmd):`

Sure, just copy paste from other module. I'll fix that.

> IIUC you are only expecting the revision as output from this command. instead of adding a custom OutputParser that assigns self.got_revision to the latest line of output (in this case, we only expect one line so it works), you can also use this[1].
> 
> e.g.
> return self.get_output_from_command(cmd)
> 
> [1] http://mxr.mozilla.org/build/source/mozharness/mozharness/base/script.py#780

Ah, I didn't know that, great, I'll use it.

> unlike ruby, I don't think python will return the last statement in a function. IIRC, you must explicitly use `return X`

Ops, you're right.

> I'm confused why we need --repo[1] and --base-repo
> 
> in TcVCS, you have:
> base_repo = c.get('base_repo', c['repo'])
> 
> are you expecting c['base_repo'] to be different than c['repo'] if c['base_repo'] exists?
> 
> 
> [1] http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildb2gbase.py#48

Yep, this is for try support, we use mozilla-central as base repo, and pull latest commits from try. So we have faster cloning because m-c is cached.

Comment 24

[Jordan Lund (:jlund)]

thanks for the log snippet. based on your last comment, it sounds like you are going to remove/change a few things: 1) retval conditions 2) output parser 3) return statement in ensure_repo_and_rev.

I'll clear the review out of my queue for now. feel free to either interdiff with a new patch or else submit the whole thing again :)

(In reply to Wander Lairson Costa [:wcosta] from comment #23)
> https://reviewboard.mozilla.org/r/1553/#review1113
> 

> That's correct, at least in current situation, do you think would be better
> to add it to config['exes']?

just asserting that it will always be there as I don't know how tc works and sets things like PATH. your way wfm :)

> > I'm confused why we need --repo[1] and --base-repo
> > 
> > in TcVCS, you have:
> > base_repo = c.get('base_repo', c['repo'])
> > 
> > are you expecting c['base_repo'] to be different than c['repo'] if c['base_repo'] exists?
> > 
> > 
> > [1] http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildb2gbase.py#48
> 
> Yep, this is for try support, we use mozilla-central as base repo, and pull
> latest commits from try. So we have faster cloning because m-c is cached.

oic, thanks for clarifying

Comment 25

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r 978d1df59e9855471b201a7908c1d187a7fb946e

Comment 26

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r 749bd24a1208f076a926fe17cd4b55938c818e56

Comment 27

[Wander Lairson Costa]

(In reply to Jordan Lund (:jlund) from comment #24)
> thanks for the log snippet. based on your last comment, it sounds like you
> are going to remove/change a few things: 1) retval conditions 2) output
> parser 3) return statement in ensure_repo_and_rev.
> 
> I'll clear the review out of my queue for now. feel free to either interdiff
> with a new patch or else submit the whole thing again :)
> 
> (In reply to Wander Lairson Costa [:wcosta] from comment #23)
> > https://reviewboard.mozilla.org/r/1553/#review1113
> > 
> 
> > That's correct, at least in current situation, do you think would be better
> > to add it to config['exes']?
> 
> just asserting that it will always be there as I don't know how tc works and
> sets things like PATH. your way wfm :)
> 
> > > I'm confused why we need --repo[1] and --base-repo
> > > 
> > > in TcVCS, you have:
> > > base_repo = c.get('base_repo', c['repo'])
> > > 
> > > are you expecting c['base_repo'] to be different than c['repo'] if c['base_repo'] exists?
> > > 
> > > 
> > > [1] http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildb2gbase.py#48
> > 
> > Yep, this is for try support, we use mozilla-central as base repo, and pull
> > latest commits from try. So we have faster cloning because m-c is cached.
> 
> oic, thanks for clarifying


Just pushed the patch again with the requested changes :)

Comment 28

[Jordan Lund (:jlund)]

https://reviewboard.mozilla.org/r/1553/#review1175

::: mozharness/base/vcs/tcvcs.py
(Diff revision 7)
> +        return self.get_output_from_command(cmd, output_parser=parser)

parser no longer exists. I think you forgot to remove the reference.

Comment 29

[Jordan Lund (:jlund)]

r+ if you remove output_parser=parser and the changes are tested :)

Comment 30

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r 5802e03817a97d5026be6b253d21291b5774b40f

Comment 31

[Wander Lairson Costa]

Attachment: Build log after requested changes

Comment 32

[Wander Lairson Costa]

(In reply to Jordan Lund (:jlund) from comment #29)
> r+ if you remove output_parser=parser and the changes are tested :)

Ops, fixed now, I attached a new build log generated after the changes.

Comment 33

[Jordan Lund (:jlund)]

https://reviewboard.mozilla.org/r/1553/#review1201

Ship It!

Comment 34

[Wander Lairson Costa]

checking-needed only for mozharness part, gecko part will be pushed to alder (perhaps a should have filed two bugs).

Comment 35

[Wander Lairson Costa]

https://hg.mozilla.org/projects/alder/rev/b71cb513b030

Comment 36

[Pete Moore [:pmoore][:pete]]

This has broken the unit tests in travis, e.g. see:
https://travis-ci.org/mozilla/build-mozharness/builds/46234845


1) ERROR: Failure: ImportError (No module named tcvcs)

   Traceback (most recent call last):
    .tox/py27/lib/python2.7/site-packages/nose/loader.py line 414 in loadTestsFromName
      addr.filename, addr.module)
    .tox/py27/lib/python2.7/site-packages/nose/importer.py line 47 in importFromPath
      return self.importFromDir(dir_path, fqname)
    .tox/py27/lib/python2.7/site-packages/nose/importer.py line 94 in importFromDir
      mod = load_module(part_fqname, fh, filename, desc)
    mozharness/mozilla/testing/gaia_test.py line 20 in <module>
      from mozharness.base.vcs.vcsbase import MercurialScript
    mozharness/base/vcs/vcsbase.py line 22 in <module>
      from mozharness.base.vcs.tcvcs import TcVCS
   ImportError: No module named tcvcs



Hopefully this should be reproducible locally with:

pip install tox
tox

Thanks,
Pete

Comment 37

[Wander Lairson Costa]

/r/1555 - Bug 1109346: Add the tcvcs.py script.

Pull down this commit:

hg pull review -r 3029a11dec0e413295077e8ccfd3686150a25820

Comment 38

[Wander Lairson Costa]

I don't know how, but the tcvcs.py was not included in the commit pushed. Found a minor typo in taskcluster-phone.py file also.

Comment 39

[Wander Lairson Costa]

https://hg.mozilla.org/build/mozharness/rev/3029a11dec0e

Comment 40

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8538154 [details]
MozReview Request: bz://1109346/wcosta

I landed a bustage fix for this:
https://hg.mozilla.org/build/mozharness/rev/073d426649ee
https://hg.mozilla.org/build/mozharness/rev/3029a11dec0e

I didn't do this via reviewboard as I was nervous it might roll back the other files.

Comment 41

[Armen [:armenzg]]

This change is affecting my try pushes:
http://hg.mozilla.org/build/mozharness/rev/5371983056ce
with:
> Traceback (most recent call last):
>   File "scripts/scripts/android_emulator_unittest.py", line 23, in <module>
>     from mozharness.base.vcs.vcsbase import VCSMixin
>   File "/builds/slave/test/scripts/mozharness/base/vcs/vcsbase.py", line 22, in <module>
>     from mozharness.base.vcs.tcvcs import TcVCS
> ImportError: No module named tcvcs

http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-ecdb1c21ae76/try-android-api-9/try_ubuntu64_vm_mobile_test-mochitest-gl-1-bm116-tests1-linux64-build153.txt.gz

I've now re-merge to pick 3029a11dec0e up. Let's see if I hit the issue again or not.

Comment 42

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/5371983056ce

Comment 43

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/3029a11dec0e

Comment 44

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/073d426649ee

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d7d6fd27d36

Comment 46

[Pete Moore [:pmoore][:pete]]

https://reviewboard.mozilla.org/r/1555/#review6673

This has already landed in https://bugzilla.mozilla.org/show_bug.cgi?id=1109346#c40, following up with formal review in reviewboard.

Comment 48

[Wander Lairson Costa]

Attachment: MozReview Request: Bug 1109346: Add the tcvcs.py script.

Comment 50

[Wander Lairson Costa]

Attachment: MozReview Request: Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo