This is an archive of the discontinued LLVM Phabricator instance.

Differential D55258

Python 2/3 compat - socket server
ClosedPublic

Authored by serge-sans-paille on Dec 4 2018, 2:19 AM.

Details

Reviewers
joerg
michaelplatings
• ddunbar
gbedwell
Commits
rG040ee58795e1: Portable Python script across Python version
rC349010: Portable Python script across Python version
rL349010: Portable Python script across Python version

Diff Detail

Event Timeline

serge-sans-paille created this revision.Dec 4 2018, 2:19 AM
serge-sans-paille added a reviewer: • ddunbar.Dec 5 2018, 6:38 AM
tstellar added a subscriber: tstellar.Dec 5 2018, 9:34 PM

Is there a particular reason why this and https://reviews.llvm.org/D55259 are separate patches?

serge-sans-paille added a comment.Dec 5 2018, 10:43 PM

Is there a particular reason why this and https://reviews.llvm.org/D55259 are separate patches?

The Python 2/3 portability is a huge patch. I have split it in several micro-patch, one for each package/feature impacted. The underlying idea is that we can keep the discussions focus. This package should just be a renaming issue, while https://reviews.llvm.org/D55259 uses a higher level API than the one before.

gbedwell accepted this revision.Dec 12 2018, 6:27 AM
gbedwell added a subscriber: gbedwell.

LGTM with one minor nit.

tools/scan-view/bin/scan-view
90

Minor nit. Elsewhere (E.g. D55202 and D55200), the pattern:

try:
    import py3name
except ImportError:
    import py2name as py3name

is used, so for consistency I'd suggest doing the same here (it also has the nice property of implying that Python 3 should be the 'real' version).

This revision is now accepted and ready to land.Dec 12 2018, 6:27 AM
Closed by commit rL349010: Portable Python script across Python version (authored by serge_sans_paille). · Explain WhyDec 12 2018, 11:48 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: llvm-commits. · View Herald TranscriptDec 12 2018, 11:49 PM
serge-sans-paille added a comment.Dec 13 2018, 12:14 AM

Thanks @gbedwell for the review o/

Revision Contents

PathSize
tools/
scan-view/
bin/
7 lines

Diff 176572

tools/scan-view/bin/scan-view

Context not available.
def port_is_open(port): def port_is_open(port):
import SocketServer
try: try:
t = SocketServer.TCPServer((kDefaultHost, port), None) import SocketServer as socketserver
except ImportError:
import socketserver
gbedwellUnsubmitted
Not Done
ReplyInline Actions

Minor nit. Elsewhere (E.g. D55202 and D55200), the pattern:

try:
    import py3name
except ImportError:
    import py2name as py3name

is used, so for consistency I'd suggest doing the same here (it also has the nice property of implying that Python 3 should be the 'real' version).

gbedwell: Minor nit. Elsewhere (E.g. D55202 and D55200), the pattern: ``` try: import py3name…
try:
t = socketserver.TCPServer((kDefaultHost, port), None)
except: except:
return False return False
t.server_close() t.server_close()
Context not available.