Closed Bug 838321 Opened 9 years ago Closed 9 years ago

Use sources.xml to checkout b2g sources

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: catlee, Assigned: catlee)

References

Details

(Whiteboard: [b2g])

Attachments

(4 files, 1 obsolete file)

Instead of using a static snapshot of b2g, we should check out sources directly from git.m.o.
>+    "repo_remote_mappings": {
>+        'https://android.googlesource.com/': 'https://git.mozilla.org/external/aosp',
>+        'git://codeaurora.org/': 'https://git.mozilla.org/external/caf',
>+        'https://git.mozilla.org/b2g': 'https://git.mozilla.org/b2g',
>+        'git://github.com/mozilla-b2g/': 'https://git.mozilla.org/b2g',
>+        'git://github.com/mozilla/': 'https://git.mozilla.org/b2g',
>+        'https://git.mozilla.org/releases': 'https://git.mozilla.org/releases',
>+        'http://android.git.linaro.org/git-ro/': 'https://git.mozilla.org/external/linaro',
>+    },

This does seem fragile, and we both don't know how to fix it elegantly.
Does the manifest-monitoring cron job reduce this risk?
I'm not going to block on this, just noting it as something we'll have to keep an eye on.

>+    def load_json_from_url(self, url, timeout=30):
>+        self.debug("Attempting to download %s; timeout=%i" % (url, timeout))
>+        r = urllib2.urlopen(url, timeout=timeout)
>+        j = json.load(r)
>+        return j

I guess the self.retry() logs the attempt so debug() is ok.
Otherwise I would have a log_level=INFO or log_level=DEBUG and self.log(message, level=log_level)

>+++ b/mozharness/mozilla/repo_manifest.py

We don't have any logging in this file.
It it likely that we'll need to debug unexpected behavior in these methods later?

If we need to add logging, we could, f/e,

def load_manifest(filename, log_obj=None):
    ...
    if log_obj is not None:
        log_obj.info(message)
    ...

and pass self in as log_obj.

>+                                # Deprecated
>                                 'checkout-gecko',
>-                                # Download via tooltool repo in gecko checkout or via explicit url
>                                 'download-gonk',
>                                 'unpack-gonk',
>                                 'checkout-gaia',
>                                 'checkout-gaia-l10n',
>                                 'checkout-gecko-l10n',
>                                 'checkout-compare-locales',
>+                                # End deprecated

Do we know when the old method of checkouts are removable?
Might be nice to add a comment about that if so.

>                             default_actions=[
>-                                'checkout-gecko',
>-                                'download-gonk',
>-                                'unpack-gonk',
>+                                'checkout-sources',
>+                                'get-blobs',
>                                 'build',
>                             ],

Are we bitten by the fact that we now have a much larger set of default actions (since checkout-sources calls more deprecated actions than are listed here)?

>+    def query_remote_gecko_config(self):
>+        repo = self.query_repo()
>+        # TODO: Hardcoding this sucks
>+        if 'hg.mozilla.org' in repo:
>+            rev = self.query_revision()
>+            if rev is None:
>+                rev = 'default'
>+
>+            config_path = self.query_gecko_config_path()
>+            # Handle local files vs. in-repo files
>+            url = self.query_hgweb_url(repo, rev, config_path)
>+            return self.retry(self.load_json_from_url, args=(url,))

Hm, what happens if this fails?

>+            repos = [
>+                {'vcs': 'gittool', 'repo': 'https://git.mozilla.org/b2g/B2G.git', 'dest': dirs['work_dir']},
>+                {'vcs': 'gittool', 'repo': 'https://git.mozilla.org/b2g/b2g-manifest.git', 'dest': os.path.join(dirs['work_dir'], 'b2g-manifest'), 'branch': b2g_manifest_branch},
>+            ]
>+            self.vcs_checkout_repos(repos)

Known hardcodes.
If we ever want to fork to test something, we can get these from config, but I'm going to let this slide, I think.

I don't think I could catch everything in this review, but we can land on default and run some builds on Cedar.  I'm ok landing as-is if you are; if you want to address some of the above questions first that's cool too.  I imagine the work week will mandate timing.
Priority: -- → P2
Attached patch gittool changes (obsolete) — Splinter Review
gittool needs some changes to work well with this. the most important change is moving to a local share per remote repo (like hg) instead of a global share.

this patch also handles some errors I hit in staging.
Attachment #737632 - Flags: review?(rail)
Comment on attachment 737632 [details] [diff] [review]
gittool changes

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

::: lib/python/util/git.py
@@ +25,5 @@
>          repo = os.path.abspath(repo)
>      return repo
>  
>  
> +def get_repo_name(repo):

Can you add tests for this method?

@@ +51,5 @@
>      except subprocess.CalledProcessError:
>          return False
>  
>  
> +def is_commitid(refname):

and for this one

@@ +53,5 @@
>  
>  
> +def is_commitid(refname):
> +    """Returns True if refname is a valid commit id (contains only a-f0-9), and len == 20"""
> +    if not len(refname) == 40:

== 40 in the comments?

BTW, do you think that supporting short revision names will be useful?

@@ +124,5 @@
> +def set_share(repo, share):
> +    alternates = os.path.join(get_git_dir(repo), 'objects', 'info', 'alternates')
> +    share_objects = os.path.join(get_git_dir(share), 'objects')
> +
> +    open(alternates, 'w').write("%s\n" % share_objects)

can you explicitly close the file? or use "with".

@@ +176,5 @@
> +    # Clean up locks
> +    lock_file = os.path.join(get_git_dir(dest), "index.lock")
> +    if os.path.exists(lock_file):
> +        log.info("removing %s", lock_file)
> +        safe_unlink(lock_file)

does this mean that you may have a race condition with another process?

@@ +215,5 @@
> +                # Something went wrong!
> +                # Clobber share_dir and re-raise
> +                log.info("error fetching into %s - clobbering", share_dir)
> +                remove_path(share_dir)
> +                raise

does this mean that you remove the shared directory on any error, even on temporary ones, like 500, network issues, etc? Probably it's OK because we retry using different mirrors...

@@ +227,5 @@
> +                log.info("clobbering %s", share_dir)
> +                remove_path(share_dir)
> +                log.info("error fetching into %s - clobbering", dest)
> +                remove_path(dest)
> +                raise

the same here
Attached patch gittool changesSplinter Review
removes unused is_commitid function
adds tests for get_repo_name
uses "with" to write to alternates file
remove file lock cleanup
Attachment #737632 - Attachment is obsolete: true
Attachment #737632 - Flags: review?(rail)
Attachment #739183 - Flags: review?(rail)
Attachment #739183 - Flags: review?(rail) → review+
Attachment #739183 - Flags: checked-in+
Just pushed http://hg.mozilla.org/build/mozharness/rev/466b3b086fab to default. Let's see how it fares on cedar.
Merged to mozharness production.
Attachment #740776 - Flags: review?(rail)
Comment on attachment 740776 [details] [diff] [review]
switch panda/unagi to use manifests

Whooohoo!
Attachment #740776 - Flags: review?(rail) → review+
https://hg.mozilla.org/mozilla-central/rev/08a750afe46f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Pushed http://hg.mozilla.org/build/mozharness/rev/64b21ad5e9ca to address nightly build bustage
In mozharness production.
Attachment #742386 - Flags: review?(rail)
Comment on attachment 742386 [details] [diff] [review]
switch mozilla-b2g18 to use manifests

Woot!
Attachment #742386 - Flags: review?(rail) → review+
Attachment #742386 - Flags: checked-in+
Attachment #742701 - Flags: review?(rail) → review+
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.