search

conda-forge / rdkit-feedstock

A conda-smithy repository for rdkit.
BSD 3-Clause "New" or "Revised" License
8 stars 22 forks source link

Switch to DLL builds on windows #86

Closed greglandrum closed 2 years ago

greglandrum commented 2 years ago

We've had a long-open issue on the rdkit side that log messages (used for error reporting) do not show up properly on Windows: https://github.com/rdkit/rdkit/issues/2760 After some investigation it looks like this is only solvable by switching the windows builds of the python wrappers to use DLL builds of the RDKit core instead of statically linking it. The Mac and Linux builds have been dynamic linkage from the beginning, so they don't have this problem.

This PR is one-liner which switches on DLL builds for windows.

Checklist

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

greglandrum commented 2 years ago

@conda-forge-admin, please rerender

greglandrum commented 2 years ago

@jaimergp, @mcs07: do either of you know how to get the recipe to copy the DLLs from the build/lib directory to wherever they should end up? This could be an RDKit misconfiguration (in which case I will fix it upstream), but it looks like the cmake config for the DLLs on windows should be the same as the shared libs on linux/mac, and those work fine with the conda recipe.

jaimergp commented 2 years ago

I think DLLs belong to LIBRARY_BIN or SCRIPTS (they need to be placed along the executables, basically, but it depends on how the package is configured). Try both and let's see which one works!

jaimergp commented 2 years ago

Ah, there's another issue:

(base) D:\bld\rdkit_1635854592478\work>REM copy .dll files to LIBRARY_LIB 

(base) D:\bld\rdkit_1635854592478\work>copy lib\*.dll D:\bld\rdkit_1635854592478\_h_env\Library\lib 
lib\*.dll 
The system cannot find the file specified.
        0 file(s) copied.

It doesn't look like the DLLs are there :/

greglandrum commented 2 years ago

It doesn't look like the DLLs are there :/

hmmm, let me take a look at that. It may need to be ../lib

greglandrum commented 2 years ago

from looking at the output of the build on windows in CI, it looks like there is stuff being installed into:

[ 37%] Linking CXX shared library ..\..\bin\RDKitGraphMol.dll

https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=401362&view=logs&j=171a126d-c574-5c8c-1269-ff3b989e923d&t=1183ba29-a0b5-5324-8463-2a49ace9e213&l=5939

and:

-- Installing: D:/bld/rdkit_1635854592478/_h_env/Library/lib/RDKitGraphMol.dll

https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=401362&view=logs&j=171a126d-c574-5c8c-1269-ff3b989e923d&t=1183ba29-a0b5-5324-8463-2a49ace9e213&l=8806

That second one looks like the conda install is putting the DLL in Library/lib, but I think the issue is that these dll files need to be in Library/bin (I guess that's $LIBRARY_BIN). Looking at a local environment on windows, the boost DLLs are in Library/bin and the corresponding .lib files are in Library/lib. The RDKit should be analogous.

My first reaction would be to just add a hack and move the files, but perhaps there's a better answer for that?

jaimergp commented 2 years ago

I don't think conda is involved in placing the DLLs anywhere. I'd say that's coming from the Makefile generated by CMake. DLLs being LIBRARY_BIN does sound like the correct way. If fixing CMakeFiles for this sounds overkill, moving them from LIBRARY_LIB to LIBRARY_BIN sounds like an acceptable workaround (but be careful that only the RDKit DLLs are moved).

greglandrum commented 2 years ago

@jaimergp @mcs07 it looks like I managed to find the appropriate incantation to make this work. The one failing CI build is on OSX and looks like an artifact, not something real