From 14af6b429ef2355ba0e8342c4af0f9dd9aafab1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20G=C3=B3rny?= Date: Thu, 7 Dec 2017 23:15:05 +0100 Subject: [PATCH 1/6] Use ensuredir() instead of os.makedirs() to fix race conditions Use the ensuredir() function consistently across Sphinx code to avoid race conditions e.g. when multiple Sphinx instances attempt to create the same output directory. The ensuredir() function that was already present in sphinx.util.osutil correctly catches EEXIST exception that occurs if the specified directory already exists (i.e. it was created between the call to os.path.isdir() and os.makedirs() that follows it). While at it, remove redundant os.path.isdir() calls when they only guarded the os.makedirs() call, and replace mkdir_p() which had pretty much the same purpose, except for being prone to race conditions. I did not modify testing-related code as race conditions mostly affect real applications and not the test environment. Fix #4281: Race conditions when creating output directory --- CHANGES | 1 + sphinx/apidoc.py | 7 +++---- sphinx/application.py | 4 ++-- sphinx/builders/__init__.py | 6 ++---- sphinx/quickstart.py | 19 ++++++------------- 5 files changed, 14 insertions(+), 23 deletions(-) diff --git a/CHANGES b/CHANGES index b7a16af4f..1b8289784 100644 --- a/CHANGES +++ b/CHANGES @@ -33,6 +33,7 @@ Bugs fixed * #4279: Sphinx crashes with pickling error when run with multiple processes and remote image * #1421: Respect the quiet flag in sphinx-quickstart +* #4281: Race conditions when creating output directory Testing -------- diff --git a/sphinx/apidoc.py b/sphinx/apidoc.py index 24ed874b0..b764cfc35 100644 --- a/sphinx/apidoc.py +++ b/sphinx/apidoc.py @@ -26,7 +26,7 @@ from fnmatch import fnmatch from sphinx import __display_version__ from sphinx.quickstart import EXTENSIONS from sphinx.util import rst -from sphinx.util.osutil import FileAvoidWrite, walk +from sphinx.util.osutil import FileAvoidWrite, ensuredir, walk if False: # For type annotation @@ -375,9 +375,8 @@ Note: By default this script will not overwrite already created files.""") if not path.isdir(rootpath): print('%s is not a directory.' % rootpath, file=sys.stderr) sys.exit(1) - if not path.isdir(opts.destdir): - if not opts.dryrun: - os.makedirs(opts.destdir) + if not opts.dryrun: + ensuredir(opts.destdir) rootpath = path.abspath(rootpath) excludes = normalize_excludes(rootpath, excludes) modules = recurse_tree(rootpath, excludes, opts) diff --git a/sphinx/application.py b/sphinx/application.py index 0d6d3d0b3..b5705face 100644 --- a/sphinx/application.py +++ b/sphinx/application.py @@ -41,7 +41,7 @@ from sphinx.util import import_object from sphinx.util import logging from sphinx.util import status_iterator, old_status_iterator, display_chunk from sphinx.util.tags import Tags -from sphinx.util.osutil import ENOENT +from sphinx.util.osutil import ENOENT, ensuredir from sphinx.util.console import bold, darkgreen # type: ignore from sphinx.util.docutils import is_html5_writer_available, directive_helper from sphinx.util.i18n import find_catalog_source_files @@ -160,7 +160,7 @@ class Sphinx(object): if not path.isdir(outdir): logger.info('making output directory...') - os.makedirs(outdir) + ensuredir(outdir) # read config self.tags = Tags(tags) diff --git a/sphinx/builders/__init__.py b/sphinx/builders/__init__.py index 007964e82..a1d5c5d22 100644 --- a/sphinx/builders/__init__.py +++ b/sphinx/builders/__init__.py @@ -9,7 +9,6 @@ :license: BSD, see LICENSE for details. """ -import os from os import path import warnings @@ -24,7 +23,7 @@ from docutils import nodes from sphinx.deprecation import RemovedInSphinx20Warning from sphinx.environment.adapters.asset import ImageAdapter from sphinx.util import i18n, path_stabilize, logging, status_iterator -from sphinx.util.osutil import SEP, relative_uri +from sphinx.util.osutil import SEP, ensuredir, relative_uri from sphinx.util.i18n import find_catalog from sphinx.util.console import bold # type: ignore from sphinx.util.parallel import ParallelTasks, SerialTasks, make_chunks, \ @@ -79,8 +78,7 @@ class Builder(object): self.confdir = app.confdir self.outdir = app.outdir self.doctreedir = app.doctreedir - if not path.isdir(self.doctreedir): - os.makedirs(self.doctreedir) + ensuredir(self.doctreedir) self.app = app # type: Sphinx self.env = None # type: BuildEnvironment diff --git a/sphinx/quickstart.py b/sphinx/quickstart.py index d23dc3b74..5d8738996 100644 --- a/sphinx/quickstart.py +++ b/sphinx/quickstart.py @@ -35,7 +35,7 @@ from six.moves.urllib.parse import quote as urlquote from docutils.utils import column_width from sphinx import __display_version__, package_dir -from sphinx.util.osutil import make_filename +from sphinx.util.osutil import ensuredir, make_filename from sphinx.util.console import ( # type: ignore purple, bold, red, turquoise, nocolor, color_terminal ) @@ -69,13 +69,6 @@ EXTENSIONS = ('autodoc', 'doctest', 'intersphinx', 'todo', 'coverage', PROMPT_PREFIX = '> ' -def mkdir_p(dir): - # type: (unicode) -> None - if path.isdir(dir): - return - os.makedirs(dir) - - # function to get input from terminal -- overridden by the test suite def term_input(prompt): # type: (unicode) -> unicode @@ -433,11 +426,11 @@ def generate(d, overwrite=True, silent=False, templatedir=None): d[key + '_str'] = d[key].replace('\\', '\\\\').replace("'", "\\'") if not path.isdir(d['path']): - mkdir_p(d['path']) + ensuredir(d['path']) srcdir = d['sep'] and path.join(d['path'], 'source') or d['path'] - mkdir_p(srcdir) + ensuredir(srcdir) if d['sep']: builddir = path.join(d['path'], 'build') d['exclude_patterns'] = '' @@ -448,9 +441,9 @@ def generate(d, overwrite=True, silent=False, templatedir=None): 'Thumbs.db', '.DS_Store', ]) d['exclude_patterns'] = ', '.join(exclude_patterns) - mkdir_p(builddir) - mkdir_p(path.join(srcdir, d['dot'] + 'templates')) - mkdir_p(path.join(srcdir, d['dot'] + 'static')) + ensuredir(builddir) + ensuredir(path.join(srcdir, d['dot'] + 'templates')) + ensuredir(path.join(srcdir, d['dot'] + 'static')) def write_file(fpath, content, newline=None): # type: (unicode, unicode, unicode) -> None From 070fbb97e5a8f41a398ac5de16938f2da5797a5a Mon Sep 17 00:00:00 2001 From: jfbu Date: Sat, 16 Dec 2017 11:05:33 +0100 Subject: [PATCH 2/6] Add markup and clarify numfig_secnum_depth docs --- doc/config.rst | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/doc/config.rst b/doc/config.rst index 7a6d20147..3b490c5d3 100644 --- a/doc/config.rst +++ b/doc/config.rst @@ -311,8 +311,8 @@ General configuration .. confval:: numfig If true, figures, tables and code-blocks are automatically numbered if they - have a caption. At same time, the `numref` role is enabled. For now, it - works only with the HTML builder and LaTeX builder. Default is ``False``. + have a caption. The :rst:role:`numref` role is enabled. + Obeyed so far only by HTML and LaTeX builders. Default is ``False``. .. note:: @@ -335,10 +335,12 @@ General configuration .. confval:: numfig_secnum_depth - The scope of figure numbers, that is, the numfig feature numbers figures - in which scope. ``0`` means "whole document". ``1`` means "in a section". - Sphinx numbers like x.1, x.2, x.3... ``2`` means "in a subsection". Sphinx - numbers like x.x.1, x.x.2, x.x.3..., and so on. Default is ``1``. + The scope for numbering: ``0`` means "continuous numbering", + ``1`` means "reset per section" + (i.e. numbers will be x.1, x.2, x.3 ... with x the section number), + ``2`` means "per subsection" + (i.e. numbers will be x.y.1, x.y.2, ...), + and so on. Default is ``1``. .. versionadded:: 1.3 From f780f92d1b0eb1c3f6b37dc1e75e0980d046b134 Mon Sep 17 00:00:00 2001 From: jfbu Date: Sat, 16 Dec 2017 11:32:40 +0100 Subject: [PATCH 3/6] Mention ``:numbered:`` needed in numfig_secnum_depth docs --- doc/config.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/config.rst b/doc/config.rst index 3b490c5d3..fc87c4ddc 100644 --- a/doc/config.rst +++ b/doc/config.rst @@ -337,7 +337,9 @@ General configuration The scope for numbering: ``0`` means "continuous numbering", ``1`` means "reset per section" - (i.e. numbers will be x.1, x.2, x.3 ... with x the section number), + (i.e. numbers will be x.1, x.2, x.3 ... with x the section number, + assuming that the :rst:dir:`toctree` directive was used with its option + ``:numbered:``), ``2`` means "per subsection" (i.e. numbers will be x.y.1, x.y.2, ...), and so on. Default is ``1``. From a9602e4597a75889edc91e65f226c72a5ae73d0e Mon Sep 17 00:00:00 2001 From: jfbu Date: Sat, 16 Dec 2017 11:46:33 +0100 Subject: [PATCH 4/6] Improve numref docs --- doc/markup/inline.rst | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/markup/inline.rst b/doc/markup/inline.rst index a59585bab..09ea49003 100644 --- a/doc/markup/inline.rst +++ b/doc/markup/inline.rst @@ -222,15 +222,15 @@ Cross-referencing figures by figure number reST labels are used. When you use this role, it will insert a reference to the figure with link text by its figure number like "Fig. 1.1". - If an explicit link text is given (like usual: ``:numref:`Image of Sphinx (Fig. - %s) ```), the link caption will be the title of the reference. - As a special character, `%s` and `{number}` will be replaced to figure - number. `{name}` will be replaced to figure caption. - If no explicit link text is given, the value of :confval:`numfig_format` is - used to default value of link text. + If an explicit link text is given (as usual: ``:numref:`Image of Sphinx (Fig. + %s) ```), the link caption will serve as title of the reference. + As place holders, `%s` and `{number}` get replaced by the figure + number and `{name}` by the figure caption. + If no explicit link text is given, the :confval:`numfig_format` setting is + used as fall-back default. - If :confval:`numfig` is ``False``, figures are not numbered. - so this role inserts not a reference but labels or link text. + If :confval:`numfig` is ``False``, figures are not numbered, + so this role inserts not a reference but the label or the link text. Cross-referencing other items of interest ----------------------------------------- From ba1368072fdfc143322a38335ebcdd8d83c6f720 Mon Sep 17 00:00:00 2001 From: jfbu Date: Sat, 16 Dec 2017 11:51:00 +0100 Subject: [PATCH 5/6] Typo place holder --> placeholder --- doc/markup/inline.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/markup/inline.rst b/doc/markup/inline.rst index 09ea49003..7ed3e7207 100644 --- a/doc/markup/inline.rst +++ b/doc/markup/inline.rst @@ -224,7 +224,7 @@ Cross-referencing figures by figure number If an explicit link text is given (as usual: ``:numref:`Image of Sphinx (Fig. %s) ```), the link caption will serve as title of the reference. - As place holders, `%s` and `{number}` get replaced by the figure + As placeholders, `%s` and `{number}` get replaced by the figure number and `{name}` by the figure caption. If no explicit link text is given, the :confval:`numfig_format` setting is used as fall-back default. From 46b18962283d4ed7cab4e87a737ab9001d6933aa Mon Sep 17 00:00:00 2001 From: jfbu Date: Sun, 17 Dec 2017 17:56:03 +0100 Subject: [PATCH 6/6] Again rewording docs about numfig_secnum_depth --- doc/config.rst | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/doc/config.rst b/doc/config.rst index fc87c4ddc..8f4656060 100644 --- a/doc/config.rst +++ b/doc/config.rst @@ -335,14 +335,21 @@ General configuration .. confval:: numfig_secnum_depth - The scope for numbering: ``0`` means "continuous numbering", - ``1`` means "reset per section" - (i.e. numbers will be x.1, x.2, x.3 ... with x the section number, - assuming that the :rst:dir:`toctree` directive was used with its option - ``:numbered:``), - ``2`` means "per subsection" - (i.e. numbers will be x.y.1, x.y.2, ...), - and so on. Default is ``1``. + - if set to ``0``, figures, tables and code-blocks are continuously numbered + starting at ``1``. + - if ``1`` (default) numbers will be ``x.1``, ``x.2``, ... with ``x`` + the section number (top level sectioning; no ``x.`` if no section). + This naturally applies only if section numbering has been activated via + the ``:numbered:`` option of the :rst:dir:`toctree` directive. + - ``2`` means that numbers will be ``x.y.1``, ``x.y.2``, ... if located in + a sub-section (but still ``x.1``, ``x.2``, ... if located directly under a + section and ``1``, ``2``, ... if not in any top level section.) + - etc... + + .. note:: + + The LaTeX builder currently ignores this configuration setting. It will + obey it at Sphinx 1.7. .. versionadded:: 1.3