conda-forge / cctbx-base-feedstock

A conda-smithy repository for cctbx-base.
BSD 3-Clause "New" or "Revised" License
1 stars 6 forks source link

ANN/ANN.h missing #46

Closed ndevenish closed 2 years ago

ndevenish commented 2 years ago

#include <ANN/ANN.h> is used by lib/python3.10/site-packages/annlib_adaptbx/include/annlib_adaptbx/ann_adaptor.h, currently itself included by DIALS indexing.

The lib/libann.so (built by annlib_adaptbx using the annlib sources) and share/cctbx/annlib_adaptbx/include/ANNSELF_INCLUDE/ANN.h from the build/ folder are included, so it looks like it's mostly a simple case of just being missed. I've never worked out the reason for the ANN/ANNSELF_INCLUDE duplication for headers (duplication except the #define guard, and namespace), but copying them both into the share/cctbx/annlib_adaptbx/include folder might be sufficient?

I don't know what the state of this was when https://github.com/conda-forge/cctbx-base-feedstock/issues/12, but that implies it was missing and most of it seems to actually be included now.

Anthchirp commented 2 years ago

I think #12 was about a declared dependency that wasn't actually required. Vendoring ann is probably not the preferred solution since that's already a separate conda-forge package (https://github.com/conda-forge/ann-feedstock)

ndevenish commented 2 years ago

Hmm, this makes it a little more complicated, because IIRC you aren't supposed to vendor other libraries and I don't know what differences the annlib_adaptbx-compiled version of ann has (other than being v1.1 instead of v1.1.3).

ann also does a lib/libann.so, so that means that this package is clobbering, if installed afterwards.

bkpoon commented 2 years ago

Clobbering lib/libann.so is not desired so we can probably rename the library and related linking.

@nksauter and @phyy-nx What else uses annlib?

nksauter commented 2 years ago

Annlib is used in labelit, possibly dials 1D-FFT, and for various ad-hoc nearest neighbor applications in XFEL. It should be treated as a required dependency for XFEL. Nick

On Mon, Apr 4, 2022 at 10:29 AM Billy K. Poon @.***> wrote:

Clobbering lib/libann.so is not desired so we can probably rename the library and related linking.

@nksauter https://github.com/nksauter and @phyy-nx https://github.com/phyy-nx What else uses annlib?

— Reply to this email directly, view it on GitHub https://github.com/conda-forge/cctbx-base-feedstock/issues/46#issuecomment-1087823392, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADQ24VXCDHD52SHLC33K3TTVDMRG7ANCNFSM5SPPZIOA . You are receiving this because you were mentioned.Message ID: @.***>

ndevenish commented 2 years ago

What does the annself_include variant actually do? Can this be a separately named library and use upstream libann?

nksauter commented 2 years ago

The two instances represent two distinct algorithms: nearest neighbors with or without inclusion of self. The source code has to be copied and edited to provide both algorithms. --Nick

On Mon, Apr 4, 2022 at 11:29 AM Nicholas Devenish @.***> wrote:

What does the annself_include variant actually do? Can this be a separately named library and use upstream libann?

— Reply to this email directly, view it on GitHub https://github.com/conda-forge/cctbx-base-feedstock/issues/46#issuecomment-1087878116, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADQ24VXGGAKY6CEC6DJ7CH3VDMYHHANCNFSM5SPPZIOA . You are receiving this because you were mentioned.Message ID: @.***>

ndevenish commented 2 years ago

So ANN/ANN.h is included now?

bkpoon commented 2 years ago

No, I just renamed the library to avoid clobbering. The modules directory from bootstrap.py has annlib which will have that header. Or do you want me to put a copy into ${CONDA_PREFIX}/share/cctbx/annlib_adaptbx/include/ANN?

ndevenish commented 2 years ago

I think that'd be the best option; if you want to use ANNSELF_INCLUDE/ANN.h, then you have already added that exact path to the include search paths - but without copying the files in there also you only have the header files to make use of 50% of the symbols compiled into libann_cctbx.

The headers are also included in conda-forge/ann, but then you need to be careful to constrain versions to match the one that cctbx uses internally (and would have a different set of symbols for binaries linking to libann than in ones linking to libann_cctbx).

Since the symbols are being included in this package anyway, it seems to make sense to include the headers to use them.

bkpoon commented 2 years ago

I'll make another PR later today to add that.

ndevenish commented 2 years ago

Awesome, thanks!

bkpoon commented 2 years ago

The headers should now be available in ${CONDA_PREFIX}/share/cctbx/annlib_adaptbx/include/ANN on unix and %CONDA_PREFIX%\Library\share\cctbx\annlib_adaptbx\include\ANN on Windows.