Closed Bug 1109346 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jlal, Assigned: wcosta)

References

Details

Attachments

(3 files, 3 obsolete files)

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)
Assignee: nobody → wcosta
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Attached file MozReview Request: bz://1109346/wcosta (obsolete) —
Attachment #8537810 - Flags: review?(jlal)
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r c54a5679edf6710f1169d53f3b3908d57e097d41
Attached file MozReview Request: bz://1109346/wcosta (obsolete) —
Attachment #8538154 - Flags: review?(catlee)
/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=catlee

Pull down this commit:

hg pull review -r 24d8c5a7c745b01025a0e135375f773727567847
Attachment #8538154 - Flags: review?(jlal)
/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=catlee

Pull down this commit:

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

Pull down this commit:

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

Pull down this commit:

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

Pull down this commit:

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

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 612c93dddb3ca8dc9c4abb5052b7eb3734290b71
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 on attachment 8537810 [details]
MozReview Request: bz://1109346/wcosta

one comment to address otherwise lgtm
Attachment #8537810 - Flags: review?(jlal) → review+
Comment on attachment 8538154 [details]
MozReview Request: bz://1109346/wcosta

tc-vcs bits lgtm
Attachment #8538154 - Flags: review?(jlal) → review+
Attachment #8538154 - Flags: review?(jlund)
Attachment #8538154 - Flags: review?(catlee)
Attachment #8538154 - Flags: review+
/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r ce9ce960cda87f2417e1c087ffd5d626b9c5b9e1
Attachment #8537810 - Flags: review+ → review?(jlal)
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 1b003db06c5196cf75eb5446d1c072e148aa9faa
/r/1535 - Bug 1109346: Use tc-vcs for cloning repos. r=lightsofapollo

Pull down this commit:

hg pull review -r 1b003db06c5196cf75eb5446d1c072e148aa9faa
Attachment #8537810 - Flags: review?(jlal) → review+
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
Attached file Mozharness build log (obsolete) —
: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
Flags: needinfo?(jlund)
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.
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
Flags: needinfo?(jlund)
/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r 978d1df59e9855471b201a7908c1d187a7fb946e
/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r 749bd24a1208f076a926fe17cd4b55938c818e56
(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 :)
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.
r+ if you remove output_parser=parser and the changes are tested :)
/r/1555 - Bug 1109346: Add taskcluster-vcs support to mozharness. r=jlund

Pull down this commit:

hg pull review -r 5802e03817a97d5026be6b253d21291b5774b40f
Attachment #8541323 - Attachment is obsolete: true
(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.
Blocks: 1085632
Blocks: 1085633
Blocks: 1085634
Blocks: 1085636
Blocks: 1085637
Blocks: 1085638
Blocks: 1085639
Blocks: 1085641
Attachment #8538154 - Flags: review?(jlund) → review+
checking-needed only for mozharness part, gecko part will be pushed to alder (perhaps a should have filed two bugs).
Component: TaskCluster → Mozharness
Keywords: checkin-needed
Product: Testing → Release Engineering
QA Contact: jlund
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Flags: needinfo?(wcosta)
Resolution: FIXED → ---
Attachment #8538154 - Flags: review+ → review?(pmoore)
/r/1555 - Bug 1109346: Add the tcvcs.py script.

Pull down this commit:

hg pull review -r 3029a11dec0e413295077e8ccfd3686150a25820
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.
Flags: needinfo?(wcosta)
https://hg.mozilla.org/build/mozharness/rev/3029a11dec0e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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.
Attachment #8538154 - Flags: review?(pmoore) → review+
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/armenzg@mozilla.com-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.
Attachment #8538154 - Attachment is obsolete: true
Attachment #8537810 - Attachment is obsolete: true
Attachment #8618876 - Flags: review+
Attachment #8618877 - Flags: review+
You need to log in before you can comment on or make changes to this bug.