search

ukoethe / vigra

a generic C++ library for image analysis
http://ukoethe.github.io/vigra/
Other
407 stars 191 forks source link

Update CMake FindPython to obtain pypy support #485

Closed hmaarrfk closed 11 months ago

hmaarrfk commented 3 years ago

See build logs https://github.com/conda-forge/vigra-feedstock/pull/71

Closes https://github.com/ukoethe/vigra/issues/484

hmaarrfk commented 3 years ago

I have to say, i'm not particularly excited about getting python 2.7 to work.

hmaarrfk commented 3 years ago

The issue is likely how python 2.7 resolves compiled libraries.

PyPy refused to load extensions compiled with .so. I tried to use a directive that was common to Python 3.6.-3.9 and PyPy, but it seems incompatible with PyPy.

IF Python 2.7 support is still a requirement, then I will have more work to you.

Let me know if you need Python 2.7 support or not.

hmeine commented 3 years ago

IMO, we could stop supporting Python 2.7; we may just introduce a tag for future reference, but nowadays Python 3.x is the default and a reasonable dependency.

hmaarrfk commented 3 years ago

Is Vigra 1.11.1 a good enough tag? or do you need time to cut a new release?

Whatever you decide i'm ok with. Just I would rather spend my bandwidth tyring to understand why the mac builds are failing instead of fixing Python 2.7.

hmeine commented 3 years ago

True. For a moment I thought you were suggesting to make changes that ditch 2.7 support, in which case I would have tagged the immediate revision before that change. But now I realize that you're just not trying to add 2.7 support for the new pypy setup, which I find totally reasonable. In that case, we do not need a new tag, I believe.

hmaarrfk commented 3 years ago

I think this PR does break 2.7 support. I'm not sure why.

It should only affect the build system. But I'm not sure if I can take on the task of ensuring it works for pypy and 2.7 all at once.

hmeine commented 3 years ago

I hope I was clear enough that I don't think 2.7 support should be your concern anymore.

Let me know when I should review / merge; I have not yet taken a closer look (with the "[WIP]" tag and discussion in mind).

hmaarrfk commented 3 years ago

If that is the case, then i guess we should create a tag for the version immediately preceding this PR.

Python Version vigra CIs on master vigra CIs after this PR Conda Forge with this patch
Linux + CPython 3.6 Pass Pass Pass
Linux + CPython 2.7 Pass Fail N/A
Mac + CPython 3.6 Fail Fail Pass
Mac + CPython 2.7 Fail Fail N/A
Windows + CPython Pass Pass Pass
Linux + PyPy 3.6 N/A N/A Pass
Mac + PyPy 3.6 N/A N/A Pass
hmeine commented 3 years ago
  1. Question: The Conda Forge column is obviously with the PR, right? Just want to make sure that the two last rows would be "Fail" without it.
  2. I have pushed a tag on HEAD.
  3. Again, please tell me when I should review / merge.
hmaarrfk commented 3 years ago

Yes it is "with this patch" that said, I probably touched it up a little bit and an earlier version of my work was merged to conda-forge.

I would like to test it on conda-forge before asking you to review. I will remove the WIP when that is done.

Thank you for working through confirming that dropping python 2.7 was OK!

hmaarrfk commented 3 years ago

Building started: https://github.com/conda-forge/vigra-feedstock/pull/74

hmaarrfk commented 3 years ago

Conda forge build matrix seems to be ok now using the patch from this PR.

hmeine commented 2 years ago

Should I now merge this? (I am afraid I just created a conflict by merging another change.) I realize you have removed the "[WIP]" as announced – sorry for being so late to check that.

@ukoethe I know you are no longer interested in maintaining vigra – would it make sense to give someone like @hmaarrfk who has made a few contributions and is maintaining vigra on conda-forge maintainer rights? IMO as long as people are actively looking into issues, I think it's a pity if such things are not merged at all, and I am also not really fulfilling a maintainer role with my occasional reviews, comments or merges.

hmaarrfk commented 2 years ago

i think occasional stuff is fine! thanks for looking at this again! should I try to resolve the conflicts?

hmaarrfk commented 2 years ago

@hmeine, @k-dominik has been helping alot keeping the package up to date on conda-forge. I think we would benefit from shedding some of our numerous patches and creating an official release.

Patch list: https://github.com/conda-forge/vigra-feedstock/blob/master/recipe/meta.yaml

hmeine commented 2 years ago

I am interested in merging changes that make vigra compile with current compilers and environments again. I am not so familiar with conda-forge, plus I think if I was importing patches from some other source, the contributions would be incorrectly attributed to myself (if I don't use pay attention to teach git about it at least), so if you could prepare PRs, that would be really helpful. I see that @k-dominik has also replied in #501, so I will see that I monitor my GitHub notifications more often in the near future.

hmaarrfk commented 2 years ago

I'm happy to take superpowers where i can. I do try to to stay professional and in accordance to to the projects existing spirit.