bug fix and performance improve
cover bug fix and performance improve
Hi Paul,
The patch 4 can improve a lot on update.py (up to 98%), here is the testing data, please feel free to give your comments.
$ time update.py -b master --nofetch [--fullreload]
Before Now Reduced No update: 276s 3.6s 98% Partial update: 312s 87s 72% Full repload: 1016s 980s 3%
Note:
* All of the testing are based on --nofetch
* "No update" means all layers on the branch is up-to-date, for example, when we run it twice, there is no update in the second run, so we only need about 3s now, which is the most common case when we use cron to run it per half an hour.
* "Partly update" means part of the layers have been updated.
* "Fullreload" means all of the layers have been updated.
// Robert
The following changes since commit 611c96883c35240d3c291951146154d828745774:
requirements.txt: use the most recent Django 1.8 version (2018-03-26 08:29:27 +1300)
are available in the git repository at:
git://git.pokylinux.org/poky-contrib rbt/layerindex
http://git.pokylinux.org/cgit.cgi//log/?h=rbt/layerindex
Robert Yang (4):
fixup! update: don't stop on unsatisfied layer dependencies update.py: add an option --timeout for lockfile update.py: print failed layers summary in the end update_layer.py: move layer validation to update.py (Performance
improve)
layerindex/update.py | 97 ++++++++++++++++++++++++++++++++++++++++++++-- layerindex/update_layer.py | 39 ++----------------- layerindex/utils.py | 14 +++++-- 3 files changed, 108 insertions(+), 42 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/4 fixup! update: don't stop on unsatisfied layer dependencies
The previous commit broke the layer order, e.g.:
A -> B -> C -> D
The algorithm is checking the dependencies one by one, and until we find D, add D to layerquery_sorted, and add it "collections", the one in "collections" means it's dependencies are OK, then C, B and A will check against collections, so that update_layer.py will update them one by one. The previous commit added A/B/C/D to collections directly, so that when check against it, all the dependencies are met, thus broke the layer sorting, and then there would be failures if we pass layer A to update_layer.py before B, C and D (suppose they are newly to database). This commit fix the problem.
BTW., why I use collections to record the one whose dependencies are matched, but not directly use layerquery_sorted, it is because collections contains both the ones to be added/updated and the ones in database, but layerquery_sorted only contains the ones to be updated/added.
--- layerindex/update.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
layerindex/update.py +2 -2
@@ -348,12 +348,11 @@ def main(): deps = re.search("^LAYERDEPENDS = \"(.*)\"", output, re.M).group(1) or '' recs = re.search("^LAYERRECOMMENDS = \"(.*)\"", output, re.M).group(1) or '' - collections.add((col, ver)) - deps_dict = utils.explode_dep_versions2(bitbakepath, deps + ' ' + recs) if len(deps_dict) == 0: # No depends, add it firstly layerquery_sorted.append(layer) + collections.add((col, ver)) continue deps_dict_all[layer] = {'requires': deps_dict, 'collection': col, 'version': ver}
@@ -374,6 +373,7 @@ def main(): # All the depends are in collections: del(deps_dict_all[layer]) layerquery_sorted.append(layer) + collections.add((value['collection'], value['version'])) if not len(deps_dict_all): break
Signature · 1 line
2.7.4
PATCH 2/4 update.py: add an option --timeout for lockfile
In reply to Robert YangWe have an update.py running perodically in background, but we also need run it manually, for example, run it to update actual_branch, the manually run usually failed because can't get lockfile, we have to run it again and again. A timeout option helps a lot in such a case. Now the following command can make sure we can run the command successfully:
$ update.py -b master -a actual_branch -t 2000
--- layerindex/update.py | 5 ++++- layerindex/utils.py | 14 +++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-)
layerindex/update.py +4 -1
@@ -156,6 +156,9 @@ def main(): parser.add_option("-l", "--layer", help = "Specify layers to update (use commas to separate multiple). Default is all published layers.", action="store", dest="layers") + parser.add_option("-t", "--timeout", + help = "Specify timeout in seconds to get layerindex.lock. Default is 30 seconds.", + type="int", action="store", dest="timeout", default=30) parser.add_option("-r", "--reload", help = "Reload recipe data instead of updating since last update", action="store_true", dest="reload")
@@ -265,7 +268,7 @@ def main(): update.save() try: lockfn = os.path.join(fetchdir, "layerindex.lock") - lockfile = utils.lock_file(lockfn) + lockfile = utils.lock_file(lockfn, options.timeout, logger) if not lockfile: logger.error("Layer index lock timeout expired") sys.exit(1)
layerindex/utils.py +11 -3
@@ -309,8 +309,10 @@ class ListHandler(logging.Handler): return log -def lock_file(fn): - starttime = time.time() +def lock_file(fn, timeout=30, logger=None): + start = time.time() + last = start + counter = 1 while True: lock = open(fn, 'w') try:
@@ -318,8 +320,14 @@ def lock_file(fn): return lock except IOError: lock.close() - if time.time() - starttime > 30: + current = time.time() + if current - start > timeout: return None + # Print a message in every 5 seconds + if logger and (current - last > 5): + last = current + logger.info('Trying to get lock on %s (tried %s seconds) ...' % (fn, (5 * counter))) + counter += 1 def unlock_file(lock): fcntl.flock(lock, fcntl.LOCK_UN)
Signature · 1 line
2.7.4
PATCH 3/4 update.py: print failed layers summary in the end
In reply to Robert YangThis makes it easy to see which layers are failed. For example:
ERROR: Failed layers on branch master: openembedded-core meta-python
--- layerindex/update.py | 9 +++++++++ 1 file changed, 9 insertions(+)
layerindex/update.py +9 -0
@@ -319,6 +319,7 @@ def main(): # unreliable due to leaking memory (we're using bitbake internals in a manner in which # they never get used during normal operation). last_rev = {} + failed_layers = {} for branch in branches: # If layer_A depends(or recommends) on layer_B, add layer_B before layer_A deps_dict_all = {}
@@ -384,9 +385,12 @@ def main(): # If nothing changed after a run then some dependencies couldn't be resolved if operator.eq(deps_dict_all_copy, deps_dict_all): logger.warning("Cannot find required collections on branch %s:" % branch) + layer_names = [] for layer, value in deps_dict_all.items(): logger.error('%s: %s' % (layer.name, value['requires'])) + layer_names.append(layer.name) logger.warning("Known collections on branch %s: %s" % (branch, collections)) + failed_layers[branch] = layer_names break for layer in layerquery_sorted:
@@ -427,6 +431,11 @@ def main(): # Interrupted by user, break out of loop logger.info('Update interrupted, exiting') sys.exit(254) + if failed_layers: + print() + for branch, err_msg_list in failed_layers.items(): + logger.error("Failed layers on branch %s: %s" % (branch, " ".join(err_msg_list))) + print() finally: utils.unlock_file(lockfile)
Signature · 1 line
2.7.4
PATCH 4/4 update_layer.py: move layer validation to update.py (Performance improve)
In reply to Robert YangThe utils.setup_django() costs a lot of time, but both update.py and update_layer.py calls it, so move layer validation from update_layer.py to update.py to avoid calling update_layer.py when possible can save a lot of time.
Now we don't have to call update_layer.py in the following cases:
* The branch doesn't exist
* The layer is already update to date on specified branch (when no reload)
* The layer dir or conf/layer.layer doesn't exist
We can save up to 98% time in my testing:
$ update.py -b master --nofetch [--fullreload]
Before Now Reduced No update: 276s 3.6s 98% Partial update: 312s 87s 72% Full repload: 1016s 980s 3%
Note:
* All of the testing are based on --nofetch
* "No update" means all layers on the branch is up-to-date, for example, when we run it twice, there is no update in the second run, so we only need about 3s now, which is the most common case when we use cron to run it per half an hour.
* "Partly update" means part of the layers have been updated.
* "Fullreload" means all of the layers have been updated.
--- layerindex/update.py | 79 +++++++++++++++++++++++++++++++++++++++++++++- layerindex/update_layer.py | 39 +++-------------------- 2 files changed, 82 insertions(+), 36 deletions(-)
layerindex/update.py +78 -1
@@ -140,6 +140,15 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name): logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output)) return (vcs_url, e.output) +def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc): + # This will error out if the directory is completely invalid or had never existed at this point + # If it previously existed but has since been deleted, you will get the revision where it was + # deleted - so we need to handle that case separately later + if newbranch: + logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layername, branchdesc, vcs_subdir)) + elif vcs_subdir: + logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc)) + def main(): if LooseVersion(git.__version__) < '0.3.1': logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")
@@ -195,7 +204,7 @@ def main(): utils.setup_django() import settings - from layerindex.models import Branch, LayerItem, Update, LayerUpdate + from layerindex.models import Branch, LayerItem, Update, LayerUpdate, LayerBranch logger.setLevel(options.loglevel)
@@ -337,6 +346,74 @@ def main(): collections.add((layerbranch.collection, layerbranch.version)) for layer in layerquery: + layerbranch = layer.get_layerbranch(branch) + branchname = branch + branchdesc = branch + newbranch = False + branchobj = utils.get_branch(branch) + if layerbranch: + if layerbranch.actual_branch: + branchname = layerbranch.actual_branch + branchdesc = "%s (%s)" % (branch, branchname) + else: + # LayerBranch doesn't exist for this branch, create it + newbranch = True + layerbranch = LayerBranch() + layerbranch.layer = layer + layerbranch.branch = branchobj + layerbranch_source = layer.get_layerbranch(branchobj) + if not layerbranch_source: + layerbranch_source = layer.get_layerbranch(None) + if layerbranch_source: + layerbranch.vcs_subdir = layerbranch_source.vcs_subdir + + # Collect repo info + urldir = layer.get_fetch_dir() + repodir = os.path.join(fetchdir, urldir) + repo = git.Repo(repodir) + assert repo.bare == False + try: + if options.nocheckout: + topcommit = repo.commit('HEAD') + else: + topcommit = repo.commit('origin/%s' % branchname) + except: + if newbranch: + logger.info("Skipping update of layer %s - branch %s doesn't exist" % (layer.name, branchdesc)) + else: + logger.info("layer %s - branch %s no longer exists, removing it from database" % (layer.name, branchdesc)) + if not options.dryrun: + layerbranch.delete() + continue + + if layerbranch.vcs_subdir and not options.nocheckout: + # Find latest commit in subdirectory + # A bit odd to do it this way but apparently there's no other way in the GitPython API + topcommit = next(repo.iter_commits('origin/%s' % branchname, paths=layerbranch.vcs_subdir), None) + if not topcommit: + print_subdir_error(newbranch, layer.name, layerbranch.vcs_subdir, branchdesc) + if not (newbranch and layerbranch.vcs_subdir): + logger.error("Failed to get last revision for layer %s on branch %s" % (layer.name, branchdesc)) + continue + + if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload: + logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc)) + collections.add((layerbranch.collection, layerbranch.version)) + continue + + if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload: + # Check out appropriate branch + if not options.nocheckout: + utils.checkout_layer_branch(layerbranch, repodir, logger=logger) + layerdir = os.path.join(repodir, layerbranch.vcs_subdir) + if layerbranch.vcs_subdir and not os.path.exists(layerdir): + print_subdir_error(newbranch, layer.name, layerbranch.vcs_subdir, branchdesc) + continue + + if not os.path.exists(os.path.join(layerdir, 'conf/layer.conf')): + logger.error("conf/layer.conf not found for layer %s - is subdirectory set correctly?" % layer.name) + continue + cmd = prepare_update_layer_command(options, branchobj, layer, initial=True) logger.debug('Running layer update command: %s' % cmd) ret, output = run_command_interruptible(cmd)
layerindex/update_layer.py +4 -35
@@ -287,19 +287,10 @@ def main(): # Collect repo info repo = git.Repo(repodir) assert repo.bare == False - try: - if options.nocheckout: - topcommit = repo.commit('HEAD') - else: - topcommit = repo.commit('origin/%s' % branchname) - except: - if layerbranch: - logger.info("layer %s - branch %s no longer exists, removing it from database" % (layer.name, branchdesc)) - if not options.dryrun: - layerbranch.delete() - else: - logger.info("Skipping update of layer %s - branch %s doesn't exist" % (layer.name, branchdesc)) - sys.exit(1) + if options.nocheckout: + topcommit = repo.commit('HEAD') + else: + topcommit = repo.commit('origin/%s' % branchname) tinfoil = None tempdir = None
@@ -329,17 +320,6 @@ def main(): # Find latest commit in subdirectory # A bit odd to do it this way but apparently there's no other way in the GitPython API topcommit = next(repo.iter_commits('origin/%s' % branchname, paths=layerbranch.vcs_subdir), None) - if not topcommit: - # This will error out if the directory is completely invalid or had never existed at this point - # If it previously existed but has since been deleted, you will get the revision where it was - # deleted - so we need to handle that case separately later - if newbranch: - logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layer.name, branchdesc, layerbranch.vcs_subdir)) - elif layerbranch.vcs_subdir: - logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layer.name, branchdesc)) - else: - logger.error("Failed to get last revision for layer %s on branch %s" % (layer.name, branchdesc)) - sys.exit(1) layerdir = os.path.join(repodir, layerbranch.vcs_subdir) layerdir_start = os.path.normpath(layerdir) + os.sep
@@ -354,17 +334,6 @@ def main(): if not options.nocheckout: utils.checkout_layer_branch(layerbranch, repodir, logger=logger) - if layerbranch.vcs_subdir and not os.path.exists(layerdir): - if newbranch: - logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layer.name, branchdesc, layerbranch.vcs_subdir)) - else: - logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layer.name, branchdesc)) - sys.exit(1) - - if not os.path.exists(os.path.join(layerdir, 'conf/layer.conf')): - logger.error("conf/layer.conf not found for layer %s - is subdirectory set correctly?" % layer.name) - sys.exit(1) - logger.info("Collecting data for layer %s on branch %s" % (layer.name, branchdesc)) try: (tinfoil, tempdir) = recipeparse.init_parser(settings, branch, bitbakepath, nocheckout=options.nocheckout, logger=logger)
Signature · 1 line
2.7.4
Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)
Quoted 4 lines · NZST Robert Yang
The
utils.setup_django()costs a lot of time, but bothupdate.pyandupdate_layer.pycalls it, so move layer validation fromupdate_layer.pytoupdate.pyto avoid callingupdate_layer.pywhen possible can save a lot of time.
Unfortunately I still can't merge this, as mentioned earlier it breaks handling older python 2-bound releases.
Cheers,
Paul
Signature · 2 lines
Paul Eggleton Intel Open Source Technology Centre
Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)
Hi Paul,
Quoted 8 lines · Paul Eggleton
Quoted 4 lines · NZST Robert Yang
The
utils.setup_django()costs a lot of time, but bothupdate.pyandupdate_layer.pycalls it, so move layer validation fromupdate_layer.pytoupdate.pyto avoid callingupdate_layer.pywhen possible can save a lot of time.Unfortunately I still can't merge this, as mentioned earlier it breaks handling older python 2-bound releases.
I think you're talking about the one that I sent a few months ago, but this one is a completely new patch, that one moved tinfoil into update.py which broke python2, but this patch doesn't, it doesn't touch anything about tinfo, so it works when I switch from YP 2.0 -> 2.2 -> 2.4, and vice visa.
// Robert
Quoted 4 lines
Cheers,
Paul
Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)
Hi Robert
Quoted 4 lines · NZST Robert Yang
Quoted 3 lines · Paul Eggleton
Quoted 2 lines · NZST Robert Yang
The
utils.setup_django()costs a lot of time, but bothupdate.pyandupdate_layer.pycalls it, so move layer validation fromupdate_layer.py
to
Quoted 1 line
Quoted 1 line
Quoted 1 line
update.pyto avoid callingupdate_layer.pywhen possible can save a lot
of
Quoted 10 lines
Quoted 4 lines
Quoted 1 line
time.
Unfortunately I still can't merge this, as mentioned earlier it breaks handling older python 2-bound releases.
I think you're talking about the one that I sent a few months ago, but this one is a completely new patch, that one moved tinfoil into
update.pywhich broke python2, but this patch doesn't, it doesn't touch anything about tinfo,
so it works when I switch from YP 2.0 -> 2.2 -> 2.4, and vice visa.
My apologies - I didn't read the patch properly. Let me take a closer look at it tomorrow.
Cheers,
Paul
Signature · 2 lines
Paul Eggleton Intel Open Source Technology Centre
Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)
In reply to Robert YangHi Robert
I like the performance improvements, thanks. A few comments below though.
Quoted 82 lines · NZST Robert Yang
The
utils.setup_django()costs a lot of time, but bothupdate.pyandupdate_layer.pycalls it, so move layer validation fromupdate_layer.pytoupdate.pyto avoid callingupdate_layer.pywhen possible can save a lot of time.Now we don't have to call
update_layer.pyin the following cases:
* The branch doesn't exist
* The layer is already update to date on specified branch (when no reload)
* The layer dir or conf/layer.layer doesn't existWe can save up to 98% time in my testing:
$ update.py -b master --nofetch [--fullreload]Before Now Reduced No update: 276s 3.6s 98% Partial update: 312s 87s 72% Full repload: 1016s 980s 3%Note:
* All of the testing are based on --nofetch* "No update" means all layers on the branch is up-to-date, for example, when we run it twice, there is no update in the second run, so we only need about 3s now, which is the most common case when we use cron to run it per half an hour.
* "Partly update" means part of the layers have been updated.
* "Fullreload" means all of the layers have been updated.
Signed-off-by Robert Yang <[email protected]>--- layerindex/update.py | 79 +++++++++++++++++++++++++++++++++++++++++++++- layerindex/update_layer.py | 39 +++-------------------- 2 files changed, 82 insertions(+), 36 deletions(-)layerindex/update.py +21
-1@@ -140,6 +140,15 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name): logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output)) return (vcs_url, e.output) +def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc): + # This will error out if the directory is completely invalid or had never existed at this point + # If it previously existed but has since been deleted, you will get the revision where it was + # deleted - so we need to handle that case separately later + if newbranch: + logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layername, branchdesc, vcs_subdir)) + elif vcs_subdir: + logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc)) + def main(): if LooseVersion(git.__version__) < '0.3.1': logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")@@ -195,7 +204,7 @@ def main(): utils.setup_django() import settings - from layerindex.models import Branch, LayerItem, Update, LayerUpdate + from layerindex.models import Branch, LayerItem, Update, LayerUpdate, LayerBranch logger.setLevel(options.loglevel)@@ -337,6 +346,74 @@ def main(): collections.add((layerbranch.collection, layerbranch.version)) for layer in layerquery: + layerbranch = layer.get_layerbranch(branch) + branchname = branch + branchdesc = branch + newbranch = False + branchobj = utils.get_branch(branch) + if layerbranch: + if layerbranch.actual_branch: + branchname = layerbranch.actual_branch + branchdesc = "%s (%s)" % (branch, branchname) + else: + # LayerBranch doesn't exist for this branch, create it
layerindex/update.py def main(): · line 369
Quoted 10 lines
layerindex/update.py +10
-0+ newbranch = True + layerbranch = LayerBranch() + layerbranch.layer = layer + layerbranch.branch = branchobj + layerbranch_source = layer.get_layerbranch(branchobj) + if not layerbranch_source: + layerbranch_source = layer.get_layerbranch(None) + if layerbranch_source: + layerbranch.vcs_subdir = layerbranch_source.vcs_subdir +
It seems that you are missing a call somewhere to layerbranch.save() later on, guarded with "if newbranch:". This was easy to miss though when copying over from update_layer.py seeing as we were relying upon saving it later on there (and that is still needed).
layerindex/update.py def main(): · line 407
Quoted 9 lines
layerindex/update.py +9
-0+ if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload: + logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc)) + collections.add((layerbranch.collection, layerbranch.version)) + continue + + if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload: + # Check out appropriate branch + if not options.nocheckout: + utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:" superfluous here? We're skipping to the next layer if that wasn't true via "continue" above, so it should always evaluate to true.
Additionally this results in the checkout being done twice - once here and a second time in the update_layer.py script. I guess it's not taking much time the second time though so we could probably ignore it.
Cheers,
Paul
Signature · 2 lines
Paul Eggleton Intel Open Source Technology Centre
Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)
Hi Paul,
Thanks for the reply, please see my comments inline.
Quoted 94 lines · Paul Eggleton
Hi Robert
I like the performance improvements, thanks. A few comments below though.
Quoted 89 lines · NZST Robert Yang
The
utils.setup_django()costs a lot of time, but bothupdate.pyandupdate_layer.pycalls it, so move layer validation fromupdate_layer.pytoupdate.pyto avoid callingupdate_layer.pywhen possible can save a lot of time.Now we don't have to call
update_layer.pyin the following cases:
* The branch doesn't exist
* The layer is already update to date on specified branch (when no reload)
* The layer dir or conf/layer.layer doesn't existWe can save up to 98% time in my testing:
$ update.py -b master --nofetch [--fullreload]Before Now Reduced
No update: 276s 3.6s 98% Partial update: 312s 87s 72% Full repload: 1016s 980s 3%Note:
* All of the testing are based on --nofetch* "No update" means all layers on the branch is up-to-date, for example, when we run it twice, there is no update in the second run, so we only need about 3s now, which is the most common case when we use cron to run it per half an hour.
* "Partly update" means part of the layers have been updated.
* "Fullreload" means all of the layers have been updated.
Signed-off-by Robert Yang <[email protected]>--- layerindex/update.py | 79 +++++++++++++++++++++++++++++++++++++++++++++- layerindex/update_layer.py | 39 +++-------------------- 2 files changed, 82 insertions(+), 36 deletions(-)layerindex/update.py +28
-1@@ -140,6 +140,15 @@ def fetch_repo(vcs_url, repodir, urldir, fetchdir, layer_name): logger.error("Fetch of layer %s failed: %s" % (layer_name, e.output)) return (vcs_url, e.output) +def print_subdir_error(newbranch, layername, vcs_subdir, branchdesc): + # This will error out if the directory is completely invalid or had never existed at this point + # If it previously existed but has since been deleted, you will get the revision where it was + # deleted - so we need to handle that case separately later + if newbranch: + logger.info("Skipping update of layer %s for branch %s - subdirectory %s does not exist on this branch" % (layername, branchdesc, vcs_subdir)) + elif vcs_subdir: + logger.error("Subdirectory for layer %s does not exist on branch %s - if this is legitimate, the layer branch record should be deleted" % (layername, branchdesc)) + def main(): if LooseVersion(git.__version__) < '0.3.1': logger.error("Version of GitPython is too old, please install GitPython (python-git) 0.3.1 or later in order to use this script")@@ -195,7 +204,7 @@ def main(): utils.setup_django() import settings - from layerindex.models import Branch, LayerItem, Update, LayerUpdate + from layerindex.models import Branch, LayerItem, Update, LayerUpdate, LayerBranch logger.setLevel(options.loglevel)@@ -337,6 +346,74 @@ def main(): collections.add((layerbranch.collection, layerbranch.version)) for layer in layerquery: + layerbranch = layer.get_layerbranch(branch) + branchname = branch + branchdesc = branch + newbranch = False + branchobj = utils.get_branch(branch) + if layerbranch: + if layerbranch.actual_branch: + branchname = layerbranch.actual_branch + branchdesc = "%s (%s)" % (branch, branchname) + else: + # LayerBranch doesn't exist for this branch, create it + newbranch = True + layerbranch = LayerBranch() + layerbranch.layer = layer + layerbranch.branch = branchobj + layerbranch_source = layer.get_layerbranch(branchobj) + if not layerbranch_source: + layerbranch_source = layer.get_layerbranch(None)
Quoted 8 lines
layerindex/update.py def main(): · line 369
Quoted 3 lines
layerindex/update.py +3
-0+ if layerbranch_source: + layerbranch.vcs_subdir = layerbranch_source.vcs_subdir +It seems that you are missing a call somewhere to
layerbranch.save()later on, guarded with "if newbranch:". This was easy to miss though when copying over fromupdate_layer.pyseeing as we were relying upon saving it later on there (and that is still needed).
We don't need it to save the new branch here, we just want to know whether it is a new branch or not, the logic is:
if newbranch and "the branch doesn't exist" in the repo:
continue
else: # "the branch exits in the repo"
update_layer.py
So we don't have to save the new branch here, the update_layer.py will save the new branch when needed, this can reducing a lot of calls to update_layer, which can save a lot of time.
Quoted 11 lines
layerindex/update.py def main(): · line 407
Quoted 10 lines
layerindex/update.py +9
-0+ if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload: + logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc)) + collections.add((layerbranch.collection, layerbranch.version)) + continue + + if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload: + # Check out appropriate branch + if not options.nocheckout: + utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
Quoted 3 lines
Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:" superfluous here? We're skipping to the next layer if that wasn't true via "continue" above, so it should always evaluate to true.
Let's ignore the update.reload here, then the logic will be clear:
if layerbranch.vcs_last_rev == topcommit.hexsha:
continue
else:
checkout_layer_branch()
So we can't ignore the else block here, the "if" block doesn't cover the "else" block. And If update.reload, then we can't continue, so the first "if" is:
"if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload"
And for the second "if", we need checkout the branch when:
"layerbranch.vcs_last_rev != topcommit.hexsha or update.reload"
And check whether there is a conf/layer.layer in the repo, we have a lot of layers which are only for specific releases, for example, a layer is for morty branch only, then we cleanup its master branch, just leave a README (no conf/layer.conf) file on master branch which says this layer is for morty only, check conf/layer.conf in update.py rather than in update_layer.py can save a lot of time, too.
Quoted 4 lines
Additionally this results in the checkout being done twice - once here and a second time in the
update_layer.pyscript. I guess it's not taking much time the second time though so we could probably ignore it.
Yes, you're right, the second checkout will be much faster, and it's worth to do it in update.py since calling update_layer.py is very slow.
The update_layer.py is slow is because setup_django() is slow, and we call setup_django() in both update.py and update_layer.py, so avoid calling update_layer.py when possible can save a lot of time.
// Robert
Quoted 4 lines
Cheers,
Paul
Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)
Quoted 9 lines · NZST Robert Yang
Quoted 4 lines · Paul Eggleton
It seems that you are missing a call somewhere to
layerbranch.save()later on, guarded with "if newbranch:". This was easy to miss though when copying over fromupdate_layer.pyseeing as we were relying upon saving it later on there (and that is still needed).We don't need it to save the new branch here, we just want to know whether it is a new branch or not, the logic is:
Quoted 8 lines
if newbranch and "the branch doesn't exist" in the repo:
continue
else: # "the branch exits in the repo"
update_layer.pySo we don't have to save the new branch here, the
update_layer.pywill save the new branch when needed, this can reducing a lot of calls toupdate_layer, which can save a lot of time.
OK - I had missed that you hadn't removed the code from update_layer.py that does the actual creation, so the logic is fine. I think however we need to adjust the "LayerBranch doesn't exist for this branch, create it" comment here though, something like "LayerBranch doesn't exist for this branch, create it temporarily (we won't save this - update_layer.py will do the actual creation if it gets called)."
Quoted 28 lines
Quoted 10 lines
layerindex/update.py def main(): · line 407
Quoted 9 lines
layerindex/update.py +9
-0+ if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload: + logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc)) + collections.add((layerbranch.collection, layerbranch.version)) + continue + + if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload: + # Check out appropriate branch + if not options.nocheckout: + utils.checkout_layer_branch(layerbranch, repodir, logger=logger)Quoted 3 lines
Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:" superfluous here? We're skipping to the next layer if that wasn't true via "continue" above, so it should always evaluate to true.
Let's ignore the update.reload here, then the logic will be clear:
if layerbranch.vcs_last_rev == topcommit.hexsha:
continue
else:
checkout_layer_branch()So we can't ignore the else block here, the "if" block doesn't cover the "else" block. And If update.reload, then we can't continue, so the first "if" is:
"if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload"And for the second "if", we need checkout the branch when:
"layerbranch.vcs_last_rev != topcommit.hexsha or update.reload"
Quoted 6 lines
And check whether there is a conf/layer.layer in the repo, we have a lot of layers which are only for specific releases, for example, a layer is for morty branch only, then we cleanup its master branch, just leave a README (no
conf/layer.conf) file on master branch which says this layer is for morty only, checkconf/layer.confinupdate.pyrather than inupdate_layer.pycan save a lot of time, too.
Right, I understand the logic, what I'm saying is that the two if statements are opposites, and the first one results in a "continue" if true, so the second one can logically never evaluate to false, thus there's no point doing the check - just put the code inside the block immediately afterwards, i.e.:
if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc)) collections.add((layerbranch.collection, layerbranch.version)) continue
# Check out appropriate branch
if not options.nocheckout:
utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
...
If you wanted to make it even more clear you could have it in an "else" block instead, but that's not strictly necessary.
Cheers,
Paul
Signature · 2 lines
Paul Eggleton Intel Open Source Technology Centre
Re: [PATCH 4/4] update_layer.py: move layer validation to update.py (Performance improve)
Yes, you're right, I updated the two comments in the repo:
+ if layerbranch:
+ if layerbranch.actual_branch:
+ branchname = layerbranch.actual_branch
+ branchdesc = "%s (%s)" % (branch, branchname)
+ else:
+ # LayerBranch doesn't exist for this branch, create it temporarily + # (we won't save this - update_layer.py will do the actual creation + # if it gets called). + newbranch = True + layerbranch = LayerBranch() + layerbranch.layer = layer
...
+ if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
+ logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))
+ collections.add((layerbranch.collection, layerbranch.version))
+ continue+ else:
+ # Check out appropriate branch
+ if not options.nocheckout:
+ utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
+ layerdir = os.path.join(repodir, layerbranch.vcs_subdir)
+ if layerbranch.vcs_subdir and not os.path.exists(layerdir):
+ print_subdir_error(newbranch, layer.name, layerbranch.vcs_subdir, branchdesc)
+ continue
+
+ if not os.path.exists(os.path.join(layerdir, 'conf/layer.conf')):
+ logger.error("conf/layer.conf not found for layer %s - is subdirectory set correctly?" % layer.name)
+ continue
// Robert
Quoted 83 lines · Paul Eggleton
Quoted 9 lines · NZST Robert Yang
Quoted 4 lines · Paul Eggleton
It seems that you are missing a call somewhere to
layerbranch.save()later on, guarded with "if newbranch:". This was easy to miss though when copying over fromupdate_layer.pyseeing as we were relying upon saving it later on there (and that is still needed).We don't need it to save the new branch here, we just want to know whether it is a new branch or not, the logic is:
Quoted 8 lines
if newbranch and "the branch doesn't exist" in the repo:
continue
else: # "the branch exits in the repo"
update_layer.pySo we don't have to save the new branch here, the
update_layer.pywill save the new branch when needed, this can reducing a lot of calls toupdate_layer, which can save a lot of time.OK - I had missed that you hadn't removed the code from
update_layer.pythat does the actual creation, so the logic is fine. I think however we need to adjust the "LayerBranch doesn't exist for this branch, create it" comment here though, something like "LayerBranch doesn't exist for this branch, create it temporarily (we won't save this -update_layer.pywill do the actual creation if it gets called)."Quoted 28 lines
Quoted 10 lines
layerindex/update.py def main(): · line 407
Quoted 9 lines
layerindex/update.py +9
-0+ if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload: + logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc)) + collections.add((layerbranch.collection, layerbranch.version)) + continue + + if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload: + # Check out appropriate branch + if not options.nocheckout: + utils.checkout_layer_branch(layerbranch, repodir, logger=logger)Quoted 3 lines
Isn't "if layerbranch.vcs_last_rev != topcommit.hexsha or update.reload:" superfluous here? We're skipping to the next layer if that wasn't true via "continue" above, so it should always evaluate to true.
Let's ignore the update.reload here, then the logic will be clear:
if layerbranch.vcs_last_rev == topcommit.hexsha:
continue
else:
checkout_layer_branch()So we can't ignore the else block here, the "if" block doesn't cover the "else" block. And If update.reload, then we can't continue, so the first "if" is:
"if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload"And for the second "if", we need checkout the branch when:
"layerbranch.vcs_last_rev != topcommit.hexsha or update.reload"Quoted 6 lines
And check whether there is a conf/layer.layer in the repo, we have a lot of layers which are only for specific releases, for example, a layer is for morty branch only, then we cleanup its master branch, just leave a README (no
conf/layer.conf) file on master branch which says this layer is for morty only, checkconf/layer.confinupdate.pyrather than inupdate_layer.pycan save a lot of time, too.Right, I understand the logic, what I'm saying is that the two if statements are opposites, and the first one results in a "continue" if true, so the second one can logically never evaluate to false, thus there's no point doing the check - just put the code inside the block immediately afterwards, i.e.:
if layerbranch.vcs_last_rev == topcommit.hexsha and not update.reload:
logger.info("Layer %s is already up-to-date for branch %s" % (layer.name, branchdesc))collections.add((layerbranch.collection, layerbranch.version))continue# Check out appropriate branch
if not options.nocheckout:
utils.checkout_layer_branch(layerbranch, repodir, logger=logger)
...If you wanted to make it even more clear you could have it in an "else" block instead, but that's not strictly necessary.
Cheers,
Paul
Quoted 1 line · Robert Yang
fixup! update: don't stop on unsatisfied layer dependencies
Pretty sure this isn't what you intended.
Ross
Hi Ross,
Quoted 4 lines · Ross
Quoted 1 line · Robert Yang
fixup! update: don't stop on unsatisfied layer dependencies
Pretty sure this isn't what you intended.
Thanks, but this is intended, I used:
$ git commit --fixup=8dd24abf99578e3d2572062ca04dbeac4321b2ea
While the 8dd24abf99578e3d2572062ca04dbeac4321b2ea is made by Paul, I use this way to specify what it fixes, I can update the subject line if this is not acceptable.
// Robert
Quoted 3 lines
Ross