[Notes] [Git][BuildStream/buildstream][bschubert/fix-atomic-move-git-repo] 7 commits: Fix bug with root mounted as non-artifact in script plugin.

[Benjamin Schubert <gitlab mg gitlab com>]

Title: GitLab

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

Commits:

• cc2e6ae5
  by Valentin David at 2018-11-08T12:41:36Z

  Fix bug with root mounted as non-artifact in script plugin.
  
  The issue was introduced by 6ccfab0b1b25990e406446d5cbe5aee83a5e158a.
  

• e578a89f
  by Valentin David at 2018-11-08T13:11:10Z

  Merge branch 'valentindavid/fix-script-install-root' into 'master'
  
  Fix bug with root mounted as non-artifact in script plugin.
  
  Closes #756 and #749
  
  See merge request BuildStream/buildstream!941

• c0a8bb66
  by Angelos Evripiotis at 2018-11-08T15:49:16Z

  contributing: WIP again to be kind to reviewers
  
  As someone coming from GitHub to GitLab, I was pleasantly surprised by
  the 'filter by WIP status' option.
  
  To make sure we get the most out of it, add a guideline to keep the
  filter clean, to reduce reviewer burden.
  

• f7231e90
  by Angelos Evripiotis at 2018-11-08T15:49:16Z

  contributing: non-WIP should always be landable
  
  I was recently surprised that we don't prefer addressing review comments
  in 'fixup!' commits.
  
  Coming from GitHub, I've found that fixup commits make it easier for
  reviewers to see what has changed since their last review. The idea is
  to use '--autosquash' before landing to clean up the history again.
  
  It's a pleasant surprise that it's easy to keep track of what changed in
  merge-requests between pushes, so we can always keep the history clean.
  
  Document this, so folks like me can see the light sooner.
  

• f7643440
  by Angelos Evripiotis at 2018-11-08T15:49:16Z

  contributing: emphasise the 'why' in commits
  
  Explain why it's useful to mention the decisions made for a change and
  provide links for background, e.g. issue numbers.
  
  My general experience of git histories is that folks don't do enough
  explaining, so it's great we have a contributing section for it.
  
  I've found it's easier for folks to stick to rules when they know the
  practical reasons for them, so provide some.
  

• dd5e7b04
  by Angelos Evripiotis at 2018-11-08T16:17:04Z

  Merge branch 'aevri/contributing_gitlab' into 'master'
  
  Add more to GitLab-relevant parts of contributing
  
  See merge request BuildStream/buildstream!935

• 7cb37389
  by Benjamin Schubert at 2018-11-09T09:58:07Z

  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
  

10 changed files:

• CONTRIBUTING.rst
• buildstream/plugins/sources/git.py
• buildstream/scriptelement.py
• tests/frontend/buildtrack.py
• + tests/integration/project/elements/script/corruption-2.bst
• + tests/integration/project/elements/script/marked-tmpdir.bst
• + tests/integration/project/elements/script/no-tmpdir.bst
• + tests/integration/project/elements/script/tmpdir.bst
• tests/integration/script.py
• tests/sources/git.py

Changes:

CONTRIBUTING.rst

---

...	...	@@ -97,7 +97,13 @@ a new merge request. You can also `create a merge request for an existing branch
97	97	You may open merge requests for the branches you create before you are ready
98	98	to have them reviewed and considered for inclusion if you like. Until your merge
99	99	request is ready for review, the merge request title must be prefixed with the
100		-``WIP:`` identifier.
	100	+``WIP:`` identifier. GitLab `treats this specially
	101	+<https://docs.gitlab.com/ee/user/project/merge_requests/work_in_progress_merge_requests.html>`_,
	102	+which helps reviewers.
	103	+
	104	+Consider marking a merge request as WIP again if you are taking a while to
	105	+address a review point. This signals that the next action is on you, and it
	106	+won't appear in a reviewer's search for non-WIP merge requests to review.
101	107
102	108
103	109	Organized commits
...	...	@@ -122,6 +128,12 @@ If a commit in your branch modifies behavior such that a test must also
122	128	be changed to match the new behavior, then the tests should be updated
123	129	with the same commit, so that every commit passes its own tests.
124	130
	131	+These principles apply whenever a branch is non-WIP. So for example, don't push
	132	+'fixup!' commits when addressing review comments, instead amend the commits
	133	+directly before pushing. GitLab has `good support
	134	+<https://docs.gitlab.com/ee/user/project/merge_requests/versions.html>`_ for
	135	+diffing between pushes, so 'fixup!' commits are not necessary for reviewers.
	136	+
125	137
126	138	Commit messages
127	139	~~~~~~~~~~~~~~~
...	...	@@ -144,6 +156,16 @@ number must be referenced in the commit message.
144	156
145	157	Fixes #123
146	158
	159	+Note that the 'why' of a change is as important as the 'what'.
	160	+
	161	+When reviewing this, folks can suggest better alternatives when they know the
	162	+'why'. Perhaps there are other ways to avoid an error when things are not
	163	+frobnicated.
	164	+
	165	+When folks modify this code, there may be uncertainty around whether the foos
	166	+should always be frobnicated. The comments, the commit message, and issue #123
	167	+should shed some light on that.
	168	+
147	169	In the case that you have a commit which necessarily modifies multiple
148	170	components, then the summary line should still mention generally what
149	171	changed (if possible), followed by a colon and a brief summary.

buildstream/plugins/sources/git.py

---

...	...	@@ -141,21 +141,24 @@ 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		- try:
147		- os.rename(tmpdir, self.mirror)
148		- 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
	144	+ self._atomic_move_mirror(tmpdir, url)
	145	+
	146	+ def _atomic_move_mirror(self, tmpdir, url):
	147	+ # Attempt atomic rename into destination, this will fail if
	148	+ # another process beat us to the punch
	149	+ try:
	150	+ os.rename(tmpdir, self.mirror)
	151	+ except OSError as e:
	152	+ # When renaming and the destination repo already exists, os.rename()
	153	+ # will fail with either ENOTEMPTY or EEXIST, depending on the underlying
	154	+ # implementation.
	155	+ # An empty directory would always be replaced.
	156	+ if e.errno in (errno.EEXIST, errno.ENOTEMPTY):
	157	+ self.source.status("{}: Discarding duplicate clone of {}"
	158	+ .format(self.source, url))
	159	+ else:
	160	+ raise SourceError("{}: Failed to move cloned git repository {} from '{}' to '{}': {}"
	161	+ .format(self.source, url, tmpdir, self.mirror, e)) from e
159	162
160	163	def _fetch(self, alias_override=None):
161	164	url = self.source.translate_url(self.url,

buildstream/scriptelement.py

---

...	...	@@ -202,7 +202,7 @@ class ScriptElement(Element):
202	202	sandbox.set_environment(self.get_environment())
203	203
204	204	# Tell the sandbox to mount the install root
205		- directories = {'/': False}
	205	+ directories = {self.__install_root: False}
206	206
207	207	# Mark the artifact directories in the layout
208	208	for item in self.__layout:
...	...	@@ -211,7 +211,10 @@ class ScriptElement(Element):
211	211	directories[destination] = item['element'] or was_artifact
212	212
213	213	for directory, artifact in directories.items():
214		- sandbox.mark_directory(directory, artifact=artifact)
	214	+ # Root does not need to be marked as it is always mounted
	215	+ # with artifact (unless explicitly marked non-artifact)
	216	+ if directory != '/':
	217	+ sandbox.mark_directory(directory, artifact=artifact)
215	218
216	219	def stage(self, sandbox):
217	220

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/integration/project/elements/script/corruption-2.bst

---

	1	+kind: script
	2	+
	3	+depends:
	4	+- filename: base.bst
	5	+ type: build
	6	+- filename: script/corruption-image.bst
	7	+ type: build
	8	+
	9	+config:
	10	+ commands:
	11	+ - echo smashed >>/canary

tests/integration/project/elements/script/marked-tmpdir.bst

---

	1	+kind: compose
	2	+
	3	+depends:
	4	+- filename: base.bst
	5	+ type: build
	6	+
	7	+public:
	8	+ bst:
	9	+ split-rules:
	10	+ remove:
	11	+ - "/tmp/**"
	12	+ - "/tmp"

tests/integration/project/elements/script/no-tmpdir.bst

---

	1	+kind: filter
	2	+
	3	+depends:
	4	+- filename: script/marked-tmpdir.bst
	5	+ type: build
	6	+
	7	+config:
	8	+ exclude:
	9	+ - remove
	10	+ include-orphans: True
	11	+
	12	+

tests/integration/project/elements/script/tmpdir.bst

---

	1	+kind: script
	2	+
	3	+depends:
	4	+- filename: script/no-tmpdir.bst
	5	+ type: build
	6	+
	7	+config:
	8	+ commands:
	9	+ - |
	10	+ mkdir -p /tmp/blah

tests/integration/script.py

---

...	...	@@ -184,3 +184,41 @@ def test_regression_cache_corruption(cli, tmpdir, datafiles):
184	184
185	185	with open(os.path.join(checkout_after, 'canary')) as f:
186	186	assert f.read() == 'alive\n'
	187	+
	188	+
	189	+@pytest.mark.datafiles(DATA_DIR)
	190	+def test_regression_tmpdir(cli, tmpdir, datafiles):
	191	+ project = str(datafiles)
	192	+ element_name = 'script/tmpdir.bst'
	193	+
	194	+ res = cli.run(project=project, args=['build', element_name])
	195	+ assert res.exit_code == 0
	196	+
	197	+
	198	+@pytest.mark.datafiles(DATA_DIR)
	199	+def test_regression_cache_corruption_2(cli, tmpdir, datafiles):
	200	+ project = str(datafiles)
	201	+ checkout_original = os.path.join(cli.directory, 'checkout-original')
	202	+ checkout_after = os.path.join(cli.directory, 'checkout-after')
	203	+ element_name = 'script/corruption-2.bst'
	204	+ canary_element_name = 'script/corruption-image.bst'
	205	+
	206	+ res = cli.run(project=project, args=['build', canary_element_name])
	207	+ assert res.exit_code == 0
	208	+
	209	+ res = cli.run(project=project, args=['checkout', canary_element_name,
	210	+ checkout_original])
	211	+ assert res.exit_code == 0
	212	+
	213	+ with open(os.path.join(checkout_original, 'canary')) as f:
	214	+ assert f.read() == 'alive\n'
	215	+
	216	+ res = cli.run(project=project, args=['build', element_name])
	217	+ assert res.exit_code == 0
	218	+
	219	+ res = cli.run(project=project, args=['checkout', canary_element_name,
	220	+ checkout_after])
	221	+ assert res.exit_code == 0
	222	+
	223	+ with open(os.path.join(checkout_after, 'canary')) as f:
	224	+ assert f.read() == 'alive\n'

tests/sources/git.py

---

...	...	@@ -21,11 +21,14 @@
21	21	#
22	22
23	23	import os
	24	+from unittest import mock
24	25	import pytest
	26	+import py.path
25	27
26	28	from buildstream._exceptions import ErrorDomain
27		-from buildstream import _yaml
	29	+from buildstream import _yaml, SourceError
28	30	from buildstream.plugin import CoreWarnings
	31	+from buildstream.plugins.sources.git import GitMirror
29	32
30	33	from tests.testutils import cli, create_repo
31	34	from tests.testutils.site import HAVE_GIT
...	...	@@ -36,6 +39,64 @@ DATA_DIR = os.path.join(
36	39	)
37	40
38	41
	42	+@pytest.mark.parametrize(
	43	+ ["type_", "setup"],
	44	+ [
	45	+ ("inexistent-dir", lambda path: None),
	46	+ ("empty-dir", lambda path: path.mkdir()),
	47	+ ("non-empty-dir", lambda path: path.join("bad").write("hi", ensure=True)),
	48	+ ("file", lambda path: path.write("no")),
	49	+ ],
	50	+)
	51	+def test_git_mirror_atomic_move(tmpdir, type_, setup):
	52	+ # Create required elements
	53	+ class DummySource:
	54	+ def get_mirror_directory(self):
	55	+ return str(tmpdir.join("source").mkdir())
	56	+
	57	+ status = mock.MagicMock()
	58	+
	59	+ url = "file://{}/url".format(tmpdir)
	60	+
	61	+ # Create fake download directory
	62	+ download_dir = tmpdir.join("download_dir")
	63	+ download_dir.join("oracle").write("hello", ensure=True)
	64	+
	65	+ # Initiate mirror
	66	+ mirror = GitMirror(DummySource(), None, url, None)
	67	+
	68	+ # Setup destination directory
	69	+ setup(py.path.local(mirror.mirror))
	70	+
	71	+ # Copy directory content
	72	+ if type_ == "file":
	73	+ with pytest.raises(SourceError):
	74	+ mirror._atomic_move_mirror(str(download_dir), mirror.url)
	75	+ else:
	76	+ mirror._atomic_move_mirror(str(download_dir), mirror.url)
	77	+
	78	+ # Validate
	79	+ if type_ == "non-empty-dir":
	80	+ # With a non empty directory, we should not have overriden
	81	+ # and notified a status
	82	+ assert DummySource.status.called
	83	+
	84	+ expected_oracle = os.path.join(mirror.mirror, "bad")
	85	+ expected_content = "hi"
	86	+ elif type_ == "file":
	87	+ expected_oracle = mirror.mirror
	88	+ expected_content = "no"
	89	+ else:
	90	+ # Otherwise we should have a new directory and the data
	91	+ expected_oracle = os.path.join(mirror.mirror, "oracle")
	92	+ expected_content = "hello"
	93	+
	94	+ assert os.path.exists(expected_oracle)
	95	+
	96	+ with open(expected_oracle) as fp:
	97	+ assert fp.read() == expected_content
	98	+
	99	+
39	100	@pytest.mark.skipif(HAVE_GIT is False, reason="git is not available")
40	101	@pytest.mark.datafiles(os.path.join(DATA_DIR, 'template'))
41	102	def test_fetch_bad_ref(cli, tmpdir, datafiles):

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