[Notes] [Git][BuildStream/buildstream][bschubert/fix-pip-python] 7 commi

Javier Jardón pushed to branch bschubert/fix-pip-python 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

427174cc

by Benjamin Schubert at 2018-11-08T18:08:54Z

plugins/sources/pip.py: also look for python version named "python"

In some virtual environments, we might not have the binary with the
exact python version, which would lead the current pip plugin to
fail to find a working pip version.

This adds "python" at the start of the list of valid python versions

8 changed files:

CONTRIBUTING.rst
buildstream/plugins/sources/pip.py
buildstream/scriptelement.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

Changes:

CONTRIBUTING.rst

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

buildstream/plugins/sources/pip.py

@@ -80,6 +80,7 @@ _PYPI_INDEX_URL = 'https://pypi.org/simple/'
  # Used only for finding pip command
  _PYTHON_VERSIONS = [
 +    'python',  # when running in a venv, we might not have the exact version
      'python2.7',
      'python3.0',
      'python3.1',

buildstream/scriptelement.py

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

tests/integration/project/elements/script/corruption-2.bst

 +kind: script
++
 +depends:
 +- filename: base.bst
 +  type: build
 +- filename: script/corruption-image.bst
 +  type: build
++
 +config:
 +  commands:
 +  - echo smashed >>/canary

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

 +kind: compose
++
 +depends:
 +- filename: base.bst
 +  type: build
++
 +public:
 +  bst:
 +    split-rules:
 +      remove:
 +        - "/tmp/**"
 +        - "/tmp"

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

 +kind: filter
++
 +depends:
 +- filename: script/marked-tmpdir.bst
 +  type: build
++
 +config:
 +  exclude:
 +  - remove
 +  include-orphans: True
++
++

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

 +kind: script
++
 +depends:
 +- filename: script/no-tmpdir.bst
 +  type: build
++
 +config:
 +  commands:
 +  - |
 +    mkdir -p /tmp/blah

tests/integration/script.py

@@ -184,3 +184,41 @@ def test_regression_cache_corruption(cli, tmpdir, datafiles):
      with open(os.path.join(checkout_after, 'canary')) as f:
          assert f.read() == 'alive\n'
++
++
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_regression_tmpdir(cli, tmpdir, datafiles):
 +    project = str(datafiles)
 +    element_name = 'script/tmpdir.bst'
++
 +    res = cli.run(project=project, args=['build', element_name])
 +    assert res.exit_code == 0
++
++
 +@pytest.mark.datafiles(DATA_DIR)
 +def test_regression_cache_corruption_2(cli, tmpdir, datafiles):
 +    project = str(datafiles)
 +    checkout_original = os.path.join(cli.directory, 'checkout-original')
 +    checkout_after = os.path.join(cli.directory, 'checkout-after')
 +    element_name = 'script/corruption-2.bst'
 +    canary_element_name = 'script/corruption-image.bst'
++
 +    res = cli.run(project=project, args=['build', canary_element_name])
 +    assert res.exit_code == 0
++
 +    res = cli.run(project=project, args=['checkout', canary_element_name,
 +                                         checkout_original])
 +    assert res.exit_code == 0
++
 +    with open(os.path.join(checkout_original, 'canary')) as f:
 +        assert f.read() == 'alive\n'
++
 +    res = cli.run(project=project, args=['build', element_name])
 +    assert res.exit_code == 0
++
 +    res = cli.run(project=project, args=['checkout', canary_element_name,
 +                                         checkout_after])
 +    assert res.exit_code == 0
++
 +    with open(os.path.join(checkout_after, 'canary')) as f:
 +        assert f.read() == 'alive\n'

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

Javier Jardón pushed to branch bschubert/fix-pip-python at BuildStream / buildstream

Commits:

8 changed files:

Changes: