conda-forge / pyqt-feedstock

A conda-smithy repository for pyqt.
BSD 3-Clause "New" or "Revised" License
5 stars 35 forks source link

Require Python >= 3.6.1 for PySlice_AdjustIndices #25

Closed jschueller closed 7 years ago

jschueller commented 7 years ago

with python 3.6.0 from miniconda-latest: PyQt5/QtCore.so: undefined symbol: PySlice_AdjustIndices, this is a symbol that was added in python 3.6.1, which is used for build

conda-forge-linter commented 7 years ago

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.

astaric commented 7 years ago

+1 for this, I have encountered this error as well

astaric commented 7 years ago

Related python ticket: https://bugs.python.org/issue29943

After reading the comments there, I am not sure whether 3.6.2 is going to be compatible with 3.6.0 or 3.6.1. Should python be pinned to the exact version (=3.6.1)?

jakirkham commented 7 years ago

It looks like the author of that patch is reverting it. Maybe excluding 3.6.1 is the right solution.

jschueller commented 7 years ago

@jakirkham as build dependency ? how ?

jakirkham commented 7 years ago

Adding python !=3.6.1. Both build and run, right? Otherwise PySlice_GetIndicesEx will be missing.

jakirkham commented 7 years ago

Does this sound right @njsmith? Also here is another case to add to your issue if needed.

jschueller commented 7 years ago

what happens if we install py361 then pyqt with your fix ? I'd only exclude 361 from build.

jakirkham commented 7 years ago

...PySlice_GetIndicesEx was converted into a macro that calls this new function [PySlice_AdjustIndices]. The patch was backported to both the 3.5 and 3.6 branches, was released in 3.6.1, and is currently slated to be released as part of 3.5.4.

Quoted from the OP in the bug report with clarification as to the new function.

jakirkham commented 7 years ago

what happens if we install py361 then pyqt with your fix ? I'd only exclude 361 from build.

Then it will be missing the PySlice_GetIndicesEx symbol and fail to load. Also patches to Python 2.7 and 3.5 have already been merged it appears. Not seeing a patch for Python 3.6 ATM.

jschueller commented 7 years ago

PySlice_GetIndicesEx already exists, its PySlice_AdjustIndices that was added

jakirkham commented 7 years ago

No, PySlice_GetIndicesEx use to be a function. Now it is a macro. Please see commit ( https://github.com/python/cpython/commit/b2a5be0763ee572bed431335eb4712fac94f5a7b ) for details. This broke CPython's API as this function was suppose to be part of the CPython API including in Python 3.6. The addition of PySlice_AdjustIndices is also bad, but the reason it shows up at all is the PySlice_GetIndicesEx macro calls it.

njsmith commented 7 years ago

After reading the comments there, I am not sure whether 3.6.2 is going to be compatible with 3.6.0 or 3.6.1. Should python be pinned to the exact version (=3.6.1)?

The change in 3.6.1 only broke compatibility in one direction. Anything built against 3.6.0 to will definitely run on 3.6.1 and 3.6.2; the problem is that things built against 3.6.1 can't necessarily run on 3.6.0. I expect that things built against 3.6.1 will also run on 3.6.2 (ie even if they revert the patch they'll be careful to do it in a way that avoids adding new breakage). I don't know what will happen if you build on 3.6.2.

So yeah, the most sensible options I think are to either make sure to build against 3.6.0 (possibly as a general policy across conda-forge...), or else add a run requirement for >=3.6.1.

jschueller commented 7 years ago

I chose the second option: add a run requirement for >=3.6.1, that's what I already did for openturns.

jschueller commented 7 years ago

ping, this is good to go only a connection failure in 1/6 appveyor job

mingwandroid commented 7 years ago

The third option is to patch Python.h until Python 3.8 so that extensions remain compatible across all of Python 3.7.x. This is what we did on defaults.

mingwandroid commented 7 years ago

The third option: https://github.com/conda-forge/python-feedstock/pull/145

jschueller commented 7 years ago

@mingwandroid is that the upstream patch ?

mingwandroid commented 7 years ago

We don't know what upstream will do yet but we did ask on the issue whether this approach was reasonable.

It disables the fix that the breakage was meant to address until Python 3.7 (when the patch would be removed).

Read the issue for details.