1139066 - Support for git gecko repos

Status: RESOLVED FIXED

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

Description

[Wander Lairson Costa]

Today, mozharness can only download configs from mercurial remotes.

Comment 1

[Wander Lairson Costa]

Attachment: MozReview Request: bz://1139066/wcosta

/r/4623 - [PATCH 1/2] Bug 1139066 part 1: Use tc-vcs checkout command.
/r/4625 - [PATCH 2/2] Bug 1139066 part 2: Add support to download configs from git remotes.

Pull down these commits:

hg pull review -r b36bf949fca309ea8314676ec47a040cb350043a

Comment 2

[James Lal [:lightsofapollo]]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

vague r+ from me ... I only know some of these bits well. I am somewhat sad we end up duplicating this logic between tc-vcs and mozharness but I am not sure how to unify them yet.

Comment 3

[Jordan Lund (:jlund)]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

https://reviewboard.mozilla.org/r/4621/#review3853

this looks pretty good to me. Again I'm leaning on jlal for the correct behaviour logic for tcvcs

since all of mozharness/mozharness/mozilla/building/detect_repo.py is filled with static functions, do not avail of mozharness' logging/configs, and is a copy of another tool outside of mh, I think a more appropriate home is somewhere like mozharness/external_tools/detect_repo.py. what do you think? I'm open to your thoughts.

::: mozharness/base/vcs/tcvcs.py
(Diff revision 1)
> -
> +        if revision is None:

I think the above c.get('revision', '') should handle the default assignment of an empty string if c['revision'] equates to None.

::: mozharness/base/vcs/tcvcs.py
(Diff revision 1)
> -        branch = c.get('branch')
> +        branch = c.get('branch', '')

do we need branch anymore? were you planning on using it?

::: mozharness/mozilla/building/buildb2gbase.py
(Diff revision 1)
> -            return json.load(open(config_path, "r"))
> +            return self.retry(self.load_json_from_url, args=(url,))

nice, good call using self.retry

Comment 4

[Wander Lairson Costa]

(In reply to Jordan Lund (:jlund) from comment #3)
> Comment on attachment 8572351 [details]
> MozReview Request: bz://1139066/wcosta
> 
> https://reviewboard.mozilla.org/r/4621/#review3853
> 
> this looks pretty good to me. Again I'm leaning on jlal for the correct
> behaviour logic for tcvcs
> 
> since all of mozharness/mozharness/mozilla/building/detect_repo.py is filled
> with static functions, do not avail of mozharness' logging/configs, and is a
> copy of another tool outside of mh, I think a more appropriate home is
> somewhere like mozharness/external_tools/detect_repo.py. what do you think?
> I'm open to your thoughts.
> 

Sure, I am going to move this module to there.

> ::: mozharness/base/vcs/tcvcs.py
> (Diff revision 1)
> > -
> > +        if revision is None:
> 
> I think the above c.get('revision', '') should handle the default assignment
> of an empty string if c['revision'] equates to None.
> 

The current behavior of query_revision() is to return None if it fails. mozharness try to query the local repo revision, and if the repo doesn't exist, it will return None and this value will be assigned to config['revision']. So, c['revision'] will be found with value None, and None will be returned. That's why the if is necessary. I could change the query_revision() behavior to return an empty string, but I was afraid to break other stuff.

> ::: mozharness/base/vcs/tcvcs.py
> (Diff revision 1)
> > -        branch = c.get('branch')
> > +        branch = c.get('branch', '')
> 
> do we need branch anymore? were you planning on using it?
> 

Ops, I forgot that.

> ::: mozharness/mozilla/building/buildb2gbase.py
> (Diff revision 1)
> > -            return json.load(open(config_path, "r"))
> > +            return self.retry(self.load_json_from_url, args=(url,))
> 
> nice, good call using self.retry

Comment 5

[Wander Lairson Costa]

Attachment: MozReview Request: bz://1139066/wcosta

/r/4773 - Bug 1139066 part 1: Use tc-vcs checkout command.
/r/4775 - Bug 1139066 part 2: Add support to download configs from git remotes.

Pull down these commits:

hg pull review -r 6c5144bd4c38257376aa9303d5f233ab2a9aa9ac

Comment 6

[Wander Lairson Costa]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

/r/4623 - Bug 1139066 part 1: Use tc-vcs checkout command.
/r/4625 - Bug 1139066 part 2: Add support to download configs from git remotes.

Pull down these commits:

hg pull review -r 6c5144bd4c38257376aa9303d5f233ab2a9aa9ac

Comment 7

[Jordan Lund (:jlund)]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

https://reviewboard.mozilla.org/r/4621/#review3921

::: mozharness/mozilla/building/buildb2gbase.py
(Diff revision 2)
> +sys.path.append(os.path.join(os.path.dirname(sys.argv[0]), '..', 'external_tools'))

so I think we already:
sys.path.insert(1, os.path.dirname(sys.path[0]))

hence why we can do things like:
from mozharness.base.log import WARNING

This makes me think we don't need to modify the path further and instead just do:
from external_tools.detect_repo import foo, bar

If I'm right, I don't think that's a blocker to you landing but just an fyi

Comment 8

[Jordan Lund (:jlund)]

So, c['revision'] will be found with value None, and
> None will be returned. That's why the if is necessary. I could change the
> query_revision() behavior to return an empty string, but I was afraid to
> break other stuff.

makes sense

Comment 9

[Jordan Lund (:jlund)]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

https://reviewboard.mozilla.org/r/4621/#review3923

lgtm :) how much of this have you tested?

Comment 10

[Wander Lairson Costa]

(In reply to Jordan Lund (:jlund) from comment #9)
> Comment on attachment 8572351 [details]
> MozReview Request: bz://1139066/wcosta
> 
> https://reviewboard.mozilla.org/r/4621/#review3923
> 
> lgtm :) how much of this have you tested?

I have tried combinations of local and taskcluster builds with git and hg repositories. Any further test I can perform to avoid breaking existing stuff?

Comment 11

[Wander Lairson Costa]

(In reply to Jordan Lund (:jlund) from comment #7)
> Comment on attachment 8572351 [details]
> MozReview Request: bz://1139066/wcosta
> 
> https://reviewboard.mozilla.org/r/4621/#review3921
> 
> ::: mozharness/mozilla/building/buildb2gbase.py
> (Diff revision 2)
> > +sys.path.append(os.path.join(os.path.dirname(sys.argv[0]), '..', 'external_tools'))
> 
> so I think we already:
> sys.path.insert(1, os.path.dirname(sys.path[0]))
> 
> hence why we can do things like:
> from mozharness.base.log import WARNING
> 
> This makes me think we don't need to modify the path further and instead
> just do:
> from external_tools.detect_repo import foo, bar
> 
> If I'm right, I don't think that's a blocker to you landing but just an fyi

Makes sense, I will update the patch.

Comment 12

[Wander Lairson Costa]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

/r/4623 - Bug 1139066 part 1: Use tc-vcs checkout command.
/r/4625 - Bug 1139066 part 2: Add support to download configs from git remotes.

Pull down these commits:

hg pull review -r 34823757addcf582918913896e604ac9ddd48735

Comment 13

[Wander Lairson Costa]

I updated the patch based on the last comments, plus two small updates:

1) I noticed that a change piece in part 2 actually belongs to part 1
2) The query_gecko_config function was broken for git.m.o, I fixed it.

Comment 14

[Jordan Lund (:jlund)]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

https://reviewboard.mozilla.org/r/4621/#review3953

Ship It!

Comment 15

[Jordan Lund (:jlund)]

(In reply to Wander Lairson Costa [:wcosta] from comment #10)
> (In reply to Jordan Lund (:jlund) from comment #9)
> > Comment on attachment 8572351 [details]
> > MozReview Request: bz://1139066/wcosta
> > 
> > https://reviewboard.mozilla.org/r/4621/#review3923
> > 
> > lgtm :) how much of this have you tested?
> 
> I have tried combinations of local and taskcluster builds with git and hg
> repositories. Any further test I can perform to avoid breaking existing
> stuff?

sounds sufficient. I wonder if there will be an odd buildbot issue that I'm not seeing. However, since mh is pinned on each branch, I'm fine with landing this and trying it out.

lgtm :)

Comment 16

[Steve Fink [:sfink] [:s:]]

I'm guessing this must be source of the (nonfatal) AttributeError: 'GitConfig' object has no attribute 'json' I'm seeing here in https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c1a52ce860 :

14:31:53     INFO - Deleting broken projects...
14:31:53     INFO - Running command: ['./repo', 'forall', '-c', 'git show-ref -q --head HEAD || rm -rfv $PWD'] in /builds/slave/b2g_try_l64-b2g-haz_dep-000000/build
14:31:53     INFO - Copy/paste: ./repo forall -c "git show-ref -q --head HEAD || rm -rfv $PWD"
14:31:53     INFO -  Traceback (most recent call last):
14:31:53     INFO -    File "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py", line 506, in <module>
14:31:53     INFO -      _Main(sys.argv[1:])
14:31:53     INFO -    File "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py", line 482, in _Main
14:31:53     INFO -      result = repo._Run(argv) or 0
14:31:53     INFO -    File "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py", line 161, in _Run
14:31:53     INFO -      result = cmd.Execute(copts, cargs)
14:31:53     INFO -    File "/builds/git-shared/repo/repo/subcmds/forall.py", line 200, in Execute
14:31:53     INFO -      mirror = self.manifest.IsMirror
14:31:53     INFO -    File "/builds/git-shared/repo/repo/manifest_xml.py", line 358, in IsMirror
14:31:53     INFO -      return self.manifestProject.config.GetBoolean('repo.mirror')
14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py", line 112, in GetBoolean
14:31:53     INFO -      v = self.GetString(name)
14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py", line 129, in GetString
14:31:53     INFO -      v = self._cache[_key(name)]
14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py", line 246, in _cache
14:31:53     INFO -      self._cache_dict = self._Read()
14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py", line 253, in _Read
14:31:53     INFO -      self._SaveJson(d)
14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py", line 283, in _SaveJson
14:31:53     INFO -      if os.path.exists(self.json):
14:31:53     INFO -  AttributeError: 'GitConfig' object has no attribute 'json'

Comment 17

[Wander Lairson Costa]

(In reply to Steve Fink [:sfink, :s:] from comment #16)
> I'm guessing this must be source of the (nonfatal) AttributeError:
> 'GitConfig' object has no attribute 'json' I'm seeing here in
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c1a52ce860 :
> 
> 14:31:53     INFO - Deleting broken projects...
> 14:31:53     INFO - Running command: ['./repo', 'forall', '-c', 'git
> show-ref -q --head HEAD || rm -rfv $PWD'] in
> /builds/slave/b2g_try_l64-b2g-haz_dep-000000/build
> 14:31:53     INFO - Copy/paste: ./repo forall -c "git show-ref -q --head
> HEAD || rm -rfv $PWD"
> 14:31:53     INFO -  Traceback (most recent call last):
> 14:31:53     INFO -    File
> "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py",
> line 506, in <module>
> 14:31:53     INFO -      _Main(sys.argv[1:])
> 14:31:53     INFO -    File
> "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py",
> line 482, in _Main
> 14:31:53     INFO -      result = repo._Run(argv) or 0
> 14:31:53     INFO -    File
> "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py",
> line 161, in _Run
> 14:31:53     INFO -      result = cmd.Execute(copts, cargs)
> 14:31:53     INFO -    File
> "/builds/git-shared/repo/repo/subcmds/forall.py", line 200, in Execute
> 14:31:53     INFO -      mirror = self.manifest.IsMirror
> 14:31:53     INFO -    File "/builds/git-shared/repo/repo/manifest_xml.py",
> line 358, in IsMirror
> 14:31:53     INFO -      return
> self.manifestProject.config.GetBoolean('repo.mirror')
> 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> line 112, in GetBoolean
> 14:31:53     INFO -      v = self.GetString(name)
> 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> line 129, in GetString
> 14:31:53     INFO -      v = self._cache[_key(name)]
> 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> line 246, in _cache
> 14:31:53     INFO -      self._cache_dict = self._Read()
> 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> line 253, in _Read
> 14:31:53     INFO -      self._SaveJson(d)
> 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> line 283, in _SaveJson
> 14:31:53     INFO -      if os.path.exists(self.json):
> 14:31:53     INFO -  AttributeError: 'GitConfig' object has no attribute
> 'json'

This happens when you run b2g_build.py in a fresh clone of B2G repo. I tried here and even mozharness master has this error. It then runs config.sh to clone the repos in the manifest file. I ignored because this error looked expect in the code flow.

Comment 18

[Wander Lairson Costa]

(In reply to Wander Lairson Costa [:wcosta] from comment #17)
> (In reply to Steve Fink [:sfink, :s:] from comment #16)
> > I'm guessing this must be source of the (nonfatal) AttributeError:
> > 'GitConfig' object has no attribute 'json' I'm seeing here in
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c1a52ce860 :
> > 
> > 14:31:53     INFO - Deleting broken projects...
> > 14:31:53     INFO - Running command: ['./repo', 'forall', '-c', 'git
> > show-ref -q --head HEAD || rm -rfv $PWD'] in
> > /builds/slave/b2g_try_l64-b2g-haz_dep-000000/build
> > 14:31:53     INFO - Copy/paste: ./repo forall -c "git show-ref -q --head
> > HEAD || rm -rfv $PWD"
> > 14:31:53     INFO -  Traceback (most recent call last):
> > 14:31:53     INFO -    File
> > "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py",
> > line 506, in <module>
> > 14:31:53     INFO -      _Main(sys.argv[1:])
> > 14:31:53     INFO -    File
> > "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py",
> > line 482, in _Main
> > 14:31:53     INFO -      result = repo._Run(argv) or 0
> > 14:31:53     INFO -    File
> > "/builds/slave/b2g_try_l64-b2g-haz_dep-000000/build/.repo/repo/main.py",
> > line 161, in _Run
> > 14:31:53     INFO -      result = cmd.Execute(copts, cargs)
> > 14:31:53     INFO -    File
> > "/builds/git-shared/repo/repo/subcmds/forall.py", line 200, in Execute
> > 14:31:53     INFO -      mirror = self.manifest.IsMirror
> > 14:31:53     INFO -    File "/builds/git-shared/repo/repo/manifest_xml.py",
> > line 358, in IsMirror
> > 14:31:53     INFO -      return
> > self.manifestProject.config.GetBoolean('repo.mirror')
> > 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> > line 112, in GetBoolean
> > 14:31:53     INFO -      v = self.GetString(name)
> > 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> > line 129, in GetString
> > 14:31:53     INFO -      v = self._cache[_key(name)]
> > 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> > line 246, in _cache
> > 14:31:53     INFO -      self._cache_dict = self._Read()
> > 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> > line 253, in _Read
> > 14:31:53     INFO -      self._SaveJson(d)
> > 14:31:53     INFO -    File "/builds/git-shared/repo/repo/git_config.py",
> > line 283, in _SaveJson
> > 14:31:53     INFO -      if os.path.exists(self.json):
> > 14:31:53     INFO -  AttributeError: 'GitConfig' object has no attribute
> > 'json'
> 
> This happens when you run b2g_build.py in a fresh clone of B2G repo. I tried
> here and even mozharness master has this error. It then runs config.sh to
> clone the repos in the manifest file. I ignored because this error looked
> expect in the code flow.

I went a bit further into this, and it feels like the problem is that "repo forall" is called before "repo init". If I understood the code piece correctly [1], this behavior is expected. Is this a blocker to land this bug?

[1] https://github.com/mozilla/build-mozharness/blob/master/mozharness/mozilla/building/buildb2gbase.py#L396-L417

Comment 19

[Wander Lairson Costa]

Ops, requested NI from wrong person =p

Comment 20

[Wander Lairson Costa]

^sfink

Comment 21

[Steve Fink [:sfink] [:s:]]

No, I would not block landing for it. It is disturbing, though, since when you click on a clean hazard build on TH it shows an error message. But that's not the end of the world.

Comment 22

[Wander Lairson Costa]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

/r/4623 - Bug 1139066 part 1: Use tc-vcs checkout command.
/r/4625 - Bug 1139066 part 2: Add support to download configs from git remotes.

Pull down these commits:

hg pull review -r 14efdd6647b528075dc7cf8ea9366223238da47d

Comment 23

[Wander Lairson Costa]

According to what we talked on irc, we modified patch 2 to fix 3 problems:

- Remove the braino "if True:"
- Make the patches pyflakes clean
- Make query_revision return None when the local repository doesn't exist.

Comment 24

[Steve Fink [:sfink] [:s:]]

https://reviewboard.mozilla.org/r/4625/#review4171

Ship It!

Comment 25

[Steve Fink [:sfink] [:s:]]

remote:   https://hg.mozilla.org/build/mozharness/rev/6b7d8d086e8a
remote:   https://hg.mozilla.org/build/mozharness/rev/14efdd6647b5

Comment 26

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

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/6b7d8d086e8a

Comment 27

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

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/14efdd6647b5

Comment 28

[Wander Lairson Costa]

Can the bug be closed?

Comment 29

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

(In reply to Wander Lairson Costa [:wcosta] from comment #28)
> Can the bug be closed?

That depends. If the trees you care about have in-tree pinning for mozharness, you will need to update the revision they point to if you have not already done so. Otherwise, yes, you can close the bug.

Comment 30

[Steve Fink [:sfink] [:s:]]

https://reviewboard.mozilla.org/r/4623/#review8189

Ship It!

Comment 31

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8572351 [details]
MozReview Request: bz://1139066/wcosta

https://reviewboard.mozilla.org/r/4621/#review8191

Ship It!

Comment 32

[Steve Fink [:sfink] [:s:]]

(Sorry for the bug spam; I had just long ago left this one at r?me because I wanted to come back sometime to see if I could understand why reviewboard never marked it r+.)

Comment 34

[Wander Lairson Costa]

Attachment: MozReview Request: Bug 1139066 part 2: Add support to download configs from git remotes.

Comment 35

[Wander Lairson Costa]

Attachment: MozReview Request: Bug 1139066 part 1: Use tc-vcs checkout command.