[Notes] [Git][BuildStream/buildstream][bschubert/fix-atomic-move-git-repo] 4 commits: plugin.py: Redirect DEBUG from `__del__` to `sys.stderr`

[Benjamin Schubert <gitlab mg gitlab com>]

Title: GitLab

Benjamin Schubert pushed to branch bschubert/fix-atomic-move-git-repo at BuildStream / buildstream

Commits:

• 7738f6df
  by Daniel Silverstone at 2018-11-09T11:25:44Z

  plugin.py: Redirect DEBUG from `__del__` to `sys.stderr`
  
  Previously, debug messages from `Plugin.__del__()` went to stdout
  which was a problem for the test suite in extremely rare and hard to
  predict circumstances.  This corrects that by sending the message to
  stderr where all the rest of the messages tend to go.
  
  Signed-off-by: Daniel Silverstone <daniel silverstone codethink co uk>
  

• 62f59eaa
  by Valentin David at 2018-11-09T13:30:06Z

  Merge branch 'danielsilverstone-ct/plugin-destroy-debug-to-stderr' into 'master'
  
  plugin.py: Redirect DEBUG from `__del__` to `sys.stderr`
  
  See merge request BuildStream/buildstream!944

• 08accfde
  by Benjamin Schubert at 2018-11-09T13:57:16Z

  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
  

• b68f02c5
  by Benjamin Schubert at 2018-11-09T13:57:16Z

  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
  

5 changed files:

• buildstream/plugin.py
• buildstream/plugins/sources/git.py
• buildstream/utils.py
• tests/frontend/buildtrack.py
• + tests/utils/movedirectory.py

Changes:

buildstream/plugin.py

---

...	...	@@ -111,6 +111,7 @@ Class Reference
111	111
112	112	import os
113	113	import subprocess
	114	+import sys
114	115	from contextlib import contextmanager
115	116	from weakref import WeakValueDictionary
116	117
...	...	@@ -190,7 +191,7 @@ class Plugin():
190	191	# Dont send anything through the Message() pipeline at destruction time,
191	192	# any subsequent lookup of plugin by unique id would raise KeyError.
192	193	if self.__context.log_debug:
193		- print("DEBUG: Destroyed: {}".format(self))
	194	+ sys.stderr.write("DEBUG: Destroyed: {}\n".format(self))
194	195
195	196	def __str__(self):
196	197	return "{kind} {typetag} at {provenance}".format(

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/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.