From e323aa888d52741e8589c0f0321a4a2f81e49236 Mon Sep 17 00:00:00 2001 From: XavierDo Date: Fri, 8 Mar 2019 10:48:04 +0100 Subject: [PATCH] [IMP] runbot: add dependencies to build Before this commit, dependencies (i.e. community commit to use when testing enterprise) were computed at checkout, when the build was going from pending to testing state and were not stored. Since the duplicate detection was done at create, the get_closest_branch_name was called in a loop for each posible duplicate candidate, then a last time at checkout. The main idea of this pr is to store the build dependecies on build at create, making the duplicate detection faster (especially when the build name is matching many indirect builds). The side effect of this change is that the build dependencies won't be affected if a new commit is pushed between the build creation and the checkout. The build is fully determined at creation. get_closest_branch is only called once per build The duplicate detection will also be more precise since we are matching on the commits groups that were used to run the build, and not only the branch name. Some work has also been done to rework the closest branch detection in order to manage new corner cases. Hopefully, everything should work as before (or in a better way). In a soon future, it will also be possible to use this information to make an "exact rebuild" or to find corresponding community build. Pr: #117 --- runbot/__manifest__.py | 2 +- runbot/models/__init__.py | 2 +- runbot/models/branch.py | 156 +++++++++------- runbot/models/build.py | 269 ++++++++++++++++------------ runbot/models/build_dependency.py | 10 ++ runbot/models/repo.py | 20 ++- runbot/security/ir.model.access.csv | 2 + runbot/templates/build.xml | 4 +- runbot/tests/test_build.py | 265 ++++++++++++++++++--------- runbot/tests/test_frontend.py | 3 +- 10 files changed, 458 insertions(+), 275 deletions(-) create mode 100644 runbot/models/build_dependency.py diff --git a/runbot/__manifest__.py b/runbot/__manifest__.py index 17562504..b27a51ea 100644 --- a/runbot/__manifest__.py +++ b/runbot/__manifest__.py @@ -6,7 +6,7 @@ 'author': "Odoo SA", 'website': "http://runbot.odoo.com", 'category': 'Website', - 'version': '3.0', + 'version': '3.1', 'depends': ['website', 'base'], 'data': [ 'security/runbot_security.xml', diff --git a/runbot/models/__init__.py b/runbot/models/__init__.py index f334bd9b..6e299960 100644 --- a/runbot/models/__init__.py +++ b/runbot/models/__init__.py @@ -1,4 +1,4 @@ # -*- coding: utf-8 -*- -from . import repo, branch, build, event +from . import repo, branch, build, event, build_dependency from . import res_config_settings \ No newline at end of file diff --git a/runbot/models/branch.py b/runbot/models/branch.py index 9686c000..eb94b5d8 100644 --- a/runbot/models/branch.py +++ b/runbot/models/branch.py @@ -101,17 +101,15 @@ class runbot_branch(models.Model): last_build = branch._get_last_coverage_build() branch.coverage_result = last_build.coverage_result or 0.0 - def _get_closest_branch_name(self, target_repo_id): - """Return (repo, branch name) of the closest common branch between build's branch and - any branch of target_repo or its duplicated repos. - to prevent the above rules to mistakenly link PR of different repos together. + def _get_closest_branch(self, target_repo_id): + """ + Return branch id of the closest branch based on name or pr informations. """ self.ensure_one() Branch = self.env['runbot.branch'] - branch, repo = self, self.repo_id - name = branch.pull_head_name or branch.branch_name - target_branch = branch.target_branch_name or 'master' + repo = self.repo_id + name = self.pull_head_name or self.branch_name target_repo = self.env['runbot.repo'].browse(target_repo_id) @@ -125,72 +123,106 @@ class runbot_branch(models.Model): _logger.debug('Search closest of %s (%s) in repos %r', name, repo.name, target_repo_ids) - sort_by_repo = lambda d: (not d['sticky'], # sticky first - target_repo_ids.index(d['repo_id'][0]), - -1 * len(d.get('branch_name', '')), - -1 * d['id']) - result_for = lambda d, match='exact': (d['repo_id'][0], d['name'], match) - fields = ['name', 'repo_id', 'sticky'] + def sort_by_repo(branch): + return ( + not branch.sticky, # sticky first + target_repo_ids.index(branch.repo_id[0].id), + -1 * len(branch.branch_name), # little change of logic here, was only sorted on branch_name in prefix matching case before + -1 * branch.id + ) # 1. same name, not a PR - domain = [ - ('repo_id', 'in', target_repo_ids), - ('branch_name', '=', name), - ('name', '=like', 'refs/heads/%'), - ] - targets = Branch.search_read(domain, fields, order='id DESC') - targets = sorted(targets, key=sort_by_repo) - if targets and self._branch_exists(targets[0]['id']): - return result_for(targets[0]) + if not self.pull_head_name: # not a pr + domain = [ + ('repo_id', 'in', target_repo_ids), + ('branch_name', '=', self.branch_name), + ('name', '=like', 'refs/heads/%'), + ] + targets = Branch.search(domain, order='id DESC') + targets = sorted(targets, key=sort_by_repo) + if targets and targets[0]._is_on_remote(): + return (targets[0], 'exact') # 2. PR with head name equals - domain = [ - ('repo_id', 'in', target_repo_ids), - ('pull_head_name', '=', name), - ('name', '=like', 'refs/pull/%'), - ] - pulls = Branch.search_read(domain, fields, order='id DESC') - pulls = sorted(pulls, key=sort_by_repo) - for pull in Branch.browse([pu['id'] for pu in pulls]): - pi = pull._get_pull_info() - if pi.get('state') == 'open': - if ':' in name: # we assume that branch exists if we got pull info - pr_branch_name = name.split(':')[1] - return (pull.repo_id.duplicate_id.id, 'refs/heads/%s' % pr_branch_name, 'exact PR') - else: - return (pull.repo_id.id, pull.name, 'exact PR') - - # 3. Match a branch which is the dashed-prefix of current branch name - branches = Branch.search_read( - [('repo_id', 'in', target_repo_ids), ('name', '=like', 'refs/heads/%')], - fields + ['branch_name'], order='id DESC', - ) - branches = sorted(branches, key=sort_by_repo) - - for branch in branches: - if name.startswith(branch['branch_name'] + '-') and self._branch_exists(branch['id']): - return result_for(branch, 'prefix') + if self.pull_head_name: + domain = [ + ('repo_id', 'in', target_repo_ids), + ('pull_head_name', '=', self.pull_head_name), + ('name', '=like', 'refs/pull/%'), + ] + pulls = Branch.search(domain, order='id DESC') + pulls = sorted(pulls, key=sort_by_repo) + for pull in Branch.browse([pu['id'] for pu in pulls]): + pi = pull._get_pull_info() + if pi.get('state') == 'open': + if ':' in self.pull_head_name: + (repo_name, pr_branch_name) = self.pull_head_name.split(':') + repo = self.env['runbot.repo'].browse(target_repo_ids).filtered(lambda r: ':%s/' % repo_name in r.name) + # most of the time repo will be pull.repo_id.duplicate_id, but it is still possible to have a pr pointing the same repo + if repo: + pr_branch_ref = 'refs/heads/%s' % pr_branch_name + pr_branch = self._get_or_create_branch(repo.id, pr_branch_ref) + # use _get_or_create_branch in case a pr is scanned before pull_head_name branch. + return (pr_branch, 'exact PR') + return (pull, 'exact PR') # 4.Match a PR in enterprise without community PR - if self.name.startswith('refs/pull') and ':' in name: - pr_branch_name = name.split(':')[1] - duplicate_branch_name = 'refs/heads/%s' % pr_branch_name - domain = [ - ('repo_id', 'in', target_repo_ids), # target_repo_ids should contain the target duplicate repo - ('branch_name', '=', pr_branch_name), - ('pull_head_name', '=', False), - ] - targets = Branch.search_read(domain, fields, order='id DESC') - targets = sorted(targets, key=sort_by_repo) - if targets and self._branch_exists(targets[0]['id']): - return result_for(targets[0], 'no PR') + # Moved before 3 because it makes more sense + if self.pull_head_name: + if self.name.startswith('refs/pull'): + if ':' in self.pull_head_name: + (repo_name, pr_branch_name) = self.pull_head_name.split(':') + repos = self.env['runbot.repo'].browse(target_repo_ids).filtered(lambda r: ':%s/' % repo_name in r.name) + else: + pr_branch_name = self.pull_head_name + repos = target_repo + if repos: + duplicate_branch_name = 'refs/heads/%s' % pr_branch_name + domain = [ + ('repo_id', 'in', tuple(repos.ids)), + ('branch_name', '=', pr_branch_name), + ('pull_head_name', '=', False), + ] + targets = Branch.search(domain, order='id DESC') + targets = sorted(targets, key=sort_by_repo) + if targets and targets[0]._is_on_remote(): + return (targets[0], 'no PR') + + # 3. Match a branch which is the dashed-prefix of current branch name + if not self.pull_head_name: + if '-' in self.branch_name: + name_start = 'refs/heads/%s' % self.branch_name.split('-')[0] + domain = [('repo_id', 'in', target_repo_ids), ('name', '=like', '%s%%' % name_start)] + branches = Branch.search(domain, order='id DESC') + branches = sorted(branches, key=sort_by_repo) + for branch in branches: + if self.branch_name.startswith('%s-' % branch.branch_name) and branch._is_on_remote(): + return (branch, 'prefix') # 5. last-resort value - return target_repo_id, 'refs/heads/%s' % target_branch, 'default' + if self.target_branch_name: + default_target_ref = 'refs/heads/%s' % self.target_branch_name + default_branch = self.search([('repo_id', 'in', target_repo_ids), ('name', '=', default_target_ref)], limit=1) + if default_branch: + return (default_branch, 'pr_target') + + default_target_ref = 'refs/heads/master' + default_branch = self.search([('repo_id', 'in', target_repo_ids), ('name', '=', default_target_ref)], limit=1) + # we assume that master will always exists + return (default_branch, 'default') def _branch_exists(self, branch_id): Branch = self.env['runbot.branch'] branch = Branch.search([('id', '=', branch_id)]) if branch and branch[0]._is_on_remote(): return True - return False \ No newline at end of file + return False + + def _get_or_create_branch(self, repo_id, name): + res = self.search([('repo_id', '=', repo_id), ('name', '=', name)], limit=1) + if res: + return res + _logger.warning('creating missing branch %s', name) + Branch = self.env['runbot.branch'] + branch = Branch.create({'repo_id': repo_id, 'name': name}) + return branch diff --git a/runbot/models/build.py b/runbot/models/build.py index aa18b340..c54ed811 100644 --- a/runbot/models/build.py +++ b/runbot/models/build.py @@ -69,11 +69,7 @@ class runbot_build(models.Model): job_age = fields.Integer(compute='_get_age', string='Job age') duplicate_id = fields.Many2one('runbot.build', 'Corresponding Build') server_match = fields.Selection([('builtin', 'This branch includes Odoo server'), - ('exact', 'branch/PR exact name'), - ('prefix', 'branch whose name is a prefix of current one'), - ('fuzzy', 'Fuzzy - common ancestor found'), - ('exact PR', 'Exact match between two PR'), - ('no PR', 'PR matching a branch without PR'), + ('match', 'This branch includes Odoo server'), ('default', 'No match found - defaults to master')], string='Server branch matching') revdep_build_ids = fields.Many2many('runbot.build', 'runbot_rev_dep_builds', @@ -94,8 +90,8 @@ class runbot_build(models.Model): ('running', 'Running job only'), ('all', 'All jobs'), ('none', 'Do not execute jobs'), - ], - ) + ]) + dependency_ids = fields.One2many('runbot.build.dependency', 'build_id') def copy(self, values=None): raise UserError("Cannot duplicate build!") @@ -104,33 +100,75 @@ class runbot_build(models.Model): branch = self.env['runbot.branch'].search([('id', '=', vals.get('branch_id', False))]) if branch.job_type == 'none' or vals.get('job_type', '') == 'none': return self.env['runbot.build'] + vals['job_type'] = vals['job_type'] if 'job_type' in vals else branch.job_type build_id = super(runbot_build, self).create(vals) extra_info = {'sequence': build_id.id if not build_id.sequence else build_id.sequence} - job_type = vals['job_type'] if 'job_type' in vals else build_id.branch_id.job_type - extra_info.update({'job_type': job_type}) context = self.env.context - if not context.get('force_rebuild'): + # compute dependencies + repo = build_id.repo_id + dep_create_vals = [] + nb_deps = len(repo.dependency_ids) + for extra_repo in repo.dependency_ids: + (build_closets_branch, match_type) = build_id.branch_id._get_closest_branch(extra_repo.id) + closest_name = build_closets_branch.name + closest_branch_repo = build_closets_branch.repo_id + last_commit = closest_branch_repo._git_rev_parse(closest_name) + dep_create_vals.append({ + 'build_id': build_id.id, + 'dependecy_repo_id': extra_repo.id, + 'closest_branch_id': build_closets_branch.id, + 'dependency_hash': last_commit, + 'match_type': match_type, + }) + + for dep_vals in dep_create_vals: + self.env['runbot.build.dependency'].sudo().create(dep_vals) + + if not context.get('force_rebuild'): # not vals.get('build_type') == rebuild': could be enough, but some cron on runbot are using this ctx key, to do later # detect duplicate duplicate_id = None domain = [ - ('repo_id', '=', build_id.repo_id.duplicate_id.id), + ('repo_id', 'in', (build_id.repo_id.duplicate_id.id, build_id.repo_id.id)), # before, was only looking in repo.duplicate_id looks a little better to search in both + ('id', '!=', build_id.id), ('name', '=', build_id.name), ('duplicate_id', '=', False), - '|', ('result', '=', False), ('result', '!=', 'skipped') + # ('build_type', '!=', 'indirect'), # in case of performance issue, this little fix may improve performance a little but less duplicate will be detected when pushing an empty branch on repo with duplicates + ('result', '!=', 'skipped'), + ('job_type', '=', build_id.job_type), ] + candidates = self.search(domain) + if candidates and nb_deps: + # check that all depedencies are matching. - for duplicate in self.search(domain, limit=10): - duplicate_id = duplicate.id - # Consider the duplicate if its closest branches are the same than the current build closest branches. - for extra_repo in build_id.repo_id.dependency_ids: - build_closest_name = build_id._get_closest_branch_name(extra_repo.id)[1] - duplicate_closest_name = duplicate._get_closest_branch_name(extra_repo.id)[1] - if build_closest_name != duplicate_closest_name: - duplicate_id = None - if duplicate_id: - extra_info.update({'state': 'duplicate', 'duplicate_id': duplicate_id}) - break + # Note: We avoid to compare closest_branch_id, because the same hash could be found on + # 2 different branches (pr + branch). + # But we may want to ensure that the hash is comming from the right repo, we dont want to compare community + # hash with enterprise hash. + # this is unlikely to happen so branch comparaison is disabled + self.env.cr.execute(""" + SELECT DUPLIDEPS.build_id + FROM runbot_build_dependency as DUPLIDEPS + JOIN runbot_build_dependency as BUILDDEPS + ON BUILDDEPS.dependency_hash = DUPLIDEPS.dependency_hash + --AND BUILDDEPS.closest_branch_id = DUPLIDEPS.closest_branch_id -- only usefull if we are affraid of hash collision in different branches + AND BUILDDEPS.build_id = %s + AND DUPLIDEPS.build_id in %s + GROUP BY DUPLIDEPS.build_id + HAVING COUNT(DUPLIDEPS.*) = %s + ORDER BY DUPLIDEPS.build_id -- remove this in case of performance issue, not so usefull + LIMIT 1 + """, (build_id.id, tuple(candidates.ids), nb_deps)) + filtered_candidates_ids = self.env.cr.fetchall() + + if filtered_candidates_ids: + duplicate_id = filtered_candidates_ids[0] + else: + duplicate_id = candidates[0].id if candidates else False + + if duplicate_id: + extra_info.update({'state': 'duplicate', 'duplicate_id': duplicate_id}) + # maybe update duplicate priority if needed build_id.write(extra_info) if build_id.state == 'duplicate' and build_id.duplicate_id.state in ('running', 'done'): @@ -140,21 +178,6 @@ class runbot_build(models.Model): def _reset(self): self.write({'state': 'pending'}) - def _get_closest_branch_name(self, target_repo_id): - """Return (repo, branch name) of the closest common branch between build's branch and - any branch of target_repo or its duplicated repos. - - Rules priority for choosing the branch from the other repo is: - 1. Same branch name - 2. A PR whose head name match - 3. Match a branch which is the dashed-prefix of current branch name - 4. Common ancestors (git merge-base) - Note that PR numbers are replaced by the branch name of the PR target - to prevent the above rules to mistakenly link PR of different repos together. - """ - self.ensure_one() - return self.branch_id._get_closest_branch_name(target_repo_id) - @api.depends('name', 'branch_id.name') def _get_dest(self): for build in self: @@ -436,96 +459,108 @@ class runbot_build(models.Model): return uniq_list(filter(mod_filter, modules)) def _checkout(self): - for build in self: - # starts from scratch - if os.path.isdir(build._path()): - shutil.rmtree(build._path()) + self.ensure_one() # will raise exception if hash not found, we don't want to fail for all build. + # starts from scratch + build = self + if os.path.isdir(build._path()): + shutil.rmtree(build._path()) - # runbot log path - os.makedirs(build._path("logs"), exist_ok=True) - os.makedirs(build._server('addons'), exist_ok=True) + # runbot log path + os.makedirs(build._path("logs"), exist_ok=True) + os.makedirs(build._server('addons'), exist_ok=True) - # update repo if needed - if not build.repo_id._hash_exists(build.name): - build.repo_id._update(build.repo_id) + # update repo if needed + if not build.repo_id._hash_exists(build.name): + build.repo_id._update(build.repo_id) - # checkout branch - build.branch_id.repo_id._git_export(build.name, build._path()) + # checkout branch + build.branch_id.repo_id._git_export(build.name, build._path()) - has_server = os.path.isfile(build._server('__init__.py')) - server_match = 'builtin' + has_server = os.path.isfile(build._server('__init__.py')) + server_match = 'builtin' - # build complete set of modules to install - modules_to_move = [] - modules_to_test = ((build.branch_id.modules or '') + ',' + - (build.repo_id.modules or '')) - modules_to_test = list(filter(None, modules_to_test.split(','))) # ??? - explicit_modules = set(modules_to_test) - _logger.debug("manual modules_to_test for build %s: %s", build.dest, modules_to_test) + # build complete set of modules to install + modules_to_move = [] + modules_to_test = ((build.branch_id.modules or '') + ',' + + (build.repo_id.modules or '')) + modules_to_test = list(filter(None, modules_to_test.split(','))) # ??? + explicit_modules = set(modules_to_test) + _logger.debug("manual modules_to_test for build %s: %s", build.dest, modules_to_test) - if not has_server: - if build.repo_id.modules_auto == 'repo': - modules_to_test += [ - os.path.basename(os.path.dirname(a)) - for a in (glob.glob(build._path('*/__openerp__.py')) + - glob.glob(build._path('*/__manifest__.py'))) - ] - _logger.debug("local modules_to_test for build %s: %s", build.dest, modules_to_test) - - for extra_repo in build.repo_id.dependency_ids: - repo_id, closest_name, server_match = build._get_closest_branch_name(extra_repo.id) - repo = self.env['runbot.repo'].browse(repo_id) - _logger.debug('branch %s of %s: %s match branch %s of %s', - build.branch_id.name, build.repo_id.name, - server_match, closest_name, repo.name) - build._log( - 'Building environment', - '%s match branch %s of %s' % (server_match, closest_name, repo.name) - ) - repo._update_git(force=True) - latest_commit = repo._git(['rev-parse', closest_name]).strip() - commit_oneline = repo._git(['show', '--pretty="%H -- %s"', '-s', latest_commit]).strip() - build._log( - 'Building environment', - 'Server built based on commit %s from %s' % (commit_oneline, closest_name) - ) - repo._git_export(closest_name, build._path()) - - # Finally mark all addons to move to openerp/addons - modules_to_move += [ - os.path.dirname(module) - for module in (glob.glob(build._path('*/__openerp__.py')) + - glob.glob(build._path('*/__manifest__.py'))) + if not has_server: + if build.repo_id.modules_auto == 'repo': + modules_to_test += [ + os.path.basename(os.path.dirname(a)) + for a in (glob.glob(build._path('*/__openerp__.py')) + + glob.glob(build._path('*/__manifest__.py'))) ] + _logger.debug("local modules_to_test for build %s: %s", build.dest, modules_to_test) - # move all addons to server addons path - for module in uniq_list(glob.glob(build._path('addons/*')) + modules_to_move): - basename = os.path.basename(module) - addon_path = build._server('addons', basename) - if os.path.exists(addon_path): - build._log( - 'Building environment', - 'You have duplicate modules in your branches "%s"' % basename - ) - if os.path.islink(addon_path) or os.path.isfile(addon_path): - os.remove(addon_path) - else: - shutil.rmtree(addon_path) - shutil.move(module, build._server('addons')) + # todo make it backward compatible, or create migration script? + for build_dependency in build.dependency_ids: + closest_branch = build_dependency.closest_branch_id + latest_commit = build_dependency.dependency_hash + repo = closest_branch.repo_id + closest_name = closest_branch.name + if build_dependency.match_type == 'default': + server_match = 'default' + elif server_match != 'default': + server_match = 'match' - available_modules = [ - os.path.basename(os.path.dirname(a)) - for a in (glob.glob(build._server('addons/*/__openerp__.py')) + - glob.glob(build._server('addons/*/__manifest__.py'))) + build._log( + 'Building environment', + '%s match branch %s of %s' % (build_dependency.match_type, closest_name, repo.name) + ) + if not repo._hash_exists(latest_commit): + repo._update(force=True) + if not repo._hash_exists(latest_commit): + repo._git(['fetch', 'origin', latest_commit]) + if not repo._hash_exists(latest_commit): + build._log('_checkout',"Dependency commit %s in repo %s is unreachable" % (latest_commit, repo.name)) + raise Exception + + commit_oneline = repo._git(['show', '--pretty="%H -- %s"', '-s', latest_commit]).strip() + build._log( + 'Building environment', + 'Server built based on commit %s from %s' % (commit_oneline, closest_name) + ) + repo._git_export(latest_commit, build._path()) + + # Finally mark all addons to move to openerp/addons + modules_to_move += [ + os.path.dirname(module) + for module in (glob.glob(build._path('*/__openerp__.py')) + + glob.glob(build._path('*/__manifest__.py'))) ] - if build.repo_id.modules_auto == 'all' or (build.repo_id.modules_auto != 'none' and has_server): - modules_to_test += available_modules - modules_to_test = self._filter_modules(modules_to_test, - set(available_modules), explicit_modules) - _logger.debug("modules_to_test for build %s: %s", build.dest, modules_to_test) - build.write({'server_match': server_match, - 'modules': ','.join(modules_to_test)}) + # move all addons to server addons path + for module in uniq_list(glob.glob(build._path('addons/*')) + modules_to_move): + basename = os.path.basename(module) + addon_path = build._server('addons', basename) + if os.path.exists(addon_path): + build._log( + 'Building environment', + 'You have duplicate modules in your branches "%s"' % basename + ) + if os.path.islink(addon_path) or os.path.isfile(addon_path): + os.remove(addon_path) + else: + shutil.rmtree(addon_path) + shutil.move(module, build._server('addons')) + + available_modules = [ + os.path.basename(os.path.dirname(a)) + for a in (glob.glob(build._server('addons/*/__openerp__.py')) + + glob.glob(build._server('addons/*/__manifest__.py'))) + ] + if build.repo_id.modules_auto == 'all' or (build.repo_id.modules_auto != 'none' and has_server): + modules_to_test += available_modules + + modules_to_test = self._filter_modules(modules_to_test, + set(available_modules), explicit_modules) + _logger.debug("modules_to_test for build %s: %s", build.dest, modules_to_test) + build.write({'server_match': server_match, + 'modules': ','.join(modules_to_test)}) def _local_pg_dropdb(self, dbname): with local_pgadmin_cursor() as local_cr: diff --git a/runbot/models/build_dependency.py b/runbot/models/build_dependency.py new file mode 100644 index 00000000..179ce2be --- /dev/null +++ b/runbot/models/build_dependency.py @@ -0,0 +1,10 @@ +from odoo import models, fields + +class RunbotBuildDependency(models.Model): + _name = "runbot.build.dependency" + + build_id = fields.Many2one('runbot.build', 'Build', required=True, ondelete='cascade', index=True) + dependecy_repo_id = fields.Many2one('runbot.repo', 'Dependency repo', required=True, ondelete='cascade') + dependency_hash = fields.Char('Name of commit', index=True) + closest_branch_id = fields.Many2one('runbot.branch', 'Branch', required=True, ondelete='cascade') + match_type = fields.Char('Match Type') diff --git a/runbot/models/repo.py b/runbot/models/repo.py index 575f216e..1d7b8a8a 100644 --- a/runbot/models/repo.py +++ b/runbot/models/repo.py @@ -86,6 +86,9 @@ class runbot_repo(models.Model): _logger.info("git command: %s", ' '.join(cmd)) return subprocess.check_output(cmd).decode('utf-8') + def _git_rev_parse(self, branch_name): + return self._git(['rev-parse', branch_name]).strip() + def _git_export(self, treeish, dest): """Export a git repo to dest""" self.ensure_one() @@ -130,7 +133,7 @@ class runbot_repo(models.Model): else: raise - def _find_new_commits(self, repo): + def _find_new_commits(self): """ Find new commits in bare repo """ self.ensure_one() Branch = self.env['runbot.branch'] @@ -140,7 +143,7 @@ class runbot_repo(models.Model): fields = ['refname', 'objectname', 'committerdate:iso8601', 'authorname', 'authoremail', 'subject', 'committername', 'committeremail'] fmt = "%00".join(["%(" + field + ")" for field in fields]) - git_refs = repo._git(['for-each-ref', '--format', fmt, '--sort=-committerdate', 'refs/heads', 'refs/pull']) + git_refs = self._git(['for-each-ref', '--format', fmt, '--sort=-committerdate', 'refs/heads', 'refs/pull']) git_refs = git_refs.strip() refs = [[field for field in line.split('\x00')] for line in git_refs.split('\n')] @@ -150,7 +153,7 @@ class runbot_repo(models.Model): SELECT t.branch, b.id FROM t LEFT JOIN runbot_branch b ON (b.name = t.branch) WHERE b.repo_id = %s; - """, ([r[0] for r in refs], repo.id)) + """, ([r[0] for r in refs], self.id)) ref_branches = {r[0]: r[1] for r in self.env.cr.fetchall()} for name, sha, date, author, author_email, subject, committer, committer_email in refs: @@ -158,8 +161,8 @@ class runbot_repo(models.Model): if ref_branches.get(name): branch_id = ref_branches[name] else: - _logger.debug('repo %s found new branch %s', repo.name, name) - branch_id = Branch.create({'repo_id': repo.id, 'name': name}).id + _logger.debug('repo %s found new branch %s', self.name, name) + branch_id = Branch.create({'repo_id': self.id, 'name': name}).id branch = Branch.browse([branch_id])[0] # skip the build for old branches (Could be checked before creating the branch in DB ?) @@ -208,10 +211,10 @@ class runbot_repo(models.Model): if latest_rev_build: _logger.debug('Reverse dependency build %s forced in repo %s by commit %s', latest_rev_build.dest, rev_repo.name, sha[:6]) latest_rev_build.build_type = 'indirect' - new_build.revdep_build_ids += latest_rev_build._force(message='Rebuild from dependency %s commit %s' % (repo.name, sha[:6])) + new_build.revdep_build_ids += latest_rev_build._force(message='Rebuild from dependency %s commit %s' % (self.name, sha[:6])) # skip old builds (if their sequence number is too low, they will not ever be built) - skippable_domain = [('repo_id', '=', repo.id), ('state', '=', 'pending')] + skippable_domain = [('repo_id', '=', self.id), ('state', '=', 'pending')] icp = self.env['ir.config_parameter'] running_max = int(icp.get_param('runbot.runbot_running_max', default=75)) builds_to_be_skipped = Build.search(skippable_domain, order='sequence desc', offset=running_max) @@ -221,7 +224,7 @@ class runbot_repo(models.Model): """ Find new commits in physical repos""" for repo in repos: try: - repo._find_new_commits(repo) + repo._find_new_commits() except Exception: _logger.exception('Fail to find new commits in repo %s', repo.name) @@ -388,6 +391,7 @@ class runbot_repo(models.Model): repos = self.search([('mode', '!=', 'disabled')]) self._update(repos, force=False) self._create_pending_builds(repos) + self.env.cr.commit() self.invalidate_cache() time.sleep(update_frequency) diff --git a/runbot/security/ir.model.access.csv b/runbot/security/ir.model.access.csv index d6eebd3a..7cfdb245 100644 --- a/runbot/security/ir.model.access.csv +++ b/runbot/security/ir.model.access.csv @@ -2,7 +2,9 @@ id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink access_runbot_repo,runbot_repo,runbot.model_runbot_repo,group_user,1,0,0,0 access_runbot_branch,runbot_branch,runbot.model_runbot_branch,group_user,1,0,0,0 access_runbot_build,runbot_build,runbot.model_runbot_build,group_user,1,0,0,0 +access_runbot_build_dependency,runbot_build_dependency,runbot.model_runbot_build_dependency,group_user,1,0,0,0 access_runbot_repo_admin,runbot_repo_admin,runbot.model_runbot_repo,runbot.group_runbot_admin,1,1,1,1 access_runbot_branch_admin,runbot_branch_admin,runbot.model_runbot_branch,runbot.group_runbot_admin,1,1,1,1 access_runbot_build_admin,runbot_build_admin,runbot.model_runbot_build,runbot.group_runbot_admin,1,1,1,1 +access_runbot_build_dependency_admin,runbot_build_dependency_admin,runbot.model_runbot_build_dependency,runbot.group_runbot_admin,1,1,1,1 access_irlogging,log by runbot users,base.model_ir_logging,group_user,0,0,1,0 diff --git a/runbot/templates/build.xml b/runbot/templates/build.xml index 90894fa6..45b07c60 100644 --- a/runbot/templates/build.xml +++ b/runbot/templates/build.xml @@ -15,9 +15,9 @@ killed manually killed - + + title="Server branch cannot be determined exactly. Please use naming convention '12.0-my-branch' to build with '12.0' server branch."/> Dep builds: diff --git a/runbot/tests/test_build.py b/runbot/tests/test_build.py index 3004c0f7..8b349fa1 100644 --- a/runbot/tests/test_build.py +++ b/runbot/tests/test_build.py @@ -158,6 +158,35 @@ class Test_Build(common.TransactionCase): log_first_part = '%s skip %%s' % (other_build.dest) mock_logger.debug.assert_called_with(log_first_part, 'A good reason') + def test_ask_kill_duplicate(self): + """ Test that the _ask_kill method works on duplicate""" + #mock_is_on_remote.return_value = True + + build1 = self.Build.create({ + 'branch_id': self.branch_10.id, + 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', + }) + build2 = self.Build.create({ + 'branch_id': self.branch_10.id, + 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', + }) + build2.write({'state': 'duplicate', 'duplicate_id': build1.id}) # this may not be usefull if we detect duplicate in same repo. + + self.assertEqual(build1.state, 'pending') + build2._ask_kill() + self.assertEqual(build1.state, 'done', 'A killed pending duplicate build should mark the real build as done') + self.assertEqual(build1.result, 'skipped', 'A killed pending duplicate build should mark the real build as skipped') + + +def rev_parse(repo, branch_name): + """ + simulate a rev parse by returning a fake hash of form + 'rp_odoo-dev/enterprise_saas-12.2__head' + should be overwitten if a pr head should match a branch head + """ + head_hash = 'rp_%s_%s_head' % (repo.name.split(':')[1], branch_name.split('/')[-1]) + return head_hash + class TestClosestBranch(common.TransactionCase): @@ -165,16 +194,15 @@ class TestClosestBranch(common.TransactionCase): branch_type = 'pull' if 'pull' in branch.name else 'branch' return '%s %s:%s' % (branch_type, branch.repo_id.name.split(':')[-1], branch.name.split('/')[-1]) - def assertClosest(self, build, closest): - extra_repo = build.repo_id.dependency_ids[0] - self.assertEqual(closest, build._get_closest_branch_name(extra_repo.id), "build on %s didn't had the extected closest branch" % self.branch_description(build.branch_id)) + def assertClosest(self, branch, closest): + extra_repo = branch.repo_id.dependency_ids[0] + self.assertEqual(closest, branch._get_closest_branch(extra_repo.id), "build on %s didn't had the extected closest branch" % self.branch_description(branch)) - def assertDuplicate(self, branch1, branch2, b1_closest=None, b2_closest=None): + def assertDuplicate(self, branch1, branch2, b1_closest=None, b2_closest=None, noDuplicate=False): """ Test that the creation of a build on branch1 and branch2 detects duplicate, no matter the order. Also test that build on branch1 closest_branch_name result is b1_closest if given Also test that build on branch2 closest_branch_name result is b2_closest if given - Test that the _ask_kill method works on duplicate """ closest = { branch1: b1_closest, @@ -188,7 +216,7 @@ class TestClosestBranch(common.TransactionCase): }) if b1_closest: - self.assertClosest(build1, closest[b1]) + self.assertClosest(b1, closest[b1]) build2 = self.Build.create({ 'branch_id': b2.id, @@ -196,15 +224,16 @@ class TestClosestBranch(common.TransactionCase): }) if b2_closest: - self.assertClosest(build2, closest[b2]) + self.assertClosest(b2, closest[b2]) + if noDuplicate: + self.assertNotEqual(build2.state, 'duplicate') + self.assertFalse(build2.duplicate_id, "build on %s was detected as duplicate of build %s" % (self.branch_description(b2), build2.duplicate_id)) + else: + self.assertEqual(build2.duplicate_id.id, build1.id, "build on %s wasn't detected as duplicate of build on %s" % (self.branch_description(b2), self.branch_description(b1))) + self.assertEqual(build2.state, 'duplicate') - self.assertEqual(build2.duplicate_id.id, build1.id, "build on %s wasn't detected as duplicate of build on %s" % (self.branch_description(b2), self.branch_description(b1))) - self.assertEqual(build2.state, 'duplicate') - - self.assertEqual(build1.state, 'pending') - build2._ask_kill() - self.assertEqual(build1.state, 'done', 'A killed pending duplicate build should mark the real build as done') - self.assertEqual(build1.result, 'skipped', 'A killed pending duplicate build should mark the real build as skipped') + def assertNoDuplicate(self, branch1, branch2, b1_closest=None, b2_closest=None): + self.assertDuplicate(branch1, branch2, b1_closest=b1_closest, b2_closest=b2_closest, noDuplicate=True) def setUp(self): """ Setup repositories that mimick the Odoo repos """ @@ -229,28 +258,34 @@ class TestClosestBranch(common.TransactionCase): self.Branch = self.env['runbot.branch'] self.branch_odoo_master = self.Branch.create({ 'repo_id': self.community_repo.id, - 'name': 'refs/heads/master' + 'name': 'refs/heads/master', + 'sticky': True, }) self.branch_odoo_10 = self.Branch.create({ 'repo_id': self.community_repo.id, - 'name': 'refs/heads/10.0' + 'name': 'refs/heads/10.0', + 'sticky': True, }) self.branch_odoo_11 = self.Branch.create({ 'repo_id': self.community_repo.id, - 'name': 'refs/heads/11.0' + 'name': 'refs/heads/11.0', + 'sticky': True, }) self.branch_enterprise_master = self.Branch.create({ 'repo_id': self.enterprise_repo.id, - 'name': 'refs/heads/master' + 'name': 'refs/heads/master', + 'sticky': True, }) self.branch_enterprise_10 = self.Branch.create({ 'repo_id': self.enterprise_repo.id, - 'name': 'refs/heads/10.0' + 'name': 'refs/heads/10.0', + 'sticky': True, }) self.branch_enterprise_11 = self.Branch.create({ 'repo_id': self.enterprise_repo.id, - 'name': 'refs/heads/11.0' + 'name': 'refs/heads/11.0', + 'sticky': True, }) self.Build = self.env['runbot.build'] @@ -279,6 +314,7 @@ class TestClosestBranch(common.TransactionCase): def test_closest_branch_01(self, mock_is_on_remote): """ test find a matching branch in a target repo based on branch name """ mock_is_on_remote.return_value = True + self.Branch.create({ 'repo_id': self.community_dev_repo.id, 'name': 'refs/heads/10.0-fix-thing-moc' @@ -287,14 +323,12 @@ class TestClosestBranch(common.TransactionCase): 'repo_id': self.enterprise_dev_repo.id, 'name': 'refs/heads/10.0-fix-thing-moc' }) - addons_build = self.Build.create({ - 'branch_id': addons_branch.id, - 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', - }) - self.assertEqual((self.enterprise_dev_repo.id, addons_branch.name, 'exact'), addons_build._get_closest_branch_name(self.enterprise_dev_repo.id)) + + self.assertEqual((addons_branch, 'exact'), addons_branch._get_closest_branch(self.enterprise_dev_repo.id)) @patch('odoo.addons.runbot.models.repo.runbot_repo._github') def test_closest_branch_02(self, mock_github): + """ test find two matching PR having the same head name """ mock_github.return_value = { # "head label" is the repo:branch where the PR comes from @@ -322,21 +356,17 @@ class TestClosestBranch(common.TransactionCase): 'repo_id': self.enterprise_repo.id, 'name': 'refs/pull/789101' }) - enterprise_build = self.Build.create({ - 'branch_id': enterprise_pr.id, - 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', - }) - - self.assertEqual((self.community_dev_repo.id, 'refs/heads/bar_branch', 'exact PR'), enterprise_build._get_closest_branch_name(self.community_repo.id)) + self.assertEqual((community_branch, 'exact PR'), enterprise_pr._get_closest_branch(self.community_repo.id)) @patch('odoo.addons.runbot.models.repo.runbot_repo._github') - @patch('odoo.addons.runbot.models.branch.runbot_branch._branch_exists') - def test_closest_branch_02_improved(self, mock_branch_exists, mock_github): + @patch('odoo.addons.runbot.models.branch.runbot_branch._is_on_remote') + def test_closest_branch_02_improved(self, mock_is_on_remote, mock_github): """ test that a PR in enterprise with a matching PR in Community uses the matching one""" - mock_branch_exists.return_value = True - self.Branch.create({ + mock_is_on_remote.return_value = True + + com_dev_branch = self.Branch.create({ 'repo_id': self.community_dev_repo.id, 'name': 'refs/heads/saas-12.2-blabla' }) @@ -375,34 +405,30 @@ class TestClosestBranch(common.TransactionCase): 'repo_id': self.community_repo.id, 'name': 'refs/pull/32156' }) + with patch('odoo.addons.runbot.models.repo.runbot_repo._git_rev_parse', new=rev_parse): + self.assertDuplicate( + ent_dev_branch, + ent_pr, + (com_dev_branch, 'exact'), + (com_dev_branch, 'exact PR') + ) - self.assertDuplicate( - ent_dev_branch, - ent_pr, - (self.community_dev_repo.id, 'refs/heads/saas-12.2-blabla', 'exact'), - (self.community_dev_repo.id, 'refs/heads/saas-12.2-blabla', 'exact PR') - ) - - @patch('odoo.addons.runbot.models.branch.runbot_branch._branch_exists') - def test_closest_branch_03(self, mock_branch_exists): + @patch('odoo.addons.runbot.models.branch.runbot_branch._is_on_remote') + def test_closest_branch_03(self, mock_is_on_remote): """ test find a branch based on dashed prefix""" - mock_branch_exists.return_value = True + mock_is_on_remote.return_value = True addons_branch = self.Branch.create({ 'repo_id': self.enterprise_dev_repo.id, 'name': 'refs/heads/10.0-fix-blah-blah-moc' }) - addons_build = self.Build.create({ - 'branch_id': addons_branch.id, - 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', - }) - self.assertEqual((self.community_repo.id, 'refs/heads/10.0', 'prefix'), addons_build._get_closest_branch_name(self.community_repo.id)) + self.assertEqual((self.branch_odoo_10, 'prefix'), addons_branch._get_closest_branch(self.community_repo.id)) @patch('odoo.addons.runbot.models.repo.runbot_repo._github') - @patch('odoo.addons.runbot.models.branch.runbot_branch._branch_exists') - def test_closest_branch_03_05(self, mock_branch_exists, mock_github): + @patch('odoo.addons.runbot.models.branch.runbot_branch._is_on_remote') + def test_closest_branch_03_05(self, mock_is_on_remote, mock_github): """ test that a PR in enterprise without a matching PR in Community and no branch in community""" - mock_branch_exists.return_value = True + mock_is_on_remote.return_value = True # comm_repo = self.repo # self.repo.write({'token': 1}) @@ -429,7 +455,7 @@ class TestClosestBranch(common.TransactionCase): mock_github.side_effect = github_side_effect - self.Branch.create({ + com_branch = self.Branch.create({ 'repo_id': self.community_repo.id, 'name': 'refs/heads/saas-12.2' }) @@ -438,22 +464,22 @@ class TestClosestBranch(common.TransactionCase): 'repo_id': self.enterprise_repo.id, 'name': 'refs/pull/3721' }) - - self.assertDuplicate( - ent_pr, - ent_dev_branch, - (self.community_repo.id, 'refs/heads/saas-12.2', 'default'), - (self.community_repo.id, 'refs/heads/saas-12.2', 'prefix'), - ) + with patch('odoo.addons.runbot.models.repo.runbot_repo._git_rev_parse', new=rev_parse): + self.assertDuplicate( + ent_pr, + ent_dev_branch, + (com_branch, 'pr_target'), + (com_branch, 'prefix'), + ) @patch('odoo.addons.runbot.models.repo.runbot_repo._github') - @patch('odoo.addons.runbot.models.branch.runbot_branch._branch_exists') - def test_closest_branch_04(self, mock_branch_exists, mock_github): + @patch('odoo.addons.runbot.models.branch.runbot_branch._is_on_remote') + def test_closest_branch_04(self, mock_is_on_remote, mock_github): """ test that a PR in enterprise without a matching PR in Community uses the corresponding exact branch in community""" - mock_branch_exists.return_value = True + mock_is_on_remote.return_value = True - self.Branch.create({ + com_dev_branch = self.Branch.create({ 'repo_id': self.community_dev_repo.id, 'name': 'refs/heads/saas-12.2-blabla' }) @@ -465,7 +491,7 @@ class TestClosestBranch(common.TransactionCase): def github_side_effect(*args, **kwargs): return { - 'head': {'label': 'ent-dev:saas-12.2-blabla'}, + 'head': {'label': 'odoo-dev:saas-12.2-blabla'}, 'base': {'ref': 'saas-12.2'}, 'state': 'open' } @@ -476,13 +502,13 @@ class TestClosestBranch(common.TransactionCase): 'repo_id': self.enterprise_repo.id, 'name': 'refs/pull/3721' }) - - self.assertDuplicate( - ent_dev_branch, - ent_pr, - (self.community_dev_repo.id, 'refs/heads/saas-12.2-blabla', 'exact'), - (self.community_dev_repo.id, 'refs/heads/saas-12.2-blabla', 'no PR') - ) + with patch('odoo.addons.runbot.models.repo.runbot_repo._git_rev_parse', new=rev_parse): + self.assertDuplicate( + ent_dev_branch, + ent_pr, + (com_dev_branch, 'exact'), + (com_dev_branch, 'no PR') + ) @patch('odoo.addons.runbot.models.repo.runbot_repo._github') def test_closest_branch_05(self, mock_github): @@ -506,11 +532,7 @@ class TestClosestBranch(common.TransactionCase): 'repo_id': self.enterprise_repo.id, 'name': 'refs/pull/789101' }) - addons_build = self.Build.create({ - 'branch_id': addons_pr.id, - 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', - }) - self.assertEqual((self.community_repo.id, 'refs/heads/%s' % server_pr.target_branch_name, 'default'), addons_build._get_closest_branch_name(self.community_repo.id)) + self.assertEqual((self.branch_odoo_10, 'pr_target'), addons_pr._get_closest_branch(self.community_repo.id)) def test_closest_branch_05_master(self): """ test last resort value when nothing common can be found""" @@ -519,9 +541,86 @@ class TestClosestBranch(common.TransactionCase): 'repo_id': self.enterprise_dev_repo.id, 'name': 'refs/head/badref-fix-foo' }) - addons_build = self.Build.create({ - 'branch_id': addons_branch.id, - 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', - }) + self.assertEqual((self.branch_odoo_master, 'default'), addons_branch._get_closest_branch(self.community_repo.id)) - self.assertEqual((self.community_repo.id, 'refs/heads/master', 'default'), addons_build._get_closest_branch_name(self.community_repo.id)) + @patch('odoo.addons.runbot.models.branch.runbot_branch._is_on_remote') + def test_no_duplicate_update(self, mock_is_on_remote): + """push a dev branch in enterprise with same head as sticky, but with a matching branch in community""" + mock_is_on_remote.return_value = True + community_sticky_branch = self.Branch.create({ + 'repo_id': self.community_repo.id, + 'name': 'refs/heads/saas-12.2', + 'sticky': True, + }) + community_dev_branch = self.Branch.create({ + 'repo_id': self.community_dev_repo.id, + 'name': 'refs/heads/saas-12.2-dev1', + }) + enterprise_sticky_branch = self.Branch.create({ + 'repo_id': self.enterprise_repo.id, + 'name': 'refs/heads/saas-12.2', + 'sticky': True, + }) + enterprise_dev_branch = self.Branch.create({ + 'repo_id': self.enterprise_dev_repo.id, + 'name': 'refs/heads/saas-12.2-dev1' + }) + # we shouldn't have duplicate since community_dev_branch exists + with patch('odoo.addons.runbot.models.repo.runbot_repo._git_rev_parse', new=rev_parse): + # lets create an old enterprise build + self.Build.create({ + 'branch_id': enterprise_sticky_branch.id, + 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', + }) + self.assertNoDuplicate( + enterprise_sticky_branch, + enterprise_dev_branch, + (community_sticky_branch, 'exact'), + (community_dev_branch, 'exact'), + ) + + @patch('odoo.addons.runbot.models.repo.runbot_repo._github') + def test_external_pr_closest_branch(self, mock_github): + """ test last resort value target_name""" + mock_github.return_value = { + 'head': {'label': 'external_repo:11.0-fix'}, + 'base': {'ref': '11.0'}, + 'state': 'open' + } + enterprise_pr = self.Branch.create({ + 'repo_id': self.enterprise_repo.id, + 'name': 'refs/pull/123456' + }) + dependency_repo = self.enterprise_repo.dependency_ids[0] + closest_branch = enterprise_pr._get_closest_branch(dependency_repo.id) + self.assertEqual(enterprise_pr._get_closest_branch(dependency_repo.id), (self.branch_odoo_11, 'pr_target')) + + @patch('odoo.addons.runbot.models.repo.runbot_repo._github') + def test_external_pr_with_comunity_pr_closest_branch(self, mock_github): + """ test matching external pr """ + mock_github.return_value = { + 'head': {'label': 'external_dev_repo:11.0-fix'}, + 'base': {'ref': '11.0'}, + 'state': 'open' + } + community_pr = self.Branch.create({ + 'repo_id': self.community_repo.id, + 'name': 'refs/pull/123456' + }) + mock_github.return_value = { + 'head': {'label': 'external_dev_repo:11.0-fix'}, # if repo doenst match, it wont work, maybe a fix to do here? + 'base': {'ref': '11.0'}, + 'state': 'open' + } + enterprise_pr = self.Branch.create({ + 'repo_id': self.enterprise_repo.id, + 'name': 'refs/pull/123' + }) + with patch('odoo.addons.runbot.models.repo.runbot_repo._git_rev_parse', new=rev_parse): + build = self.Build.create({ + 'branch_id': enterprise_pr.id, + 'name': 'd0d0caca0000ffffffffffffffffffffffffffff', + }) + dependency_repo = build.repo_id.dependency_ids[0] + self.assertEqual(build.branch_id._get_closest_branch(dependency_repo.id), (community_pr, 'exact PR')) + # this is working here because pull_head_name is set, but on runbot pull_head_name is empty for external pr. why? diff --git a/runbot/tests/test_frontend.py b/runbot/tests/test_frontend.py index 05535f0e..01e69ed5 100644 --- a/runbot/tests/test_frontend.py +++ b/runbot/tests/test_frontend.py @@ -37,7 +37,8 @@ class Test_Frontend(common.HttpCase): names = ['deadbeef', 'd0d0caca', 'deadface', 'cacafeed'] # create 5 builds in each branch for i, state, branch, name in zip(range(10), cycle(states), cycle(branches), cycle(names)): - self.Build.create({ + name = '%s%s' % (name, i) + build = self.Build.create({ 'branch_id': branch.id, 'name': '%s0000ffffffffffffffffffffffffffff' % name, 'port': '1234',