GreatAttractor / imppg

ImPPG (Image Post-Processor)
GNU General Public License v3.0
86 stars 10 forks source link

macOS build: CXXFLAGS needed (for me) on macOS Monterey 12.5.1 and Xcode 13.4.1 #12

Closed ftclausen closed 2 years ago

ftclausen commented 2 years ago

Summary: I needed to set CXXFLAGS="-I/usr/local/include -std=c++17" prior to building on macOS (with Homebrew dependencies) otherwise it failed due to incorrect C++ standard and also boost was not found.

Details:

I amended the steps as follows and successfully built ImPPG on the macOS + Xcode outlined in issue subject

$ mkdir build
$ cd build
$ CXXFLAGS="-I/usr/local/include -std=c++17" cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Release ..
$ make -j8
$ make install
$ imppg

Is this worth mentioning in the README? Of course this env var may not be needed in older (or future newer) versions. I can make a PR if so.

GreatAttractor commented 2 years ago

Thanks for the info. As for the -std=c++17, I guess this block in the root CMakeLists.txt should be fixed instead. Could you try if something like this works:

if (CMAKE_VERSION VERSION_LESS "3.1")
    if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17")
    endif()
else()
    set_property(TARGET imppg PROPERTY CXX_STANDARD 17)
    set_property(TARGET imppg PROPERTY CXX_STANDARD_REQUIRED ON)
endif()

BTW, you're using CMake older than 3.1, is that correct?

ftclausen commented 2 years ago

Regarding the CMake version: I am using Homebrew's version which is 3.24.1 (higher than 3.1) so it is strange that it takes the less than 3.1 path. Perhaps that is the root cause although I don't know why it thinks it is less than 3.1.

With regards to the CMakeLists.txt changes I tried the above and also CMAKE_CXX_COMPILER_ID MATCHES "Clang" but it doesn't appear to work and it gives convolution.h:39:1: error: unknown type name 'constexpr' implying the wrong C++ standard is being used.

Happy to try other experiments though.

GreatAttractor commented 2 years ago

OK, that's my fault - since commit db190eb635 there are now several CMake sub-targets, and I forgot to set the C++ standard for them (I haven't noticed so far, because I use GCC 11, which defaults to C++17). I'll fix that, in the meantime can you check if the previous commit d0285510a6aed builds for you?

ftclausen commented 2 years ago

With d0285510a6a it all builds fine, thanks. As you say it must be the sub-targets needing the C++ standard then.

PS: I found ImgPPG via this video by Dylan O'Donnal (humorous Australian astro imager) and, just from a quick session, it has already much improved my Solar images when reprocessing some previously acquired data.

GreatAttractor commented 2 years ago

Great, I'm glad you like it!

Try the latest master, it should be fixed now.

ftclausen commented 2 years ago

The C++ standard issues are indeed fixed, thanks for your prompt help. I still have to include CXXFLAGS="-I/usr/local/include" however that could be just on my machine. I will try on some other machine to see if it also happens there.

ftclausen commented 2 years ago

With regard to the remaining CXXFLAGS="-I/usr/local/include": I found that the isystem compiler argument for the Homebrew installation was not being used for the "image" subcomponent (this was an issue on my other laptop too). The following fixed it for me:

diff --git a/src/image/CMakeLists.txt b/src/image/CMakeLists.txt
index 2bcfdf21f0e..67cda2227d1 100644
--- a/src/image/CMakeLists.txt
+++ b/src/image/CMakeLists.txt
@@ -16,6 +16,7 @@ include(../../utils.cmake)
 set_cpp_standard(image)

 target_include_directories(image PUBLIC include)
+target_include_directories(image PRIVATE ${Boost_INCLUDE_DIRS})

 target_link_libraries(image PRIVATE common)

I am not very familiar with CMake so there is probably a better way to do this.

GreatAttractor commented 2 years ago

You're right, we need this for all subcomponents which use Boost. The change you suggest is OK, also this line needs a ${Boost_INCLUDE_DIRS} added. Please go ahead and open a pull request.