DeadSix27 / waifu2x-converter-cpp

Improved fork of Waifu2X C++ using OpenCL and OpenCV
MIT License
792 stars 86 forks source link

misc: move to stable std::filesystem #207

Closed DeadSix27 closed 4 years ago

DeadSix27 commented 5 years ago

Fixes build error with latest MSVC version

Please test these changes on x86(_64) Linux, I was only able to test on Raspberry Pi3 Arch-ARM and Windows 10/MSCV-latest

Resolves #206 due to new release after this gets merged.

kattjevfel commented 5 years ago

Compiled this on Arch and used it on a few images both with and without TTA, didn't notice anything out of the ordinary.

Equim-chan commented 5 years ago

Note that on llvm9 (which is now the version installed by brew install llvm), -lc++fs is deprecated and both <filesystem> and <experimental/filesystem> can be used directly now. Linking with -lc++fs now throws ld: library not found for -lc++fs instead.

See https://libcxx.llvm.org/docs/UsingLibcxx.html#using-filesystem

Patch advice here:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1668437..7819868 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -74,7 +74,6 @@ if(APPLE)
    set(CMAKE_CXX_COMPILER "/usr/local/opt/llvm/bin/clang++")
    set(CMAKE_EXE_LINKER_FLAGS "-L/usr/local/opt/llvm/lib -Wl,-rpath,/usr/local/opt/llvm/lib")
    set(CMAKE_SHARED_LINKER_FLAGS "-L/usr/local/opt/llvm/lib -Wl,-rpath,/usr/local/opt/llvm/lib")
-   set(FILE_SYSTEM_LIB "c++fs")
 elseif(UNIX OR MINGW)
    set(FILE_SYSTEM_LIB "stdc++fs")
 endif()
@@ -362,7 +361,7 @@ if(WIN32) # We build FOR windows.
        target_link_libraries(w2xcr w2xc user32 shlwapi gdi32)
    endif()
 elseif(APPLE)
-   target_link_libraries(w2xc ${OPENCV_LIBRARIES} ${CMAKE_DL_LIBS} pthread c++ c++fs)
+   target_link_libraries(w2xc ${OPENCV_LIBRARIES} ${CMAKE_DL_LIBS} pthread c++)
 else() # We are on linux and build for linux
    target_link_libraries(w2xc ${OPENCV_LIBRARIES} ${CMAKE_DL_LIBS} pthread stdc++fs stdc++)
 endif()
DeadSix27 commented 5 years ago

@Equim-chan Feel free to add a PR for that based on the use-stable-filesystem branch.

EDIT: Possibly add a check for < llvm9 cases in the CMake file? I dont know how common it is for apple users to have older versions of that installed when 'homebrewing?'

_Also just noticed that FILE_SYSTEM_LIB is set for apple (and other OSs) but not used in some areas over hard-coded values..._

Equim-chan commented 5 years ago

@DeadSix27 I'm not so familiar with conditions and flows in CMake file... 😭

DeadSix27 commented 5 years ago

@Equim-chan Mind testing: https://github.com/DeadSix27/waifu2x-converter-cpp/commit/5b82487de9fb620d2061384493093cbb014a9e76 / https://github.com/DeadSix27/waifu2x-converter-cpp/pull/207.patch

Equim-chan commented 4 years ago

@DeadSix27 should use MATCHES instead of STREQUAL, see https://stackoverflow.com/a/10055571.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d9d0981..b16bb3f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -69,7 +69,7 @@ message(STATUS "System is: ${CMAKE_SYSTEM_NAME} (${LOCAL_SYS_TYPE})")
 ### ########

 ### Get binary paths for APPLE users
-if(APPLE AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+if(APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")
    set(CMAKE_C_COMPILER "/usr/local/opt/llvm/bin/clang")
    set(CMAKE_CXX_COMPILER "/usr/local/opt/llvm/bin/clang++")
    set(CMAKE_EXE_LINKER_FLAGS "-L/usr/local/opt/llvm/lib -Wl,-rpath,/usr/local/opt/llvm/lib")

After the change it now works in my environment.

DeadSix27 commented 4 years ago

Ahhh I see:

As of CMake 3.0.0 the CMAKE__COMPILER_ID value for Apple-provided Clang is now AppleClang. ...

DeadSix27 commented 4 years ago

@Equim-chan Can you test whether @YukihoAA's changes work on macOS?

Equim-chan commented 4 years ago

It works.