manoharan-lab / holopy

Hologram processing and light scattering in python
GNU General Public License v3.0
135 stars 50 forks source link

Postinstall for win64 #304

Closed ralex0 closed 4 years ago

ralex0 commented 4 years ago

To get our win64 build working with conda-forge, we need to make the setup.py compatible with Microsoft Visual Studio (MSVC) 2015 (14.0)'s C compiler. I have learnt that for some [magical] reason when the C compiler is MSVC2015, when f2py runs, it puts the dynamic link libraries (dll) required for holopy's fortran extensions to run in holopy/.lib. On a whim I decided to put these libraries in their "appropriate" locations next to their .pyd files within the package. This allowed me to import the extensions compiled with MSVC2015 as the C compiler and msys2's gfortran as the Fortran compiler. Here is the conda build environment in which I discovered this. I have mingw64 installed through chocolatey, so that's why there's no m2w64-gcc-fortran in the environment.

This PR moves these libraries to the appropriate locations in a post install script if the platform is Windows.

Theoretically there is a different workaround using the f2py_options keyword argument of numpy.distutils.core.setup, but I couldn't make this work. The option that I want to pass is --compiler=mingw32. On my win64 environments that match one conda-forge uses (i.e. using MSVC2015) I can't build any f2py extension unless I either pass --compiler=mingw32 or just move the dlls like in this patch. The problem is passing the f2py_options in setup.py does nothing as far as I can see.

Since these changes are changes to the way holopy is built, I'm not sure how to test it.

barkls commented 4 years ago

It's great you got this to build! It's too bad the f2py_options method doesn't work, since that seems so much more straightforward and less likely to break if HoloPy is installed to an unexpected location.

Where did you try adding the f2py_options flag? It looks like the config.add_extension calls would be the most basic place, but somewhere higher up might also work?

What format did you use for the f2py_options argument? It looks like it can be tricky to get it to parse correctly, but it should possibly be a list of strings: f2py_options=['compiler:', 'mingw32', ':'].

I might play around with this later on my Windows laptop, but I want to make sure I'm not redoing things you already tried!

ralex0 commented 4 years ago

I tried passing as a dictionary to config.add_extension. I'll experiment as well tonight because I agree that passing --compiler=mingw32 to f2py_options is the better solution. I think you're onto something with that syntax which I did not try yet.

ralex0 commented 4 years ago

We might want to include this patch even if we get f2py_options to work as intended. Maybe we should change this PR to check if MSVC is installed instead of whether os.name == 'nt'.

holopy can cetrainly be compiled without MSVC, but currently conda-forge insists that MSVC is the C compiler that must be used on Windows in their DevOps pipeline. [IMO] The end-user shouldn't have to worry about their environment, so if they just so happen to have MSVC installed, holopy will still work for them with this patch.

This post-install script basically does nothing even if the OS is Windows and the dlls don't exist. Furthermore, any Windows user that is able to compile holopy right now probably has gfortran, so it's necessary that they'll also have g++, and the "--compiler=mingw32" keyword will succeed if we can figure it out. However, an edge case is the user has MSVC14 as the C compiler and ifort (Intel) as the Fortran compiler.

Another thing is we would definitely need to keep m2w64-toolchain as a host requirement for win in meta.yaml since there is no way to ensure the host has gcc unless we have more control over provisioning, which is not possible with conda-forge's build pipeline as far as I know. Ideally, I would run choco install msys2 before the install.

Thoughts?

barkls commented 4 years ago

I tried to play around with this on my computer, but I'm afraid it is beyond me. Did you get a chance to try the f2py_options passed in as a list of strings?

Even if we can't get that working, this PR lets conda-forge build HoloPy, which is all we need for a distribution. It should also be fairly straightforward for developers to install as well since MSVC is readily available. The developer documentation should be updated to reflect this change. Do you feel like doing that as part of this PR?

I do think it's a good idea to check if MSVC is installed rather than checking operating system.

If we can get a gcc-based build working, I prefer keeping the m2w64-toolchain as a requirement in meta.yaml vs. running installations through other package managers - that seems very opposed to the goals of conda-forge and is likely to run into issues as they update things.

ralex0 commented 4 years ago

245fd17 is based on a conversation @barkls and I had yesterday

briandleahy commented 4 years ago

As long as this works on your Windows machine from a fresh install, this looks good to me @ralex0 ! Thanks for doing this.

I'll let @barkls merge this in, in case he has any other questions or comments.