Skip to content

bazel: adapt cc_wraper.py to python3#7519

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
rojkov:clearlinux-fixes
Jul 11, 2019
Merged

bazel: adapt cc_wraper.py to python3#7519
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
rojkov:clearlinux-fixes

Conversation

@rojkov
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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
Copy Markdown
Member Author

rojkov commented Jul 10, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

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

see: more, trace.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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