search

conda-forge / vigra-feedstock

A conda-smithy repository for vigra.
BSD 3-Clause "New" or "Revised" License
0 stars 14 forks source link

Add patch for cpp17 support #78

Closed constantinpape closed 3 years ago

constantinpape commented 3 years ago

Checklist

This patch applies https://github.com/ukoethe/vigra/pull/500, which replaces the deprecated bind1st and bind2nd with bind. This should enable building vigra with c++17 on macOs.

conda-forge-linter commented 3 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.

constantinpape commented 3 years ago

@conda-forge-admin, please rerender

hmaarrfk commented 3 years ago

Thank you for this. im curious why we need this.

Chances are, your patch will be accepted upstream by the time conda-forge moves to cpp17

constantinpape commented 3 years ago

im curious why we need this.

In fact conda-forge has moved to cpp17 in the standard cxx_flags already and hence downstream dependencies need to change the cxx_flags manually to keep osx builds working, see for example https://github.com/constantinpape/nifty-feedstock/blob/master/recipe/build.sh#L13-L17. I have some other downstream dependencies that actually need c++17 now and without merging this I can't get them onto conda-forge.

Chances are, your patch will be accepted upstream by the time conda-forge moves to cpp17

There is an issue on c++17 for over a year now without any interaction from maintainers, so I doubt that this will be merged in vigra anytime soon: https://github.com/ukoethe/vigra/issues/481.

hmaarrfk commented 3 years ago

i guess we had a build 17 days ago in master that passed. what happened there?

hmaarrfk commented 3 years ago

FWIW, the patch looks pretty good. i just want to make sure I understand.

hmaarrfk commented 3 years ago

i think our build.sh sets the cpp standard. we should remove it with this. Thanks!

constantinpape commented 3 years ago

i guess we had a build 17 days ago in master that passed. what happened there?

I think this: https://github.com/conda-forge/vigra-feedstock/blob/master/recipe/build.sh#L13-L16 hard-codes the cpp standard to c++ 11 and over-rides the conda-forge default flags.

I can remove it if you want and see if the build still passes. (That's not necessary for my downstream use case, because it uses vigra as include dependency, but it is probably more clean to build with the conda-forge defaults.)

hmaarrfk commented 3 years ago

Thanks. I think that the without the compilation settings, this patch would have been moot. This looks good and makes sense from a cleanup standpoint.

hmaarrfk commented 3 years ago

One last question, given that this is a compile change, how does this affect your workflow? Aren't you downloading the packages from conda-forge using conda?

How does compiling with C++17 change downstream compatibility for your application?

constantinpape commented 3 years ago

One last question, given that this is a compile change, how does this affect your workflow? Aren't you downloading the packages from conda-forge using conda?

How does compiling with C++17 change downstream compatibility for your application?

Like I said, I am only using vigra as a header only dependency downstream, so changing the compilation flags here does not change anything for me; but I need the patches applied in order to build with c++17 myself.

I guess that if there are any downstream dependencies that are linking to the vigra libraries they would need to change to build with c++17 now. (Which I think is fair, given that this is the default on conda-forge now).

hmaarrfk commented 3 years ago

Fully understood. This makes ALOT of sense.

These patches are to support the headers, and help downstream projects migrate to c++17 instead of being stuck on c++11.

Thank you for helping me clarify this.

constantinpape commented 3 years ago

Thanks for the review and quick merge!

stuarteberg commented 3 years ago

I'm just chiming in here to say thanks to you both, @hmaarrfk @constantinpape, for your attention on this feedstock. I use this package a lot and I'm very glad to see it well maintained.