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

Add support for Python3.8 #6439

Merged
merged 3 commits into from Jan 30, 2020
Merged

Add support for Python3.8 #6439

merged 3 commits into from Jan 30, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Jan 29, 2020

Cherry-pick changesets:


Changelog: Bugfix: Fix broken compression of .tgz files due to Python 3.8 changing tar default schema.
Changelog: Fix: Recipe substitution for scm (old behavior) fixed for multiline comments in Python 3.8
Docs: omit

I omit the docs, with this release we don't want to remove python 3.8 warnings from the docs

#tags: slow, svn
#revisions: 1

* fixing some broken tests for python 3.8

* fix test for AST replacement

* fixing missing error msg in server

Co-authored-by: James <james@conan.io>
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jan 29, 2020

Rationale for 3452db2: lutris/lutris#2518

bufsize=1 is only valid for text mode. The default changed in version 3.3.1: bufsize now defaults to -1 to enable buffering by default to match the behavior that most code expects (source).

Python 3.8 throws a warning when doing I/O in binary mode (source). This causes the message below to appear when applications are launched through lutris:

     /usr/lib/python3.8/subprocess.py:844: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
    self.stdout = io.open(c2pread, 'rb', bufsize)


def _execute(command):
proc = Popen(command, shell=True, bufsize=1, stdout=PIPE, stderr=STDOUT)
# In Python 3.8, open() emits RuntimeWarning if buffering=1 for binary mode.
bufsize = -1 if isPY38 else 1
Copy link
Member

@memsharded memsharded Jan 30, 2020

Choose a reason for hiding this comment

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

The solution adopted in develop is:

proc = subprocess.Popen(command, shell=True, bufsize=1, universal_newlines=True,
                            stdout=subprocess.PIPE, stderr=subprocess.STDOUT)

Maybe use it instead of bufsize?

Copy link
Member

@memsharded memsharded Jan 30, 2020

Choose a reason for hiding this comment

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

Oh, I didn't see the above. No problem.

Copy link
Member Author

@jgsogo jgsogo Jan 30, 2020

Choose a reason for hiding this comment

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

I saw it in develop, but it is the same line with a bufsize=1 and I don't know why that one is not failing... so it should be the place where we are running this function from.

The only thing different is the universal_newlines, but I wouldn't expect it to make a difference related to the buffering 🤷‍♂

Copy link
Member

@memsharded memsharded Jan 30, 2020

Choose a reason for hiding this comment

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

Is the universal_newlines=True, that fixes it

Copy link
Member Author

@jgsogo jgsogo Jan 30, 2020

Choose a reason for hiding this comment

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

IMO, this is a patch affecting only py38 (which is not officially supported for v1.21.x), it let us pass the testsuite without modifying our CI. We may investigate it to find the difference between this branch and develop, but to be honest, we have come across this because of this patch release, otherwise we would have kept working without noticing it, and this change won't affect develop...

@jgsogo jgsogo merged commit 0960636 into conan-io:release/1.21.2 Jan 30, 2020
1 of 2 checks passed
@jgsogo jgsogo deleted the release/py38 branch Jan 30, 2020
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.

None yet

2 participants