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 py3.8 patches from pyside 5.14 #62

Merged
merged 7 commits into from Jan 13, 2020

Conversation

looooo
Copy link
Contributor

@looooo looooo commented Jan 12, 2020

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jschueller
Copy link
Contributor

Nice, I tried with 5.12.x but the patches did not apply.
Which commits did you backport ? did it apply cleanly ? If the apply cleanly its best to retain the git messages. Else it would be good to mention which commits the patches are from.

@looooo
Copy link
Contributor Author

looooo commented Jan 12, 2020

I used these two commits:

commit 095c6437152aca85174662cdb78cfd583c03f5e4
Author: Christian Tismer <tismer@stackless.com>
Date:   Thu Dec 5 11:08:50 2019 +0100

    Optimize the Python 3.8 refcount fix a tiny bit
    
    This change uses the fact that our workaround to temporarily remove
    the Py_TPFLAGS_METHOD_DESCRIPTOR flag uses the "mro" function
    of PyType_Type, which never will change.
    Therefore, the static keyword makes sure that this function lookup
    happens only once.
    
    Change-Id: I44b74556da1fac2596c81339af30cb66218276e2
    Reviewed-by: Friedemann Kleint <Friedemann.Kleint@qt.io>

commit 40a1a6bbeea82d8ccaaf7ec840ae8e0b8a183bc6
Author: Christian Tismer <tismer@stackless.com>
Date:   Tue Nov 26 11:54:37 2019 +0100

    Fix Python 3.8 problems
    
    This patch fixes some refcounting problems with Python 3.8 .
    One incompatible change was announced in the what's new
    document, but actually there were two more problems which
    were not explicitly mentioned but took much time to sort out.
    
    The patch is compatible with the limited API changes
    (tested with debug build and API error disabled).
    It is also independent of the Python version which is
    full Limited API support.
    
    For more info, see the documentation mentioned below.
    
    The flag error is circumvented now! We either find a better
    solution or leave it as it is. For now this is ok.
    
    Fixes: PYSIDE-939
    Change-Id: Iff4a9816857a6ebe86efd4b654d8921e4e464939
    Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
    Reviewed-by: Cristian Maureira-Fredes <cristian.maureira-fredes@qt.io>

And added these changes to make the compilation work again:

diff --git a/sources/shiboken2/libshiboken/basewrapper.cpp b/sources/shiboken2/libshiboken/basewrapper.cpp
index 1e3b7406..5824dd66 100644
--- a/sources/shiboken2/libshiboken/basewrapper.cpp
+++ b/sources/shiboken2/libshiboken/basewrapper.cpp
@@ -506,7 +506,8 @@ PyObject *SbkObjectTypeTpNew(PyTypeObject *metatype, PyObject *args, PyObject *k
     // PYSIDE-939: This is a temporary patch that circumvents the problem
     // with Py_TPFLAGS_METHOD_DESCRIPTOR until this is finally solved.
     PyObject *ob_PyType_Type = reinterpret_cast<PyObject *>(&PyType_Type);
-    static PyObject *mro = PyObject_GetAttr(ob_PyType_Type, Shiboken::PyName::mro());
+    static PyObject *const mro_string = Shiboken::String::fromCString("mro");
+    static PyObject *mro = PyObject_GetAttr(ob_PyType_Type, mro_string);
     auto hold = Py_TYPE(mro)->tp_flags;
     Py_TYPE(mro)->tp_flags &= ~Py_TPFLAGS_METHOD_DESCRIPTOR;
     auto *newType = reinterpret_cast<SbkObjectType *>(type_new(metatype, args, kwds));


build:
number: 6
number: 7
Copy link
Contributor

Choose a reason for hiding this comment

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

you are still patching up 5.13.1, do you want to patch up 5.12.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tried to do so. I used the last available build (pyside2 5.13.1) as it was working nicely for me.

@looooo
Copy link
Contributor Author

looooo commented Jan 12, 2020

@hmaarrfk we used pyside2 5.13 with qt5.12 for a while without too much trouble. So I thought this combination is fine. Are there any known issues of pyside2 5.13 + qt5.12?

@hmaarrfk
Copy link
Contributor

its just not so supported, see discussion in #61

Not sure what version your patches were targetting.

@looooo
Copy link
Contributor Author

looooo commented Jan 12, 2020

@hmaarrfk While pyside2 5.13 might not be supported to work with qt 5.12 (officially) I didn't see any bigger issues with this combination. I also saw this combination used in other distros, so I think we should stick to pyside5.13 for now and switch back once issues are reported.

// PYSIDE-939: This is a temporary patch that circumvents the problem
// with Py_TPFLAGS_METHOD_DESCRIPTOR until this is finally solved.
PyObject *ob_PyType_Type = reinterpret_cast<PyObject *>(&PyType_Type);
- static PyObject *mro = PyObject_GetAttr(ob_PyType_Type, Shiboken::PyName::mro());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure where the static keyword is comming from. It seems to me that it isn't in the 5.13.1 or 5.13.2 source

Copy link
Contributor

Choose a reason for hiding this comment

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

OHHHHHHHHHHHHH. You need to flip 0003 and 0004.

@hmaarrfk
Copy link
Contributor

you seem to have accidentally deleted the patches.

@hmaarrfk
Copy link
Contributor

hmm, didn't know i had rights to your branch. lets see what happens.

@hmaarrfk
Copy link
Contributor

Nice thanks for doing this. Should all versions of python be patched with this?

@jschueller
Copy link
Contributor

jschueller commented Jan 13, 2020

Patches can be applied for all versions, no need for selectors.

@jschueller jschueller mentioned this pull request Jan 13, 2020
@@ -3,6 +3,7 @@
XVFB_RUN=""
if test `uname` = "Linux"
then
cp -r /usr/include/xcb ${PREFIX}/include/qt
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a work-around as pyside messes up system includes and does not find xcb headers installed in the prefix

recipe/meta.yaml Outdated
# pyside2 5.13.1 needs a few backports from 5.14 to work with python3.8
- 0002-Fix-Python-3.8-problems.patch # [py >= 38]
- 0003-Optimize-the-Python-3.8-refcount-fix-a-tiny-bit.patch # [py >= 38]
- 0004-make-compilation-work-after-backporting.patch # [py >= 38]
Copy link
Contributor

Choose a reason for hiding this comment

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

Patches can be applied for all versions, no need for selectors.

@jschueller jschueller merged commit 37e0b33 into conda-forge:master Jan 13, 2020
@looooo looooo deleted the backport5.14 branch January 13, 2020 17:39
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

5 participants