Skip to content
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

Decode python_eval template resource as utf-8. #6379

Merged
merged 2 commits into from
Aug 22, 2018

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Aug 22, 2018

Without this, the template was rendered with literal \n's under
python3. I did not linger to figure out how the rendered string could
possibly load in a python interpreter with no errors!

Fixes #6354
Fixes #6384

Without this, the template was rendered with literal `\n`'s under
python3. I did not linger to figure out how the rendered string could
possibly load in a python interpreter with no errors!

Fixes pantsbuild#6354
@jsirois
Copy link
Contributor Author

jsirois commented Aug 22, 2018

/cc @Eric-Arellano

@jsirois jsirois requested a review from stuhood August 22, 2018 01:50
@jsirois
Copy link
Contributor Author

jsirois commented Aug 22, 2018

NB: The error described in #6354 reproduced 100% before this fix and now does not occur.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find. I've been bitten by pkgutil a couple of other times.

In general, it looks like pkgutil.get_data leads to extremely silent errors, i.e. it doesn't raise errors but results in undesirable behavior.

I have no idea how you debugged this, but thank you so much for finding the issue!

This means contrib CI will be 100% py3 compliant :)

@Eric-Arellano
Copy link
Contributor

The Python 3 CI failures look quite similar to #6384, in that the actual output has more values than expected and in both instances the tests are called test_foo_incremental().

Copy link
Member

@mateor mateor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow

@jsirois
Copy link
Contributor Author

jsirois commented Aug 22, 2018

@Eric-Arellano

The Python 3 CI failures look quite similar to #6384, ...

Got this sussed. Fix forthcoming. I'll probably lump the fix into this RB for expediency but push back if you want it broken out.

Previously it wrote and read keys asymmetrically; now the key is
uniformly represented (and compared!) as a unicode utf-8 string in
memory.
@@ -223,7 +223,7 @@ def _read_sha(self, cache_key):
def _read_sha_by_id(self, id):
try:
with open(self._sha_file_by_id(id), 'rb') as fd:
return fd.read().strip()
return fd.read().strip().decode('utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preferred to open in mode 'r', and then you can avoid the decode. Don't worry about fixing and repushing though - https://github.com/pantsbuild/pants/pull/6372/files#diff-0bfcef172d3faad2ba6f85c7330adae2R225 will fix it.

Thanks again John for your help here!

@jsirois jsirois merged commit e6cefed into pantsbuild:master Aug 22, 2018
@jsirois jsirois deleted the issues/6354 branch August 22, 2018 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants