New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix one or two bugs in trace.py #38807
Comments
|
There is one bug and one maybe-bug in creation of the The maybe-bug is that it doesn't attempt to create the Normally the directory will already exist, but if you The other bug is that it attempts to create a directory if not os.path.exists(dir):
os.makedirs(dir), which is a race condition that will very rarely raise The fix provided in this patch is to copy from my This patch hasn't been tested at ALL. In fact, I |
|
Zooko, is this patch still necessary? |
|
Hi! Sorry it took me so long to look at this. I just checked the Here is an updated version of the patch which simply removes some regards, Zooko diff -rN -u old-up/setuptools-0.6c7/ez_setup.py new-up/
setuptools-0.6c7/ez_setup.py
--- old-up/setuptools-0.6c7/ez_setup.py 2007-09-28 16:41:24.000000000
-0600
+++ new-up/setuptools-0.6c7/ez_setup.py 2007-09-28 16:41:25.000000000
-0600
@@ -1,4 +1,4 @@
-#!python
+#!/usr/bin/env python
"""Bootstrap setuptools installation
If you want to use setuptools in your package's setup.py, just
include this
@@ -13,7 +13,7 @@
This file can also be run as a script to install or upgrade setuptools.
"""
-import sys
+import os, re, subprocess, sys
DEFAULT_VERSION = "0.6c7"
DEFAULT_URL = "http://pypi.python.org/packages/%s/s/setuptools/"
% sys.version[:3]
@@ -44,8 +44,6 @@
'setuptools-0.6c6-py2.5.egg': 'b2f8a7520709a5b34f80946de5f02f53',
}
-import sys, os
-
def _validate_md5(egg_name, data):
if egg_name in md5_data:
from md5 import md5
@@ -58,6 +56,42 @@
sys.exit(2)
return data
+# The following code to parse versions is copied from
pkg_resources.py so that
+# we can parse versions without importing that module.
+component_re = re.compile(r'(\d+ | [a-z]+ | \.| -)', re.VERBOSE)
+replace = {'pre':'c',
'preview':'c','-':'final-','rc':'c','dev':'@'}.get
+
+def _parse_version_parts(s):
+ for part in component_re.split(s):
+ part = replace(part,part)
+ if not part or part=='.':
+ continue
+ if part[:1] in '0123456789':
+ yield part.zfill(8) # pad for numeric comparison
+ else:
+ yield '*'+part
+
+ yield '*final' # ensure that alpha/beta/candidate are before final
+
+def parse_version(s):
+ parts = []
+ for part in _parse_version_parts(s.lower()):
+ if part.startswith('*'):
+ if part<'*final': # remove '-' before a prerelease tag
+ while parts and parts[-1]=='*final-': parts.pop()
+ # remove trailing zeros from each series of numeric parts
+ while parts and parts[-1]=='00000000':
+ parts.pop()
+ parts.append(part)
+ return tuple(parts)
+
+def setuptools_is_new_enough(required_version):
+ """Return True if setuptools is already installed and has a version
+ number >= required_version."""
+ sub = subprocess.Popen([sys.executable, "-c", "import
setuptools;print setuptools.__version__"], stdout=subprocess.PIPE)
+ verstr = sub.stdout.read().strip()
+ ver = parse_version(verstr)
+ return ver and ver >= parse_version(required_version)
def use_setuptools(
version=DEFAULT_VERSION, download_base=DEFAULT_URL,
to_dir=os.curdir,
@@ -74,32 +108,11 @@
this routine will print a message to ``sys.stderr`` and raise
SystemExit in
an attempt to abort the calling script.
"""
- try:
- import setuptools
- if setuptools.__version__ == '0.0.1':
- print >>sys.stderr, (
- "You have an obsolete version of setuptools installed.
Please\n"
- "remove it from your system entirely before rerunning
this script."
- )
- sys.exit(2)
- except ImportError:
+ if not setuptools_is_new_enough(version):
egg = download_setuptools(version, download_base, to_dir,
download_delay)
sys.path.insert(0, egg)
import setuptools; setuptools.bootstrap_install_from = egg
- import pkg_resources
- try:
- pkg_resources.require("setuptools>="+version)
-
- except pkg_resources.VersionConflict, e:
- # XXX could we install in a subprocess here?
- print >>sys.stderr, (
- "The required version of setuptools (>=%s) is not
available, and\n"
- "can't be installed while this script is running. Please
install\n"
- " a more recent version first.\n\n(Currently using %r)"
- ) % (version, e.args[0])
- sys.exit(2)
-
def download_setuptools(
version=DEFAULT_VERSION, download_base=DEFAULT_URL,
to_dir=os.curdir,
delay = 15
@@ -150,9 +163,14 @@
def main(argv, version=DEFAULT_VERSION):
"""Install or upgrade setuptools and EasyInstall"""
- try:
- import setuptools
- except ImportError:
+ if setuptools_is_new_enough(version):
+ if argv:
+ from setuptools.command.easy_install import main
+ main(argv)
+ else:
+ print "Setuptools version",version,"or greater has been
installed."
+ print '(Run "ez_setup.py -U setuptools" to reinstall or
upgrade.)'
+ else:
egg = None
try:
egg = download_setuptools(version, delay=0)
@@ -162,31 +180,6 @@
finally:
if egg and os.path.exists(egg):
os.unlink(egg)
- else:
- if setuptools.__version__ == '0.0.1':
- # tell the user to uninstall obsolete version
- use_setuptools(version)
-
- req = "setuptools>="+version
- import pkg_resources
- try:
- pkg_resources.require(req)
- except pkg_resources.VersionConflict:
- try:
- from setuptools.command.easy_install import main
- except ImportError:
- from easy_install import main
- main(list(argv)+[download_setuptools(delay=0)])
- sys.exit(0) # try to force an exit
- else:
- if argv:
- from setuptools.command.easy_install import main
- main(argv)
- else:
- print "Setuptools version",version,"or greater has been
installed."
- print '(Run "ez_setup.py -U setuptools" to reinstall or
upgrade.)'
-
-
def update_md5(filenames):
"""Update our built-in md5 registry""" |
Oops, the in-lined patch contents were a different patch entirely, Just for completeness, here is the correct in-lined patch contents: Index: Lib/trace.py --- Lib/trace.py (revision 58282)
+++ Lib/trace.py (working copy)
@@ -85,7 +85,12 @@
-r, --report Generate a report from a counts file; do not
execute
any code. `--file' must specify the results
file to
read, which must have been created in a
previous run
- with `--count --file=FILE'.
+ with `--count --file=FILE'. If --coverdir is not
+ specified, the .cover files will be written
into the
+ directory that the modules were in when the
report was
+ generated. Whether or not --coverdir is
specified,
+ --report will always create the cover file
directory if
+ necessary.
Modifiers:
-f, --file=<file> File to accumulate counts over several runs.
@@ -197,6 +202,33 @@
filename, ext = os.path.splitext(base)
return filename
+# The following function is copied from the fileutil module from the
pyutil
+# project:
+# http://pypi.python.org/pypi/pyutil
+# We use this function instead of os.makedirs() so that we don't get a
+# spurious exception when someone else creates the directory at the
same
+# moment we do. (For example, another thread or process that is
also running
+# trace.)
+def make_dirs(dirname, mode=0777):
+ """
+ A threadsafe and idempotent version of os.makedirs(). If the
dir already
+ exists, do nothing and return without raising an exception. If
this call
+ creates the dir, return without raising an exception. If there
is an
+ error that prevents creation or if the directory gets deleted after
+ make_dirs() creates it and before make_dirs() checks that it
exists, raise
+ an exception.
+ """
+ tx = None
+ try:
+ os.makedirs(dirname, mode)
+ except OSError, x:
+ tx = x
+
+ if not os.path.isdir(dirname):
+ if tx:
+ raise tx
+ raise exceptions.IOError, "unknown error prevented creation
of directory, or deleted the directory immediately after creation: %
s" % dirname # careful not to construct an IOError with a 2-tuple, as
that has a special meaning...
+
class CoverageResults:
def __init__(self, counts=None, calledfuncs=None, infile=None,
callers=None, outfile=None):
@@ -290,15 +322,15 @@
if filename.endswith((".pyc", ".pyo")):
filename = filename[:-1]
- if coverdir is None:
+ if coverdir is not None:
+ dir = coverdir
+ modulename = fullmodname(filename)
+ else:
dir = os.path.dirname(os.path.abspath(filename))
modulename = modname(filename)
- else:
- dir = coverdir
- if not os.path.exists(dir):
- os.makedirs(dir)
- modulename = fullmodname(filename)
+ make_dirs(dir)
+
# If desired, get a list of the line numbers which
represent
# executable content (returned as a dict for better
lookup speed)
if show_missing: |
|
I would suggest that the need to create directories that may already |
|
unassigning |
|
Is there any interest in this issue? |
|
Eli, Would you like to review this patch? |
|
Alexander, I reviewed the patch and ported the changes to the newest sources (since the fix to bpo-9299, os.makedirs can be naturally used with its new flag to fix the bug Zooko refers to). However, while experimenting, I think I ran into much larger problems. Either that or I've forgotten how to use the module :-) Attaching two files (one imports the other) on which I try to run the following: python -m trace -c trace_target.py
However, now running: python -m trace -r --file=trace_target.cover
Also, trying to provide --file to -c: python -m trace -c trace_target.py --file=xyz.cover
Can you take a look at this? |
|
On Sat, Dec 4, 2010 at 3:34 AM, Eli Bendersky <report@bugs.python.org> wrote:
I am afraid it is the latter. :-) The file specified in --file option $ ./python.exe -m trace -s -f counts.pickle -c trace_target.py
K is 380
Skipping counts file 'counts.pickle': [Errno 2] No such file or
directory: 'counts.pickle'
lines cov% module (path)
1 100% trace (/Users/sasha/Work/python-svn/py3k-commit/Lib/trace.py)
9 100% trace_target (trace_target.py)
6 100% traced_module (traced_module.py)
$ ./python.exe -m pickletools counts.pickle
0: ( MARK
1: } EMPTY_DICT
2: q BINPUT 0
4: ( MARK
5: ( MARK
6: X BINUNICODE 'trace_target.py'
...However, there is a problem here, I think: $ ./python.exe -m trace -s -f counts.pickle -c trace_target.py
K is 380
lines cov% module (path)
1 100% trace (/Users/sasha/Work/python-svn/py3k-commit/Lib/trace.py)
9 100% trace_target (trace_target.py)
6 100% traced_module (traced_module.py)
$ ./python.exe -m trace -s -f counts.pickle -c trace_target.py
K is 380
lines cov% module (path)
1 100% trace (/Users/sasha/Work/python-svn/py3k-commit/Lib/trace.py)
9 100% trace_target (trace_target.py)
6 100% traced_module (traced_module.py)The counts should grow in repeated runs. |
|
On Wed, Jan 12, 2011 at 12:17 PM, Alexander Belopolsky
I did not pay attention: the numbers in summary are numbers of lines, |
|
On Sat, Dec 4, 2010 at 3:34 AM, Eli Bendersky <report@bugs.python.org> wrote:
Eli, I don't think you ever posted an updated patch. Do you still have it? This may be a good starting issue for you as a committer. |
|
Just a gentle bump. |
|
Ah, no time, no time... :-/ I may get back to this in the future. Bumping to more relevant versions for now. |
|
Hoi, was poking around the codebase looking to learn something, if I follow the conversation correctly, is this the patch which is being talked about for this issue? diff --git a/Lib/trace.py b/Lib/trace.py
index fb9a423ea0..0e20d3ab73 100755
--- a/Lib/trace.py
+++ b/Lib/trace.py
@@ -258,9 +258,8 @@ def write_results(self, show_missing=True, summary=False, coverdir=None):
modulename = _modname(filename)
else:
dir = coverdir
- if not os.path.exists(dir):
- os.makedirs(dir)
modulename = _fullmodname(filename)
+ os.makedirs(dir, exist_ok=True)
# If desired, get a list of the line numbers which represent
# executable content (returned as a dict for better lookup speed) |
|
TLDR at the bottom, some history at the top. Original Issues (2003) #38807:
Race Condition in os.makedirs (2007) #46016:
Superseeder (2010) #53545:
Vulnerability fix (2014) #65281:
benjaminp:
So yeah there is still a problem lurking here, but I am not quite sure if it is Issue #25583: Avoid incorrect errors raised by
|
Instead of checking if a directory does not exist and thereafter creating it, directly call `os.makedirs` with the `exist_ok` kwarg.
Instead of checking if a directory does not exist and thereafter creating it, directly call `os.makedirs` with the `exist_ok` kwarg.
Instead of checking if a directory does not exist and thereafter creating it, directly call `os.makedirs` with the `exist_ok` kwarg.


Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: