seeing-things / zwo

ZWO SDK and custom software for debugging and using it.
24 stars 4 forks source link

Replace Makefile with cmake #71

Closed bgottula closed 1 year ago

bgottula commented 1 year ago

Some specific things to look at:

Things I still need to do:

jgottula commented 1 year ago

Okay so I did release-mode builds on the old and new branches on my Arch box.


Notes

A number of essentially completely vestigial features from the capture Makefile actually come from the ASI SDK demo Makefile. Because, as it happens, the capture Makefile is merely a (heavily) evolved version of that file. Some examples of unnecessary junk that came from there:


Makefile Build

Modifications needed to make stuff work:

Compile+Link step for zwo_fixer:

g++ -std=c++17 -fno-exceptions -fno-strict-aliasing -D_GLIBCXX_USE_CXX11_ABI=0 -fdiagnostics-color=always -Wall -Wno-unused-variable -Wno-unused-function -O1 -fuse-linker-plugin -fvisibility=hidden -fvisibility-inlines-hidden -g3 -fvar-tracking-assignments -fno-omit-frame-pointer -fuse-ld=gold -shared -fPIC -fno-plt -fno-gnu-unique -rdynamic -Wl,--no-gc-sections -Wl,--no-undefined -Wl,-z,defs -lbsd -ldl -lelf -lusb-1.0 -o libzwo_fixer.so zwo_fixer.cpp

Compile+Link step for capture:

g++ src/*.cpp -o bin/capture -std=c++17 -Wall -g -D_LIN -D_GNU_SOURCE -I./include -pthread -DGLIBC_20 -DSPDLOG_SHARED_LIB -DSPDLOG_COMPILED_LIB -DSPDLOG_FMT_EXTERNAL  -O3 -m64 -lrt -lbsd -lusb-1.0 -lspdlog -pthread -lfmt  -lopencv_core -lopencv_highgui -lopencv_imgproc -I/usr/include/opencv4 -I../1.18/linux_sdk/include -L../1.18/linux_sdk/lib/x64 -l:libASICamera2.so.1.18 -I../zwo_fixer -Wl,-rpath,../zwo_fixer -Wl,-rpath,$BASE/zwo_fixer -L../zwo_fixer -l:libzwo_fixer.so

CMake Build

Modifications needed to make stuff work:

in capture/src/CMakeLists.txt:

target_compile_options(capture PRIVATE -DSPDLOG_FMT_EXTERNAL) # this is almost 100% guaranteed the wrong way to do a CPP def

...

target_link_libraries(capture PRIVATE fmt::fmt)


Note: I've substituted `$BASE` for the git repo base dir to shorten up absolute paths.

Compile step for zwo_fixer:

c++ -O3 -DNDEBUG -Wall -MD -MT zwo_fixer/CMakeFiles/libzwo_fixer.dir/zwo_fixer.cpp.o -MF CMakeFiles/libzwo_fixer.dir/zwo_fixer.cpp.o.d -o CMakeFiles/libzwo_fixer.dir/zwo_fixer.cpp.o -c $BASE/zwo_fixer/zwo_fixer.cpp


~~Link~~ Archive step for zwo_fixer:

ar qc liblibzwo_fixer.a CMakeFiles/libzwo_fixer.dir/zwo_fixer.cpp.o ranlib liblibzwo_fixer.a


Compile step for capture:

c++ -DFMT_SHARED -DSPDLOG_COMPILED_LIB -DSPDLOG_FMT_EXTERNAL -DSPDLOG_SHARED_LIB -I$BASE/capture/src/../include -I$BASE/capture/src/../../zwo_fixer -isystem /usr/include/libusb-1.0 -isystem /usr/include/opencv4 -O3 -DNDEBUG -Wall -MD -MT src/CMakeFiles/capture.dir/$FILE.cpp.o -MF CMakeFiles/capture.dir/$FILE.cpp.o.d -o CMakeFiles/capture.dir/$FILE.cpp.o -c $BASE/capture/src/$FILE.cpp


Link step for capture:

c++ -O3 -DNDEBUG CMakeFiles/capture.dir/agc.cpp.o CMakeFiles/capture.dir/camera.cpp.o CMakeFiles/capture.dir/capture.cpp.o CMakeFiles/capture.dir/disk.cpp.o CMakeFiles/capture.dir/Frame.cpp.o CMakeFiles/capture.dir/preview.cpp.o CMakeFiles/capture.dir/SERFile.cpp.o -o ../capture /usr/lib/libopencv_highgui.so.4.5.3 /usr/lib/libspdlog.so.1.11.0 -lASICamera2 ../zwo_fixer/liblibzwo_fixer.a /usr/lib/libusb-1.0.so /usr/lib/libbsd.so /usr/lib/libopencv_videoio.so.4.5.3 /usr/lib/libopencv_imgcodecs.so.4.5.3 /usr/lib/libopencv_imgproc.so.4.5.3 /usr/lib/libopencv_core.so.4.5.3 /usr/lib/libfmt.so.9.1.0 -ldl



---

### Relevant Differences

- libzwo_fixer -> liblibzwo_fixer (derp)
- libzwo_fixer not properly built as a dynamic library (aka shared object)

Many compiler/linker flags were dropped that were actually fairly important.

General:
- `-std=c++17` dropped: odd; perhaps CMake is being clever and knows that `-std=gnu++17` is the default on recent GCC versions...?

zwo_fixer:
- `-shared -fPIC` dropped: these flags make the thing be a shared library
- `-O1` replaced by `-O3`: frankly the fixer does not need to go super fast; it's more important that it be debuggable
- `-DNDEBUG` added: would render inert any runtime assertions; fortunately I wasn't relying on any
- `-g3` dropped: I am pretty sure you want debug info enabled sir
- `-fvar-tracking-assignments -fno-omit-frame-pointer` dropped: these were there to increase debuggability
- `-fno-strict-aliasing` dropped: if this is gone, then when I do certain sane-but-undefined-behavior things, the compiler is free to do fun stuff like completely optimize out intentional assignments to memory
- `-rdynamic -Wl,--no-gc-sections -Wl,--no-undefined -Wl,-z,defs` dropped: all rather important aspects of how the library is linked
- `-D_GLIBCXX_USE_CXX11_ABI=0` dropped: actually pretty important when interfacing with another binary (like libASICamera2) that is built with older GCC versions and uses the old pre-GCC5 ABI for e.g. `std::string`
- `-fvisibility=hidden -fvisibility-inlines-hidden` dropped: these are just fundamentally a good idea for a number of reasons (not exporting internal stuff; not kneecapping performance by forcing the compiler to do even _internal_ references via the GOT and/or PLT)
- `-fno-plt` dropped: a non-essential but basically-free small perf boost
- `-fno-gnu-unique` dropped: not essential, but I have a grudge against `STB_GNU_UNIQUE`, probably acquired back when it was new and binutils didn't have full support for it
- `-Wno-unused-variable -Wno-unused-function` dropped: you'll likely get annoying compiler warnings without these
- `-lusb-1.0` dropped: best to keep the fixer itself explicitly linked to libusb rather than assuming the program will always transitively do so for it
- `-lelf` dropped: technically not needed; but I had WIP stuff that was going to use libelf
- `-fno-exceptions` dropped: debatable

capture:
- `-g` dropped: I am pretty sure you want debug info enabled sir
- `-D_GNU_SOURCE` dropped: you probably do want this; various libc calls become more restrictive without it (man pages frequently mention it)
- `-Wl,-rpath` dropped: without this, if zwo_fixer is properly built as a shared library, capture won't know where to find it unless you install it to /usr/lib/ or use some `LD_LIBRARY_PATH` garbage
- relative include/lib paths to ASI SDK dropped: not necessarily a bad thing, but it means that the system installation of libASICamera2 is now used (and required), which is a bit different from before
bgottula commented 1 year ago

Thanks for the thorough review. Responding to a few specific items:

libzwo_fixer not properly built as a dynamic library (aka shared object)

Why is a static library not "proper"? This is easy to change, but you still haven't explained what's wrong with static linkage.

-O1 replaced by -O3: frankly the fixer does not need to go super fast; it's more important that it be debuggable -fno-plt dropped: a non-essential but basically-free small perf boost

Pick a lane. Also, you can ask for Debug or Release builds with CMake (e.g., cmake -D CMAKE_BUILD_TYPE=Debug), but you're generally not supposed to force it to always use an ultra exacting set of optimization flags. What's the point of even using CMake if you're going to just override everything it does for you.

-g3 dropped: I am pretty sure you want debug info enabled sir

No, I want CMake to take care of that for me. The default build mode is "Release." If I want debug info I can ask for a "Debug" build. Or if I really want to pass -g3 to the compiler I can use cmake -D CMAKE_CXX_FLAGS="-g3".

I'll fix up a few things here soon.

Since zwo_fixer is essentially your project, would you please propose how you'd like to restore all of the compiler arguments you think ought to be present? I leave it up to you whether to add them in a compiler-independent way or just hard-code the gcc-specific ones. Frankly, I'm leaning towards just reverting to the Makefile for zwo_fixer since CMake is just getting in the way if we want to micro-manage the compiler flags ourselves.

bgottula commented 1 year ago

Relevant if we want capture's CMake to build zwo_fixer with the old Makefile: https://stackoverflow.com/a/30146692

jgottula commented 1 year ago

Seems pretty reasonable.