[Notes] [Git][BuildStream/buildstream][master] 6 commits: tests/frontend/buildtrack.py: check for success after invocation

[Benjamin Schubert <gitlab mg gitlab com>]

Title: GitLab

Benjamin Schubert pushed to branch master at BuildStream / buildstream

Commits:

• f23b6031
  by Benjamin Schubert at 2018-11-19T11:39:51Z

  tests/frontend/buildtrack.py: check for success after invocation
  
  Previously the result of the first call in test_build_track was not
  checked to be successful, which would lead to potential erros later on
  that were hard to debug.
  
  Let's check it everytime
  

• a6defc0b
  by Benjamin Schubert at 2018-11-19T11:39:51Z

  Fix os.rename in git source element to correctly handle error codes
  
  According to the documentation
  (https://www.unix.com/man-page/POSIX/3posix/rename/), when the directory
  already is there, either EEXIST or ENOTEMPTY could be thrown.
  
  Previously only ENOTEMPTY was checked.
  
  Done:
    - Separated the move into its own function
    - Check for both errors
    - Create unit tests for it, covering most test cases
  

• 88089d2d
  by Benjamin Schubert at 2018-11-19T11:39:51Z

  Extract atomic move function to utils.py
  
  Moving atomically a file/directory can be tricky since different
  errors might be raised for the same underlying problem.
  
  Having a utility function to reduce this discrepancies will help
  in ensuring we have correct behavior
  

• fd9e46be
  by Benjamin Schubert at 2018-11-19T11:39:51Z

  cascache.py: use move_atomic instead of manual error checking
  

• d32e0b83
  by Benjamin Schubert at 2018-11-19T11:39:51Z

  sources/pip.py: Use move_atomic instead of manual os.rename
  
  This uses move_atomic insteand of the manual os.rename and manual error
  checking and throws a SourceError for consistency with other modules.
  

• 6f837118
  by Benjamin Schubert at 2018-11-19T12:22:40Z

  Merge branch 'bschubert/fix-atomic-move-git-repo' into 'master'
  
  Fix os.rename in git source element to correctly handle error codes
  
  See merge request BuildStream/buildstream!938

6 changed files:

• buildstream/_artifactcache/cascache.py
• buildstream/plugins/sources/git.py
• buildstream/plugins/sources/pip.py
• buildstream/utils.py
• tests/frontend/buildtrack.py
• + tests/utils/movedirectory.py

Changes:

buildstream/_artifactcache/cascache.py

---

...	...	@@ -24,7 +24,6 @@ import os
24	24	import stat
25	25	import tempfile
26	26	import uuid
27		-import errno
28	27	from urllib.parse import urlparse
29	28
30	29	import grpc
...	...	@@ -140,17 +139,13 @@ class CASCache():
140	139	checkoutdir = os.path.join(tmpdir, ref)
141	140	self._checkout(checkoutdir, tree)
142	141
143		- os.makedirs(os.path.dirname(dest), exist_ok=True)
144	142	try:
145		- os.rename(checkoutdir, dest)
	143	+ utils.move_atomic(checkoutdir, dest)
	144	+ except utils.DirectoryExistsError:
	145	+ # Another process beat us to rename
	146	+ pass
146	147	except OSError as e:
147		- # With rename it's possible to get either ENOTEMPTY or EEXIST
148		- # in the case that the destination path is a not empty directory.
149		- #
150		- # If rename fails with these errors, another process beat
151		- # us to it so just ignore.
152		- if e.errno not in [errno.ENOTEMPTY, errno.EEXIST]:
153		- raise CASError("Failed to extract directory for ref '{}': {}".format(ref, e)) from e
	148	+ raise CASError("Failed to extract directory for ref '{}': {}".format(ref, e)) from e
154	149
155	150	return originaldest
156	151

buildstream/plugins/sources/git.py

---

...	...	@@ -86,7 +86,6 @@ This plugin also utilises the following configurable core plugin warnings:
86	86	"""
87	87
88	88	import os
89		-import errno
90	89	import re
91	90	import shutil
92	91	from collections.abc import Mapping
...	...	@@ -97,6 +96,7 @@ from configparser import RawConfigParser
97	96	from buildstream import Source, SourceError, Consistency, SourceFetcher
98	97	from buildstream import utils
99	98	from buildstream.plugin import CoreWarnings
	99	+from buildstream.utils import move_atomic, DirectoryExistsError
100	100
101	101	GIT_MODULES = '.gitmodules'
102	102
...	...	@@ -141,21 +141,16 @@ class GitMirror(SourceFetcher):
141	141	fail="Failed to clone git repository {}".format(url),
142	142	fail_temporarily=True)
143	143
144		- # Attempt atomic rename into destination, this will fail if
145		- # another process beat us to the punch
146	144	try:
147		- os.rename(tmpdir, self.mirror)
	145	+ move_atomic(tmpdir, self.mirror)
	146	+ except DirectoryExistsError:
	147	+ # Another process was quicker to download this repository.
	148	+ # Let's discard our own
	149	+ self.source.status("{}: Discarding duplicate clone of {}"
	150	+ .format(self.source, url))
148	151	except OSError as e:
149		-
150		- # When renaming and the destination repo already exists, os.rename()
151		- # will fail with ENOTEMPTY, since an empty directory will be silently
152		- # replaced
153		- if e.errno == errno.ENOTEMPTY:
154		- self.source.status("{}: Discarding duplicate clone of {}"
155		- .format(self.source, url))
156		- else:
157		- raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
158		- .format(self.source, url, tmpdir, self.mirror, e)) from e
	152	+ raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
	153	+ .format(self.source, url, tmpdir, self.mirror, e)) from e
159	154
160	155	def _fetch(self, alias_override=None):
161	156	url = self.source.translate_url(self.url,

buildstream/plugins/sources/pip.py

---

...	...	@@ -68,7 +68,6 @@ details on common configuration options for sources.
68	68	The ``pip`` plugin is available since :ref:`format version 16 <project_format_version>`
69	69	"""
70	70
71		-import errno
72	71	import hashlib
73	72	import os
74	73	import re
...	...	@@ -193,13 +192,14 @@ class PipSource(Source):
193	192	# process has fetched the sources before us and ensure that we do
194	193	# not raise an error in that case.
195	194	try:
196		- os.makedirs(self._mirror)
197		- os.rename(package_dir, self._mirror)
198		- except FileExistsError:
199		- return
	195	+ utils.move_atomic(package_dir, self._mirror)
	196	+ except utils.DirectoryExistsError:
	197	+ # Another process has beaten us and has fetched the sources
	198	+ # before us.
	199	+ pass
200	200	except OSError as e:
201		- if e.errno != errno.ENOTEMPTY:
202		- raise
	201	+ raise SourceError("{}: Failed to move downloaded pip packages from '{}' to '{}': {}"
	202	+ .format(self, package_dir, self._mirror, e)) from e
203	203
204	204	def stage(self, directory):
205	205	with self.timed_activity("Staging Python packages", silent_nested=True):

buildstream/utils.py

---

...	...	@@ -72,6 +72,11 @@ class ProgramNotFoundError(BstError):
72	72	super().__init__(message, domain=ErrorDomain.PROG_NOT_FOUND, reason=reason)
73	73
74	74
	75	+class DirectoryExistsError(OSError):
	76	+ """Raised when a `os.rename` is attempted but the destination is an existing directory.
	77	+ """
	78	+
	79	+
75	80	class FileListResult():
76	81	"""An object which stores the result of one of the operations
77	82	which run on a list of files.
...	...	@@ -500,6 +505,38 @@ def get_bst_version():
500	505	.format(__version__))
501	506
502	507
	508	+def move_atomic(source, destination, ensure_parents=True):
	509	+ """Move the source to the destination using atomic primitives.
	510	+
	511	+ This uses `os.rename` to move a file or directory to a new destination.
	512	+ It wraps some `OSError` thrown errors to ensure their handling is correct.
	513	+
	514	+ The main reason for this to exist is that rename can throw different errors
	515	+ for the same symptom (https://www.unix.com/man-page/POSIX/3posix/rename/).
	516	+
	517	+ We are especially interested here in the case when the destination already
	518	+ exists. In this case, either EEXIST or ENOTEMPTY are thrown.
	519	+
	520	+ In order to ensure consistent handling of these exceptions, this function
	521	+ should be used instead of `os.rename`
	522	+
	523	+ Args:
	524	+ source (str or Path): source to rename
	525	+ destination (str or Path): destination to which to move the source
	526	+ ensure_parents (bool): Whether or not to create the parent's directories
	527	+ of the destination (default: True)
	528	+ """
	529	+ if ensure_parents:
	530	+ os.makedirs(os.path.dirname(str(destination)), exist_ok=True)
	531	+
	532	+ try:
	533	+ os.rename(str(source), str(destination))
	534	+ except OSError as exc:
	535	+ if exc.errno in (errno.EEXIST, errno.ENOTEMPTY):
	536	+ raise DirectoryExistsError(*exc.args) from exc
	537	+ raise
	538	+
	539	+
503	540	@contextmanager
504	541	def save_file_atomic(filename, mode='w', *, buffering=-1, encoding=None,
505	542	errors=None, newline=None, closefd=True, opener=None, tempdir=None):

tests/frontend/buildtrack.py

---

...	...	@@ -115,6 +115,7 @@ def test_build_track(cli, datafiles, tmpdir, ref_storage,
115	115	args += ['0.bst']
116	116
117	117	result = cli.run(project=project, silent=True, args=args)
	118	+ result.assert_success()
118	119	tracked_elements = result.get_tracked_elements()
119	120
120	121	assert set(tracked_elements) == set(tracked)

tests/utils/movedirectory.py

---

	1	+import pytest
	2	+
	3	+from buildstream.utils import move_atomic, DirectoryExistsError
	4	+
	5	+
	6	+@pytest.fixture
	7	+def src(tmp_path):
	8	+ src = tmp_path.joinpath("src")
	9	+ src.mkdir()
	10	+
	11	+ with src.joinpath("test").open("w") as fp:
	12	+ fp.write("test")
	13	+
	14	+ return src
	15	+
	16	+
	17	+def test_move_to_empty_dir(src, tmp_path):
	18	+ dst = tmp_path.joinpath("dst")
	19	+
	20	+ move_atomic(src, dst)
	21	+
	22	+ assert dst.joinpath("test").exists()
	23	+
	24	+
	25	+def test_move_to_empty_dir_create_parents(src, tmp_path):
	26	+ dst = tmp_path.joinpath("nested/dst")
	27	+
	28	+ move_atomic(src, dst)
	29	+ assert dst.joinpath("test").exists()
	30	+
	31	+
	32	+def test_move_to_empty_dir_no_create_parents(src, tmp_path):
	33	+ dst = tmp_path.joinpath("nested/dst")
	34	+
	35	+ with pytest.raises(FileNotFoundError):
	36	+ move_atomic(src, dst, ensure_parents=False)
	37	+
	38	+
	39	+def test_move_non_existing_dir(tmp_path):
	40	+ dst = tmp_path.joinpath("dst")
	41	+ src = tmp_path.joinpath("src")
	42	+
	43	+ with pytest.raises(FileNotFoundError):
	44	+ move_atomic(src, dst)
	45	+
	46	+
	47	+def test_move_to_existing_empty_dir(src, tmp_path):
	48	+ dst = tmp_path.joinpath("dst")
	49	+ dst.mkdir()
	50	+
	51	+ move_atomic(src, dst)
	52	+ assert dst.joinpath("test").exists()
	53	+
	54	+
	55	+def test_move_to_existing_file(src, tmp_path):
	56	+ dst = tmp_path.joinpath("dst")
	57	+
	58	+ with dst.open("w") as fp:
	59	+ fp.write("error")
	60	+
	61	+ with pytest.raises(NotADirectoryError):
	62	+ move_atomic(src, dst)
	63	+
	64	+
	65	+def test_move_file_to_existing_file(tmp_path):
	66	+ dst = tmp_path.joinpath("dst")
	67	+ src = tmp_path.joinpath("src")
	68	+
	69	+ with src.open("w") as fp:
	70	+ fp.write("src")
	71	+
	72	+ with dst.open("w") as fp:
	73	+ fp.write("dst")
	74	+
	75	+ move_atomic(src, dst)
	76	+ with dst.open() as fp:
	77	+ assert fp.read() == "src"
	78	+
	79	+
	80	+def test_move_to_existing_non_empty_dir(src, tmp_path):
	81	+ dst = tmp_path.joinpath("dst")
	82	+ dst.mkdir()
	83	+
	84	+ with dst.joinpath("existing").open("w") as fp:
	85	+ fp.write("already there")
	86	+
	87	+ with pytest.raises(DirectoryExistsError):
	88	+ move_atomic(src, dst)

—
View it on GitLab.
You're receiving this email because of your account on gitlab.com. If you'd like to receive fewer emails, you can adjust your notification settings.