New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[deferred-sources] fix glob expansion issue in deferred sources mappe… #6824

Merged
merged 4 commits into from Nov 29, 2018

Conversation

Projects
None yet
4 participants
@baroquebobcat
Contributor

baroquebobcat commented Nov 28, 2018

…r task

Problem

When there is more than one sources_target to unpack, there is a bug where the deferred sources mapper raises this exception.

                     logs/exceptions.66766.log >>> Exception message: expected glob filespec: u'.pants.d/tmp/tmpfch67d.pants.d/unpack-jars/unpack-jars/.pants.d.tmp.tmpfch67d.pants.d.external-source/com/squareup/testing/protolib/external.proto' to start with its root path: u'.pants.d/tmp/tmpfch67d.pants.d/unpack-jars/unpack-jars/.pants.d.tmp.tmpfch67d.pants.d.other-external-source'!

This is because the last value of a local var set inside one loop is reused in a subsequent loop without resetting its value.

Solution

Hold onto the unpack dirs in the first loop and zip their values together with the appropriate target in the second.

Includes an adjustment to an integration test that causes it to encounter the failure.

@baroquebobcat baroquebobcat requested a review from illicitonion Nov 28, 2018

@stuhood

Thanks!

@wisechengyi

Thank you @baroquebobcat !

@illicitonion

Thanks!

@wisechengyi wisechengyi merged commit 792553f into pantsbuild:master Nov 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wisechengyi wisechengyi deleted the twitter:nhoward/fix_deferred_source_mapper branch Nov 29, 2018

wisechengyi added a commit that referenced this pull request Nov 29, 2018

[deferred-sources] fix glob expansion issue in deferred sources mappe… (
#6824)

### Problem

When there is more than one sources_target to unpack, there is a bug where the deferred sources mapper raises this exception.
```
                     logs/exceptions.66766.log >>> Exception message: expected glob filespec: u'.pants.d/tmp/tmpfch67d.pants.d/unpack-jars/unpack-jars/.pants.d.tmp.tmpfch67d.pants.d.external-source/com/squareup/testing/protolib/external.proto' to start with its root path: u'.pants.d/tmp/tmpfch67d.pants.d/unpack-jars/unpack-jars/.pants.d.tmp.tmpfch67d.pants.d.other-external-source'!
```

This is because the last value of a local var set inside one loop is reused in a subsequent loop without resetting its value.

### Solution

Hold onto the unpack dirs in the first loop and zip their values together with the appropriate target in the second.

Includes an adjustment to an integration test that causes it to encounter the failure.

cosmicexplorer added a commit that referenced this pull request Dec 3, 2018

don't copy over the os environment to avoid an encoding error (#6846)
### Problem

See https://pantsbuild.slack.com/archives/CB8F24LPM/p1543518771002700 for context -- if that link dies, the issue is: e.g. https://travis-ci.org/pantsbuild/pants/jobs/461402174 on `1.12.x` is failing, and this happens to occur in the native toolchain testing. This arises from an incorrectly encoded environment variable which was set somewhere else, I believe in the test harness somewhere. I gleaned that from this excerpt of the build output, where it errors out at trying to set an existing environment variable with `environment_as()`:
```
pants_test/backend/native/subsystems/test_native_toolchain.py:136: in _invoke_capturing_output
    with environment_as(**env):
/usr/lib/python2.7/contextlib.py:17: in __enter__
    return self.gen.next()
pants/util/contextutil.py:69: in environment_as
    setenv(key, val)
pants/util/contextutil.py:62: in setenv
    os.environ[key] = val if PY3 else _os_encode(val)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

u = '[deferred-sources] fix glob expansion issue in deferred sources mappe\xe2\x80\xa6 (#6824)\n\n### Problem\r\n\r\nWhen ...te target in the second.\r\n\r\nIncludes an adjustment to an integration test that causes it to encounter the failure.'
enc = 'UTF-8'

    def _os_encode(u, enc=sys.getfilesystemencoding()):
      """Turns a `unicode` into `bytes` via encoding."""
>     return u.encode(enc, 'strict')
E     UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 69: ordinal not in range(128)

pants/util/contextutil.py:41: UnicodeDecodeError
```

This appears to trigger nondeterministically. I wouldn't be surprised if the changes in #6824 ended up triggering this, but the real issue is that (it seems) some pants output is being stuffed into an environment variable, and that it happens to not be encoded correctly somehow. I'm not sure why this is the case, but if it is process output of some form, I might be able to see why this is nondeterministic.

So the underlying issue is twofold -- one that we're stuffing process output into an env var (which might be legitimate), and two that we aren't encoding the value of the env var we're setting correctly. For now, this commit allows us to avoid triggering the underlying issue.

### Solution

- Remove the unnecessary `environment_as()` call with `os.environ.copy()` in the native toolchain testing to avoid triggering the environment variable issue described above.

### Result

- Test no longer flakes in the `1.12.x` branch.

wisechengyi added a commit that referenced this pull request Dec 4, 2018

don't copy over the os environment to avoid an encoding error (#6846)
### Problem

See https://pantsbuild.slack.com/archives/CB8F24LPM/p1543518771002700 for context -- if that link dies, the issue is: e.g. https://travis-ci.org/pantsbuild/pants/jobs/461402174 on `1.12.x` is failing, and this happens to occur in the native toolchain testing. This arises from an incorrectly encoded environment variable which was set somewhere else, I believe in the test harness somewhere. I gleaned that from this excerpt of the build output, where it errors out at trying to set an existing environment variable with `environment_as()`:
```
pants_test/backend/native/subsystems/test_native_toolchain.py:136: in _invoke_capturing_output
    with environment_as(**env):
/usr/lib/python2.7/contextlib.py:17: in __enter__
    return self.gen.next()
pants/util/contextutil.py:69: in environment_as
    setenv(key, val)
pants/util/contextutil.py:62: in setenv
    os.environ[key] = val if PY3 else _os_encode(val)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

u = '[deferred-sources] fix glob expansion issue in deferred sources mappe\xe2\x80\xa6 (#6824)\n\n### Problem\r\n\r\nWhen ...te target in the second.\r\n\r\nIncludes an adjustment to an integration test that causes it to encounter the failure.'
enc = 'UTF-8'

    def _os_encode(u, enc=sys.getfilesystemencoding()):
      """Turns a `unicode` into `bytes` via encoding."""
>     return u.encode(enc, 'strict')
E     UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 69: ordinal not in range(128)

pants/util/contextutil.py:41: UnicodeDecodeError
```

This appears to trigger nondeterministically. I wouldn't be surprised if the changes in #6824 ended up triggering this, but the real issue is that (it seems) some pants output is being stuffed into an environment variable, and that it happens to not be encoded correctly somehow. I'm not sure why this is the case, but if it is process output of some form, I might be able to see why this is nondeterministic.

So the underlying issue is twofold -- one that we're stuffing process output into an env var (which might be legitimate), and two that we aren't encoding the value of the env var we're setting correctly. For now, this commit allows us to avoid triggering the underlying issue.

### Solution

- Remove the unnecessary `environment_as()` call with `os.environ.copy()` in the native toolchain testing to avoid triggering the environment variable issue described above.

### Result

- Test no longer flakes in the `1.12.x` branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment