[opensuse-buildservice] [PATCH] build: Do not leave filedescriptors open

There is no need for the whole build process to have the _buildinfo* and _buildconfig* files open. Signed-off-by: Michal Marek <mmarek@suse.cz> --- osc/build.py | 23 ++++++++++++----------- 1 files changed, 12 insertions(+), 11 deletions(-) diff --git a/osc/build.py b/osc/build.py index 3c6aa6c..a77865c 100644 --- a/osc/build.py +++ b/osc/build.py @@ -527,14 +527,16 @@ def main(apiurl, opts, argv): bc_file = None bi_filename = '_buildinfo-%s-%s.xml' % (repo, arch) bc_filename = '_buildconfig-%s-%s' % (repo, arch) + buildfiles_temporary = False if is_package_dir('.') and os.access(osc.core.store, os.W_OK): bi_filename = os.path.join(os.getcwd(), osc.core.store, bi_filename) bc_filename = os.path.join(os.getcwd(), osc.core.store, bc_filename) elif not os.access('.', os.W_OK): - bi_file = NamedTemporaryFile(prefix=bi_filename) + bi_file = NamedTemporaryFile(prefix=bi_filename, delete=False) bi_filename = bi_file.name - bc_file = NamedTemporaryFile(prefix=bc_filename) + bc_file = NamedTemporaryFile(prefix=bc_filename, delete=False) bc_filename = bc_file.name + buildfiles_temporary = True else: bi_filename = os.path.abspath(bi_filename) bc_filename = os.path.abspath(bc_filename) @@ -565,12 +567,12 @@ def main(apiurl, opts, argv): specfile=build_descr_data, addlist=extra_pkgs)) bi_file.write(bi_text) - bi_file.flush() + bi_file.close() print 'Getting buildconfig from server and store to %s' % bc_filename if not bc_file: bc_file = open(bc_filename, 'w') bc_file.write(get_buildconfig(apiurl, prj, repo)) - bc_file.flush() + bc_file.close() except urllib2.HTTPError, e: if e.code == 404: # check what caused the 404 @@ -788,10 +790,10 @@ def main(apiurl, opts, argv): rpmlist.append('cbignore: ' + ' '.join(bi.cbpreinstall_list) + '\n') rpmlist.append('runscripts: ' + ' '.join(bi.runscripts_list) + '\n') - rpmlist_file = NamedTemporaryFile(prefix='rpmlist.') + rpmlist_file = NamedTemporaryFile(prefix='rpmlist.', delete=False) rpmlist_filename = rpmlist_file.name rpmlist_file.writelines(rpmlist) - rpmlist_file.flush() + rpmlist_file.close() subst = { 'repo': repo, 'arch': arch, 'project' : prj, 'package' : pacname } vm_options = [] @@ -882,10 +884,9 @@ def main(apiurl, opts, argv): for i in b_built.splitlines() + s_built.splitlines(): shutil.copy2(i, os.path.join(opts.keep_pkgs, os.path.basename(i))) - if bi_file: - bi_file.close() - if bc_file: - bc_file.close() - rpmlist_file.close() + if buildfiles_temporary: + os.unlink(bi_filename) + os.unlink(bc_filename) + os.unlink(rpmlist_filename) # vim: sw=4 et -- 1.7.3.4 -- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org

On 2010-12-31 16:39:18 +0100, Michal Marek wrote:
There is no need for the whole build process to have the _buildinfo* and _buildconfig* files open.
Hmm with this patch it might be possible that the temporary file isn't unlinked (because you call NamedTemporaryFile with "delete=False"). For instance if an exception is raised before the "if buildfiles_temporary..." check is reached the tempfiles won't be unlinked (with "delete=True" the file will be removed whenever the _TemporaryFileWrapper instance is "deleted"). I need to think about it again (because I want to avoid a big try-finally block)...
Signed-off-by: Michal Marek <mmarek@suse.cz> ---
<SNIP>
@@ -882,10 +884,9 @@ def main(apiurl, opts, argv): for i in b_built.splitlines() + s_built.splitlines(): shutil.copy2(i, os.path.join(opts.keep_pkgs, os.path.basename(i)))
- if bi_file: - bi_file.close() - if bc_file: - bc_file.close() - rpmlist_file.close() + if buildfiles_temporary: + os.unlink(bi_filename) + os.unlink(bc_filename) + os.unlink(rpmlist_filename)
Marcus -- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org

On 2.1.2011 23:37, Marcus Hüwe wrote:
On 2010-12-31 16:39:18 +0100, Michal Marek wrote:
There is no need for the whole build process to have the _buildinfo* and _buildconfig* files open.
Hmm with this patch it might be possible that the temporary file isn't unlinked (because you call NamedTemporaryFile with "delete=False"). For instance if an exception is raised before the "if buildfiles_temporary..." check is reached the tempfiles won't be unlinked (with "delete=True" the file will be removed whenever the _TemporaryFileWrapper instance is "deleted"). I need to think about it again (because I want to avoid a big try-finally block)...
Thanks for the review. Would it be possible to subclass _TemporaryFileWrapper to only unlink the file if the object is destroyed and not when it is closed? Or is this an "internal" class, because it is underscore-prefixed? Or we can leave it as is, I just stumbled upon the open fds when debugging something unrelated during build. BTW, I find the default behavior of NamedTemporaryFile() rather odd, I would expect that open temporary file write data to it close it pass the filename to an external process delete the file would be a common pattern. Michal -- To unsubscribe, e-mail: opensuse-buildservice+unsubscribe@opensuse.org For additional commands, e-mail: opensuse-buildservice+help@opensuse.org
participants (2)
-
Marcus Hüwe
-
Michal Marek