Skip to content

Conversation

rojkov
Copy link
Member

@rojkov rojkov commented Jul 10, 2019

Description:
In Arch and Clear Linux /usr/bin/python points to python3 causing build failures due to type mismatch in os.write(): string is used where bytestring is expected.
Explicitly convert string to bytestring.

Risk Level: low
Testing: unit tests
Release Notes: N/A
Documentation: N/A

Dmitry Rozhkov added 2 commits July 10, 2019 14:53
Description:
In Arch and Clear linux /usr/bin/python points to python3 causing
build failures due to type mismatch in os.write(): string is used
where bytestring is expected.
Explicitly convert string to bytestring.

Risk Level: low
Testing: unit tests
Release Notes: N/A
Documentation: N/A

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
@alyssawilk
Copy link
Contributor

@envoyproxy/maintainers as an aside, given the number of things that had trouble I think we should be more careful tweaking our python library in future

@alyssawilk
Copy link
Contributor

And thanks for picking up this fix, @rojkov :-)

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Is there a way to force the file to be Python 3 compatible? To @alyssawilk point, I think that as we roll over files to Python 3, we need to ensure we don't regress, and machine checking is the way to go.

@rojkov
Copy link
Member Author

rojkov commented Jul 10, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7519 (comment) was created by @rojkov.

see: more, trace.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

@rojkov LGTM, but can you address my above comment?

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
@rojkov
Copy link
Member Author

rojkov commented Jul 11, 2019

@htuch I've updated the file's shebang. Or you want to enforce compatibility with both pythons?

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks; I'm hoping this doesn't break folks, I think it's reasonable to assume that most of the world has python3 these days, given python2 is deprecated.

@mattklein123 mattklein123 merged commit 74e3487 into envoyproxy:master Jul 11, 2019
@moderation
Copy link
Contributor

moderation commented Jul 11, 2019

This change breaks compiling on RHEL7. I do have python3 but in a non-standard place. After updating the shebang to point to python3 the compilation fails in spectacular ways. Will be interesting to see if this breaks those on Centos. /cc @htuch @rojkov

Edit - upon digging further I think that #7329 is responsible for the RHEL7 issues, not this PR /cc @lizan

@rojkov rojkov deleted the clearlinux-fixes branch July 12, 2019 07:20
@htuch
Copy link
Member

htuch commented Jul 12, 2019

@moderation would switching to #!/usr/bin/env python3 work?

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.

5 participants