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

Python 3 fixes - add open backport to contrib #6295

Merged
merged 17 commits into from Aug 7, 2018

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Aug 3, 2018

Ensures these folders use Python 3 semantics for opening files:

  • build-support/bin
  • contrib/avro/
  • contrib/buildgen/
  • contrib/buildrefactor/
  • contrib/codeanlysis/
  • contrib/confluence/
  • contrib/cpp/
  • contrib/errorprone/
  • contrib/findbugs/
  • contrib/go/
  • contrib/googlejavaformat/
  • contrib/jax_ws/
  • contrib/mypy/
  • contrib/node/
  • contrib/python/
  • contrib/scalajs/
  • contrib/scrooge/
  • contrib/thrifty/
  • examples/python/
  • migrations/
  • pants-plugins/
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 3, 2018

I'm confused why changes made to contrib are causing a failure in Linux Native Engine Builder. The error looks meaningful. https://travis-ci.org/pantsbuild/pants/jobs/411761210

Is any of the source code calling contrib code? If so, what folder? The stack trace doesn't mention contrib anywhere.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 3, 2018

I'm confused why changes made to contrib are causing a failure in Linux Native Engine Builder. The error looks meaningful. https://travis-ci.org/pantsbuild/pants/jobs/411761210

'ascii' codec can't decode byte 0xe2 in position 154973: ordinal not in range(128)

So... I think that that is probably in the release notes, which pants_setup_py parses, which would be affected by the pants-plugins directory.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 3, 2018

This is pretty wild. Removing a layer of wrapping from the exception, I see:

Traceback (most recent call last):
  File "/travis/workdir/src/python/pants/engine/native.py", line 476, in extern_generator_send
    res = c.from_value(func[0]).send(c.from_value(arg[0]))
  File "/travis/workdir/src/python/pants/engine/build_files.py", line 62, in parse_address_family
    address_mapper.parser))
  File "/travis/workdir/src/python/pants/engine/mapper.py", line 53, in parse
    objects = parser.parse(filepath, filecontent)
  File "/travis/workdir/src/python/pants/engine/legacy/parser.py", line 138, in parse
    six.exec_(python, dict(self._symbols))
  File "/travis/workdir/build-support/pants_dev_deps.venv/lib/python2.7/site-packages/six.py", line 709, in exec_
    exec("""exec _code_ in _globs_, _locs_""")
  File "<string>", line 1, in <module>
  File "<string>", line 20, in <module>
  File "/travis/workdir/pants-plugins/src/python/internal_backend/utilities/register.py", line 53, in pants_setup_py
    notes = PantsReleases.global_instance().notes_for_version(PANTS_SEMVER)
  File "/travis/workdir/pants-plugins/src/python/internal_backend/utilities/register.py", line 134, in notes_for_version
    return _read_contents(branch_notes_file)
  File "/travis/workdir/pants-plugins/src/python/internal_backend/utilities/register.py", line 23, in _read_contents
    return fp.read()
  File "/travis/workdir/build-support/pants_dev_deps.venv/lib64/python2.7/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 157026: ordinal not in range(128)

Which indicates that in that shard on travis, the default encoding might be different? docker is involved, so that's my next guess. But if that's the case, then... oof.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 4, 2018

Hm, yea... confirmed. It looks like the default encoding on that shard is ANSI_X3.4-1968 (via locale.getpreferredencoding()), which means that open results in a stream that decodes ~ascii: see https://stackoverflow.com/a/36316351.

We have code that is supposed to detect this here: https://github.com/pantsbuild/pants/blob/master/src/python/pants/bin/pants_loader.py#L31-L45 , but I think it's buggy, and should probably just try to confirm that locale.getpreferredencoding is UTF-8.

Alternatively, we could... set the encoding on all of the non-binary open calls.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 4, 2018

Wow that’s crazy, great job finding this.

Do you know the breakdown of default encoding for Pants users? Do we have anyone who doesn’t use UTF-8 that we want to support? I’m afraid it would be extremely difficult to reason about other encodings so advocate for modifying the check to guarantee if possible.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Aug 4, 2018

@Eric-Arellano : Opened #6305. To land it, I'll likely have to rebuild that docker image.

stuhood added a commit that referenced this pull request Aug 6, 2018

Require the system encoding to be UTF-8 (#6305)
### Problem

As seen in #6295, python 3's `open` call defaults to decoding from `locale.getpreferredencoding()` when opened in non-binary mode, which can cause odd misconfigurations to be detected late in the game.

One solution to that problem would be to explicitly specify encodings on all `open` calls where the encoding is known ahead of time, but that game of whack a mole is hard to win, since libraries might also open files indirectly.

### Solution

Require a preferred encoding of `UTF-8`, and address a few locations where pants' own CI was not providing it.

### Result

If folks are using other encodings for valid reasons, hopefully they will file issues; if not, they will have a workaround.

stuhood added a commit that referenced this pull request Aug 6, 2018

Python 3 fixes - add open() backport to safe_open() (#6304)
Final stage of adding Python 3 open semantics. Builds off of #6291, #6295, and #6290.

@illicitonion illicitonion merged commit b8cf29d into pantsbuild:master Aug 7, 2018

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-fixes_open-contrib branch Aug 7, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Require the system encoding to be UTF-8 (pantsbuild#6305)
### Problem

As seen in pantsbuild#6295, python 3's `open` call defaults to decoding from `locale.getpreferredencoding()` when opened in non-binary mode, which can cause odd misconfigurations to be detected late in the game.

One solution to that problem would be to explicitly specify encodings on all `open` calls where the encoding is known ahead of time, but that game of whack a mole is hard to win, since libraries might also open files indirectly.

### Solution

Require a preferred encoding of `UTF-8`, and address a few locations where pants' own CI was not providing it.

### Result

If folks are using other encodings for valid reasons, hopefully they will file issues; if not, they will have a workaround.

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment