search

CarVac / librtprocess

A project to make RawTherapee's processing algorithms more readily available.
Other
53 stars 24 forks source link

Use modern cmake #37

Closed cryptomilk closed 5 years ago

cryptomilk commented 5 years ago

Hi,

this is work in progress. I've started to clean up cmake for librtprocess. This is not ready yet.

cryptomilk commented 5 years ago

Should be working now.

heckflosse commented 5 years ago

@cryptomilk Andreas, thanks a lot :+1: Builds fine on Windows7. Let's wait for confirmation on Linux. ping @CarVac

cryptomilk commented 5 years ago

If you wonder what do to with those flags e.g. for AddressSanitzer or UndefinedSanitizer, see https://gitlab.com/cmocka/cmocka/pipelines/49122087 for example.

You should add unit tests to make sure the stuff really works and to find bugs.

cryptomilk commented 5 years ago

Also someone should fix the build warnings ;-)

heckflosse commented 5 years ago

@cryptomilk

You should add unit tests to make sure the stuff really works and to find bugs.

Unit tests would be good for new implementations and improvements to the current implementations. Imho the current initial implementations have got enough tests in RawTherapee the last years to be a good base for unit tests ;-)

heckflosse commented 5 years ago

@cryptomilk

Trying to build your 'master-cmake' branch I get this (Windows)

$ cmake -G "MSYS Makefiles" -DCMAKE_INSTALL_PREFIX="$MSYSTEM_PREFIX" ..
CMake Error at src/CMakeLists.txt:49 (install):
  install Library TARGETS given no DESTINATION!

-- Configuring incomplete, errors occurred!
See also "Z:/H2/librt_crypto/build/CMakeFiles/CMakeOutput.log".
heckflosse commented 5 years ago

@cryptomilk When I wrote above 'Builds fine on windows', I was on the master branch of your fork by accident. master-cmake does not build on Windows

fcomida commented 5 years ago

@cryptomilk @heckflosse it works on fedora, just one issue: the final pkgconfig file must be rtprocess.pc, not rtprocessin.pc

cryptomilk commented 5 years ago

I will fix the pkg-config and check what to do with Windows. CPack is still missing.

cryptomilk commented 5 years ago

A few more updates and fixes ...

heckflosse commented 5 years ago

@cryptomilk Andreas, I made a fresh clone of your fork and did:

git checkout master-cmake
$ cmake -G "MSYS Makefiles" -DCMAKE_INSTALL_PREFIX="$MSYSTEM_PREFIX" ..
-- The CXX compiler identification is GNU 8.3.0
-- Check for working CXX compiler: C:/msys64/mingw64/bin/g++.exe
-- Check for working CXX compiler: C:/msys64/mingw64/bin/g++.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Could NOT find NSIS (missing: NSIS_MAKE)
-- Found PkgConfig: C:/msys64/mingw64/bin/pkg-config.exe (found version "0.29.2")
-- Checking for module 'glibmm-2.4'
--   Found glibmm-2.4, version 2.58.0
-- Found OpenMP_CXX: -fopenmp (found version "4.5")
-- Found OpenMP: TRUE (found version "4.5") found components:  CXX
-- Configuring done
WARNING: Target "rtprocess" requests linking to directory "C:/msys64/mingw64/lib".  Targets may link only to libraries.  CMake is dropping the item.
-- Generating done
-- Build files have been written to: Z:/H2/librt_crypto/build
heckflosse commented 5 years ago

@cryptomilk Andreas, on Windows your changes now create a librtprocess.dll while the former code created a file for static linkage. We should discuss what is intended.

cryptomilk commented 5 years ago

@heckflosse Why do you want a static lib on Windows?

heckflosse commented 5 years ago

@cryptomilk

Why do you want a static lib on Windows?

I didn't say I want a static lib on Windows. But I would like to hear what maintainers of for example RT windows build think. For my RT builds (which are only for my personal use) it makes no difference.

ping @gaaned92

cryptomilk commented 5 years ago

@heckflosse Could you try building again with MSYS. I haven't had the time to install it in a VM yet. Or I will just use gitlab to cross-compile.

heckflosse commented 5 years ago

@cryptomilk I will check before midnight

cryptomilk commented 5 years ago

On which day? :-)

heckflosse commented 5 years ago

Oh, sorry. Completely forgot. Builds fine on Win7/msys

cryptomilk commented 5 years ago

So we're good to go? Now it would be nice to run with AddressSanitzer and UndefinedSanitizer but without any tests that's not possible ...

cryptomilk commented 5 years ago

It starts to bitrot ...