conda-forge / pyside2-feedstock

A conda-smithy repository for pyside2.
BSD 3-Clause "New" or "Revised" License
17 stars 19 forks source link

disable propagation of PY_LIMITED_API to dependent targets #233

Open looooo opened 1 month ago

looooo commented 1 month ago

Checklist

conda-forge-webservices[bot] commented 1 month 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/meta.yaml) and found it was in an excellent condition.

looooo commented 1 month ago

232

hmaarrfk commented 1 month ago

I'm not too sure if removing this file would break things:

  cp -v ./tests/pysidetest/testbinding.abi3.so ${SP_DIR}

are you able to use the limited ABI in your package? it seems that more and more packages are moving in that direction. May I ask if there is 1 particular function that needs it or is it more wide spread?

looooo commented 1 month ago

are you able to use the limited ABI in your package?

not yet. Also I don't think the transition will happen for the next release.

May I ask if there is 1 particular function that needs it or is it more wide spread?

From looking at the build-errors it seems like it's used in more than one place.

I am wondering why the LIMITED_API is propragated to downstream packages? Is there any reason for this?

conda-forge-webservices[bot] commented 1 month ago

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

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

conda-forge-webservices[bot] commented 1 month 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/meta.yaml) and found it was in an excellent condition.

looooo commented 1 month ago

this addition changes in ${PREFIX}/lib/cmake/Shiboken6/Shiboken6Targets.cmake this:

set_target_properties(Shiboken6::libshiboken PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "Py_LIMITED_API=0x03060000;NDEBUG"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/shiboken6"
)

to

set_target_properties(Shiboken6::libshiboken PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "NDEBUG"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/shiboken6"
)

In my mind Pyside6/Shiboken6 should not propagate the PY_LIMITED_API to downstram packages. At least it makes no sense for me.

hmaarrfk commented 1 month ago

it may be appropriate for us to patch that out and simultaneously issue a bug report upstream.

hmaarrfk commented 1 month ago

ps it may have been due to https://bugreports.qt.io/browse/PYSIDE-1775?jql=text+%7E+%22PY_LIMITED_API%22

i think it would be good if you could open an issue upstream, and we can "track it" even if for years to come...

looooo commented 1 month ago

done: https://bugreports.qt.io/browse/PYSIDE-2822

and it is closed already. I guesss it's good practice to move to PY_LIMITED_API, but forcing downstream projects to do so, is not the right way (in my opinion)