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 - fix issues with engine #6279

Merged
merged 15 commits into from Aug 1, 2018

Conversation

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

Eric-Arellano commented Jul 30, 2018

@illicitonion's patch shared via Slack allows us to run the project in Python 3 and fixes the Rust ImportError. However, when doing this, a bunch of new problems pop up related to the core engine and base code.

This PR solves a lot of those problems. I was not able to get a fully green run of ./pants test tests/python/pants_test/invalidation:cache_manager yet using Python 3, but stopped myself and am submitting this PR because it's getting too big to reason about.

@@ -107,7 +105,7 @@ def _log_exception(self, msg):
exception_log.write('timestamp: {}\n'.format(datetime.datetime.now().isoformat()))
exception_log.write('args: {}\n'.format(sys.argv))
exception_log.write('pid: {}\n'.format(os.getpid()))
exception_log.write(msg.encode('utf-8'))
exception_log.write(msg)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

The file output_path, which == work_dir/logs/exceptions.log, is opened in text mode.

I don't think it matters much if we choose to open it in binary vs text, it seems like a small contained change, and either works.

@@ -294,6 +294,8 @@ def bootstrap_c_source(output_dir, module_name=NATIVE_ENGINE_MODULE):
temp_output_prefix = os.path.join(tempdir, module_name)
real_output_prefix = os.path.join(output_dir, module_name)
temp_c_file = '{}.c'.format(temp_output_prefix)
if PY2:
temp_c_file = temp_c_file.encode('utf-8')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

ffibuilder.emit_c_code expects unicode in Py3, bytes in Py2.

return isinstance(obj, Serializable) or (not inspect.isclass(obj) and hasattr(obj, '_asdict'))
if inspect.isclass(obj):
return Serializable.is_serializable_type(obj)
return isinstance(obj, Serializable) or hasattr(obj, '_asdict')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

Fix to issue discussed here. Sometimes the passed value isn't a class, and in Py3 it complains about this whereas in Py2 it silently works.

@@ -317,7 +317,7 @@ def handle_output(self, workunit, label, s):
output_files[path] = f
else:
f = output_files[path]
f.write(self._htmlify_text(s).encode('utf-8'))
f.write(self._htmlify_text(s))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

This reporter code is worth a sanity check. It seemed to be expecting unicode in most places, as Py3 yells about all the original uses of bytes.

Using unicode works when testing locally for me, semi-tested because when this code was failing the output of Pants was unformatted until fixing this.

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

If you're able to actually invoke pants with python 3, you can confirm that clicking around in ./pants server --open looks reasonable.

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

But yea, HTML shouldn't contain any binary data, so this seems reasonable.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 30, 2018

Contributor

I won't be able to test that yet, unfortunately, because ./pants does not yet fully compile with Python 3 virtual env. This PR fixes a lot of the issues, but not all of them because it was getting to be too big.

Fortunately, the changes we're making with all of these PRs are innocuous for now—so long as they don't break Python 2—because we haven't yet turned on Python 3 for anything. So, if we realize in a few days this change was a mistake, we can easily revert back.

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -317,7 +317,7 @@ def handle_output(self, workunit, label, s):
output_files[path] = f
else:
f = output_files[path]
f.write(self._htmlify_text(s).encode('utf-8'))
f.write(self._htmlify_text(s))

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

If you're able to actually invoke pants with python 3, you can confirm that clicking around in ./pants server --open looks reasonable.

@@ -317,7 +317,7 @@ def handle_output(self, workunit, label, s):
output_files[path] = f
else:
f = output_files[path]
f.write(self._htmlify_text(s).encode('utf-8'))
f.write(self._htmlify_text(s))

This comment has been minimized.

@stuhood

stuhood Jul 30, 2018

Member

But yea, HTML shouldn't contain any binary data, so this seems reasonable.

Eric-Arellano added some commits Jul 30, 2018

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Jul 30, 2018

Reverting the Reporting changes and will break into separate PR. I'm having a hard time figuring out what's wrong and reproducing locally.

Also think the reporting changes are substantial enough to be separate PR for sake of git blame etc.

cp = configparser.SafeConfigParser()
cp.readfp(StringIO(self.DEFAULT_COVERAGE_CONFIG))
cp = configparser.ConfigParser()
cp.read_file(StringIO(self.DEFAULT_COVERAGE_CONFIG))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 1, 2018

Contributor

Both of the originals were deprecated.

@stuhood stuhood merged commit f189af8 into pantsbuild:master Aug 1, 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_rust-import-error branch Aug 1, 2018

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

Python 3 fixes - fix issues with engine (pantsbuild#6279)
@illicitonion's patch shared via Slack allows us to run the project in Python 3 and fixes the Rust ImportError. However, when doing this, a bunch of new problems pop up related to the core engine and base code. 

This PR solves a lot of those problems. I was not able to get a fully green run of `./pants test tests/python/pants_test/invalidation:cache_manager` yet using Python 3, but stopped myself and am submitting this PR because it's getting too big to reason about.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment