Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Building for Windows with MSVC #12

Open
mcm001 opened this issue Dec 1, 2023 · 36 comments
Open

Building for Windows with MSVC #12

mcm001 opened this issue Dec 1, 2023 · 36 comments

Comments

@mcm001
Copy link

mcm001 commented Dec 1, 2023

I realize this is a pretty huge ask but putting it out there anyways. Since a lot of our users run Windows it would be awesome to be able to support mrcal and all the related visualization tools on that platform too. We compile all our other libraries with MSVC right now, so I'm not sure if mingw-gcc would play particularly nicely. I started hacking away at things just to see how hard it would be to make work (link below + some other changes needed to libdogleg), but I'm curious to hear your thoughts.

master...mcm001:mrcal:master

@dkogan
Copy link
Owner

dkogan commented Dec 1, 2023

Hi. I'll help with whatever porting issues you encounter, but you're going to have to do all the actual work on this one. What if you target wsl? Wouldn't it make most of the problems disappear?

For the visualization, gnuplot is the backend, and it DOES support windows. The wrapper libraries had multiple bug reports over the years saying that they are broken on windows, but it sounds like all that needs to happen is to find the win32 flavor of select(). I'm sure that's not difficult

@dkogan
Copy link
Owner

dkogan commented Dec 1, 2023

I just looked at your work tree. It looks like you're effectively trying to re-architect big chunks of this: moving the build system to cmake, moving the main sources from C to C++ and so on. I wouldn't take that route because

  • It's a lot of work for you
  • I wouldn't accept most of those changes back into my tree when you're done (most of this isn't a "port" anymore), so you now have a fork

But this all doesn't seem necessary, though. You can get most of the dependencies (make, gcc, suitesparse) working natively on windows. Even if you want to stick with the msvc compiler for whatever reason, surely it can handle C99 code? This feels like not the most expedient way to get where you want to go.

@mcm001
Copy link
Author

mcm001 commented Dec 2, 2023

A lot of mrcal and libdogleg use GCC-specific compiler extensions that are unsupported by MSVC. Most problematically, variable length arrays and nested functions. It seems like Clang might be able to handle this, and generate MSVC-abi-compatible DLLS and such as well?

And yes WSL is a good option. It works great for me for developing code. But for distributing tools to users, it's a lot lower friction for us to be able to distribute an exe instead of asking people to install WSL and configure USB camera passthrough (which doesn't even always work? My luck with V4L and WSL hasn't been great) and such.

So in conclusion:

  • We use other libraries compiled using MSVC, so need to produce MSVC-compatible DLLs for libdogleg and mrcal
  • MSVC doesn't support VLAs or nested functions
  • Clang claims to support those? But I haven't gotten clang + cmake working yet.

@mcm001
Copy link
Author

mcm001 commented Dec 2, 2023

Regarding build systems. I haven't tried but I suspect Mrbuild wouldn't Just Work in windows without some effort?

@dkogan
Copy link
Owner

dkogan commented Dec 2, 2023 via email

@mcm001
Copy link
Author

mcm001 commented Dec 2, 2023

Sorry, I wasn't 100% precise. VLAs are part of the C99 standard, but MSVC isn't fully standards compliant. I briefly tried installing Clang through Visual Studio, and compiling a test program using a VLA, but looks like that doesn't work either? Which would limit us to GCC through mingw or similar.

PS D:\Documents\GitHub\libdogleg> & "C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.33.31629/bin/Hostx64/x64/cl.exe" .\test.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.33.31630 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.c
.\test.c(2): error C2057: expected constant expression
.\test.c(2): error C2466: cannot allocate an array of constant size 0
.\test.c(2): error C2133: 'arr': unknown size

I'm using right now these random pre-compiled binaries, but it seems possible to also compile using VCPKG too.

I don't have any docs to back me up, but I've read online and have anecdotes from friends about ABI incompatabilities between binaries compiled using GCC and MSVC (specifically they were trying to work with ipopt/casadi, which need fortran compilers? Not 100% on the details there). And yeah, by MSVC I specifically mean the compiler.

I can give mrbuild a try, but my main goal here was to figure out how painful the build system setup will be on Windows, and CMake was the lowest friction path to getting that to happen for me.

@dkogan
Copy link
Owner

dkogan commented Dec 2, 2023

OK. If MSVC is primarily a C++ compiler, and doesn't do C99 (which is the sense I'm getting), then I would use that here only as a last resort. I would only undertake MSVC support with the upstream maintainer's cooperation, and this upstream maintainer is cantankerous :)

Today it looks like I don't build with clang on any platform. But that's something I'm happy to fix. I'm not familiar with mingw. Is clang more "native" on windows than gcc in some significant way? If today you can get away with gcc (thus not requiring me to fix clang support right now) that would be great. But if not, tell me, and I'll fix it.

In the example above you're running cl.exe, which isn't actually clang. clang should support VLAs, since those are C99.

As for the build, since you're not building the Python stuff, the build system will be trivial, regardless of the tooling. You shouldn't see problems with mrbuild, but whatever you do is fine. Just don't ask me to merge your cmake stuff :)

@mcm001
Copy link
Author

mcm001 commented Dec 4, 2023

I'm not sure I would agree that MSVC is primarily a C++ compiler? Anyways, I spent some time over the weekend playing with vcpkg and clang. Vcpkg at least gets me a fairly clean way to install suitesparse and BLAS/LAPACK.

I confirmed Clang supports VLAs and empty structs, but not inner functions. I'll continue trying to figure out my local build setup -- you're right, at the end of the day there isn't a whole lot going on in the build system, it's mostly just fighting with Windows.

If you're open to additionally patching mrcal and libdogleg to be buildable with MSVC by moving away from the two missing features from above, that would be incredible and ideal from my side. I need to confirm that VLAs and empty structs are the only two blockers that would prevent us from being able to build with MSVC, and we would be able to avoid moving to C++.

FYA, I did get decently far with my fork. I was able to replace nearly all VLAs with C++ vectors, but haven't yet nailed down a weird bug that seems to be corrupting memory inside of the callback that projects points. Not expecting any support there from you obviously, just wanted to note I was trying to follow the pointer chain and figure out where my patch went wrong.

@dkogan
Copy link
Owner

dkogan commented Dec 5, 2023 via email

@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023

Hello. I made a branch for the 2.4 release: https://github.com/dkogan/mrcal/tree/release-2.4

This will have some fixes, in particular, it will build with clang, and it will fix the memory leak you found. I just un-nested the functions in the C library, so libmrcal.so now builds with clang. The python-wrapping needs more work still, but is this enough to fix your problem? I think you were saying that building with clang would be enough for your use case, or did I misunderstand, and you can only use MSVC specifically? If so, then you still will need to deal with the other issues (empty structs and VLAs)

@mcm001
Copy link
Author

mcm001 commented Dec 8, 2023

Awesome, thank you for doing that! And yes, no python is fine for us right now for Windows. I'll LYK if that changes.

I was also able to get mrcal to build using Clang-cl from Visual Studio 2022! Some notes:

  • I had to define M_PI manually (both in libdogleg and here in mrcal)
  • I installed sparsesuite using vcpkg and linked to those libraries
  • I have to export symbols manually on any parts of the mrcal api I wanted to be able to call from code that links to mrcal.dll. I used this macro:
#ifdef __GNUC__
#define MRCAL_DLLEXPORT __attribute__((dllexport))
#else
#define MRCAL_DLLEXPORT __declspec(dllexport)
#endif

MRCAL int some_mrcal_thing();

I used this bat script to build:

clang-cl /LD /MT -v -I D:\Documents\GitHub\vcpkg\installed\x64-windows\include\suitesparse -I ../libdogleg mrcal.c cahvore.cc poseutils-opencv.c poseutils-uses-autodiff.cc poseutils.c mrcal-opencv.c D:\Documents\GitHub\vcpkg\installed\x64-windows\lib\libcholmod.lib D:\Documents\GitHub\vcpkg\installed\x64-windows\lib\suitesparseconfig.lib D:\Documents\GitHub\vcpkg\installed\x64-windows\lib\lapack.lib ../libdogleg/dogleg.lib

clang-cl -o mrcal_test.exe test_mrcal.c mrcal.lib

SET PATH=%PATH%;D:\Documents\GitHub\vcpkg\installed\x64-windows\bin
start mrcal_test.exe

@mcm001
Copy link
Author

mcm001 commented Dec 8, 2023

Here's the commit with all my changes: 98cd1f2

In terms of what I'd like upstreamed, i think it's just finding a fix for M_PI and adding DLLEXPORTs to mrcal.h?

@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023

OK, so for M_PI, I think you just need one extra define:

https://godbolt.org/z/1jY8TP515

It hurts nothing, so I'm happy to add it. Let me add it (to libdogleg master branch and mrcal release-2.4 branch) right after this. For the library export business, you need that extra thing for mrcal only, or for libdogleg also?

dkogan added a commit that referenced this issue Dec 8, 2023
dkogan added a commit to dkogan/libdogleg that referenced this issue Dec 8, 2023
@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023

I didn't remember how I did this in the two projects (mrcal, libdogleg), but I just looked. In both I use the default visibility settings: the symbols are exported by default, unless explicitly marked otherwise. It looks like there's exactly one function across both projects that does something other than the default: project_cahvore_internals() in cahvore.cc. Maybe it can be modified to not do that (msvc probably won't like the __attribute__), but let's first get all the other functions working properly.

Clearly the default setting is inverted in msvc: they hide the symbols by default. You should poke around the project settings to flip the visibility default to what the GNU system does. I can't find any obvious notes about what to do, specifically, but you have the thing in front of you, so you can go poke around the dialogs. Look at the linker options; stuff about "visibility" or "exports" or something.

@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023

There's a cmake property to do the magic thing:
https://cmake.org/cmake/help/latest/prop_tgt/WINDOWS_EXPORT_ALL_SYMBOLS.html

You can either use cmake, or you can make a test cmake project with that option, see what it does, and apply it to your visual studio project. I really don't want to be manually annotating each one of my symbols.

@mcm001
Copy link
Author

mcm001 commented Dec 8, 2023

Looks like it auto-generates a .def file, and uses that when building to export symbols? I agree that it's pretty annoying for you.

Since I'm happy to DIY my own build system, I think it's actually probably easier for me to just add mrcal and libdogleg as submodules and build that + my JNI at the same time so we never even have to worry about symbol exporting? Speaking of, mrcal is alive on Windows now! Haven't compared results to Linux, but it's passing my unit tests still so that's good
image

@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023

Nice! Maybe I'm being needlessly stubborn. Sometimes it IS useful to have an export whitelist instead of blacklist, and I don't have all that many exported symbols anyway. Let me think about it.

How did you end up dealing with the other issues? Are you using clang now, or msvc + patches?

@mcm001
Copy link
Author

mcm001 commented Dec 8, 2023

That screenshot is using clang-cl from Visual Studio. I pulled the latest from the mrcal's release-2.4 (244ef97) and libdogleg's master (1f88d01) to get the m_pi patches. I'm building using this little script, which is on the master branch of my mrcal-java repo

"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin\clang-cl.exe" /EHsc /LD /MD /std:c++20 -I ../mrcal -I D:\Documents\GitHub\vcpkg\installed\x64-windows\include\ -I D:\Documents\GitHub\vcpkg\installed\x64-windows\include\suitesparse -I build\_deps\opencv_header-src -I C:/Users/matth/.jdks/liberica-11.0.7/include -I ../libdogleg -I C:/Users/matth/.jdks/liberica-11.0.7/include/win32 mrcal_jni.cpp mrcal_wrapper.cpp  D:\Documents\GitHub\vcpkg\installed\x64-windows\lib\libcholmod.lib D:\Documents\GitHub\vcpkg\installed\x64-windows\lib\suitesparseconfig.lib D:\Documents\GitHub\vcpkg\installed\x64-windows\lib\lapack.lib ../libdogleg/dogleg.lib build\_deps\opencv_lib-src\windows\x86-64\shared\opencv_core460.lib build\_deps\opencv_lib-src\windows\x86-64\shared\opencv_calib3d460.lib ../mrcal/mrcal.c ../mrcal/cahvore.cc ../mrcal/poseutils-opencv.c ../mrcal/poseutils-uses-autodiff.cc ../mrcal/poseutils.c ../mrcal/mrcal-opencv.c 

@REM "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin\clang-cl.exe" /MT -o mrcal_test.exe ../mrcal/test_mrcal.c mrcal_jni.lib

I'm installing suitesparse using vcpkg and downloading opencv libraries from a wpilib build of opencv. It's all pretty cursed right now, tis but a proof of concept. I end up needing to put these DLLs onto my PATH as well for Java to be able to load the library:

matth@LAPTOP-F96B9E90 MINGW64 /d/Documents/GitHub/mrcal/allthedlls (release-2.4-windows)
$ ls
libamd.dll*      libgfortran-5.dll*      opencv_core460.dll*
libcamd.dll*     liblapack.dll*          opencv_features2d460.dll*
libccolamd.dll*  libquadmath-0.dll*      opencv_flann460.dll*
libcholmod.dll*  openblas.dll*           opencv_imgproc460.dll*
libcolamd.dll*   opencv_calib3d460.dll*

@mcm001
Copy link
Author

mcm001 commented Dec 8, 2023

I am also curious to see if I can avoid loading opencv. We load it separately already, so if there's an opencv_core460.dll loaded and we try to load mrcal_jni.dll, maybe it'll be happy to use the one already in memory? Would cut down the size of stuff I gotta ship a fair bit.

I'm also curious to see if we can statically link to lapack/blas and friends on Linux. It would reduce the number of libraries people need to install on their system.

And now that this at least sort of works, I'm curious to see some proper speed benchmarks for Linux vs windows since I think I'm using openblas above

@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023

Good news! You don't actually need opencv. Look at the Makefiles more closely. None of them us it. You definitely do need blas and lapack and suitesparse (cholmod specifically, and whatever it needs). Static linking could work. You might lose some performance, compared to a hardware optimized BLAS, but it might be worth a try.

@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023

Isn't there some form of package manager on Windows? Chocolatey? Could you get dependencies from that?

@mcm001
Copy link
Author

mcm001 commented Dec 8, 2023

Good news! You don't actually need opencv.

I know mrcal doesn't, but my JNI code uses cv::solvePNP as the initial board orientation guess. If you have a function in mrcal to estimate that for me though it would be awesome to delete that dependency.

Chocoltey or something?

Yeah, that and Vcpkg both claim to be package managers for Windows. Best I can tell, Vcpkg has good cmake integration, but not much else? It's worth looking closer into chocolatey though I think.

@dkogan
Copy link
Owner

dkogan commented Dec 8, 2023 via email

@mcm001
Copy link
Author

mcm001 commented Dec 8, 2023

Quick update: looks like Chocolatey does NOT have cholmod or suitesparse? Vcpkg does though. And I know how to make Vcpkg work the Right Way, I think.

@dkogan
Copy link
Owner

dkogan commented Dec 23, 2023

An update you may/may not care about. I just released numpysane 0.40. This removes nested functions from the code it generates, which means that all of mrcal (including the Python layer) builds with clang.

@mcm001
Copy link
Author

mcm001 commented Jan 1, 2024

Oh awesome! That's huge. That should mean we should be able to build/ship all of mrcal for windows now. I'm focusing on other stuff at the present, but would love to revisit this to try building Windows wheels at some poitn

@mcm001
Copy link
Author

mcm001 commented Jan 5, 2024

FYA, mrcal has made it into Photon! We also have some docs on our website, still working on building those out. Thanks again for all your help! Happy to close or leave open for ongoing windows python stuff

@dkogan
Copy link
Owner

dkogan commented Jan 5, 2024 via email

@mcm001
Copy link
Author

mcm001 commented Jan 5, 2024

Yes, I think clang is sufficient for building everything + python wheels on Windows, but I haven't yet had time to dig into it (and probably won't for a while).

Re: 2.4: how much mrcal-side work is it to expose what I'd need to make projection uncertainty graphs in the C API? It looks like there's a bunch of math that lives in Python right now.

@mcm001
Copy link
Author

mcm001 commented Jan 5, 2024

Oh and while I have you, curious to hear any feedback you have on our docs. They're intentionally pretty sparse (since your walk-though is a much better resource! And once people have a cameramodel, they can go investigate using the usual mrcal tooling), but I want people to have a good idea of what they should be taking away from looking at graphs & how they can actually improve their calibration images.

@dkogan
Copy link
Owner

dkogan commented Jan 5, 2024

Re: 2.4: how much mrcal-side work is it to expose what I'd need to make projection uncertainty graphs in the C API? It looks like there's a bunch of math that lives in Python right now

That's a lot of work, and I'd need to be convinced that it's worth doing at all. DEFINITELY will not happen for 2.4

@dkogan
Copy link
Owner

dkogan commented Jan 5, 2024

Re: docs. I will definitely look at them, but not just at this moment. Thanks again for working on this.

@mcm001
Copy link
Author

mcm001 commented Jan 6, 2024

My main motivation for projection uncertainty graphs was that I wanted a way to display them in Photon, which would mean they'd need to be able to be generated in some mix of our C->Java->Javascript stack. But to take a step back, all I really want is to help visualize results to help users tell if their calibration actually makes sense; if projection uncertainty isn't something that you think is helpful to show, or if only supporting visualization through mrcal's python tools are reasonable asks of users then it's fine to drop.

Otherwise, no asks from me on features for 2.4! We're working next on implementing splined stereographic models, but everything I need to make that happen is already exposed by mrcal's C API.

@dkogan
Copy link
Owner

dkogan commented Jan 7, 2024

I'm not against supporting everything in C, in principle. It's just a lot of work to do in the first place, and makes maintaining and extending the code in the future much harder. Bit by bit I AM migrating the internals to C (dense stereo processing is now possible in C only, for instance), but only for stuff that is useful to have in C and that isn't likely to change in the future. mrcal 3.0 will have a very different uncertainty quantification algorithm, so that isn't fully stable yet, and I don't want to move it to C just yet.

I think you should call the Python stuff from C. You can either talk to a child Python process, or use the Python C API (which is quite good). This sounds weird, but you won't do this very much, so the performance cost should be low, and it should be relatively straightforward to do.

@dkogan
Copy link
Owner

dkogan commented Jan 7, 2024

I just looked at the calibration docs. They're good. The biggest complaint I have is that it's a LOT of information, with only some of it being applicable, and it's the reader's job to figure out what is important. It would look intimidating to a student, I suspect.

Since there's already a LOT of material out there describing the calibration process (in the mrcal docs and elsewhere), I would simplify the photonvision docs as much as possible, with links to outside docs to provide detail.

So, for instance, when you talk about interpreting the mrcal results, I would say ONLY that it can provide detailed feedback, and I would show the uncertainty plot, and I'd say "for more info, click here".

I think you're talking about 3 different methods of computing a calibration: calibdb.net, photonvision, mrcal. I think this document would be more useful, if there was only ONE method described; in this case the photonvision implementation is probably the one to focus on.

And so on. Furthermore, the mrcal documentation isn't set in stone. If you think some of it could be clearer, I'm happy to change it.

@dkogan
Copy link
Owner

dkogan commented Jan 23, 2024

Hello! I just released 2.4. FYI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants