Add cmake support for the c code. #38

phcerdan · 2017-09-24T09:45:35Z

Add a CMakeLists.txt file in src/ folder to compile the c code into a library. CMake is specially useful for integrating the code with other projects using cmake.

Also, Add dummy test checking if linking works properly.

cmake ../proxTV/src -DCMAKE_INSTALL_PREFIX=./install -DENABLE_TESTING:BOOL=ON

albarji · 2017-09-24T17:41:35Z

@phcerdan thanks for the pull request! This project is currently lacking of a convenient way to generate a C library of the core algorithms, so this might be useful.

However I have never used cmake. Could you give me an example on how the library is compiled, then linked into the compilation of the test program you have added? I might want to include this process both in the docs and in the CI tests.

I have tried the cmake line you wrote above but I'm unsure about from which directory one is supposed to run it. If I just run

cmake src -DCMAKE_INSTALL_PREFIX=./install -DENABLE_TESTING:BOOL=ON

at the project root folder I get the following error:

CMake Error at CMakeLists.txt:39 (find_package):
  By not providing "FindLAPACKE.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "LAPACKE", but
  CMake did not find one.

  Could not find a package configuration file provided by "LAPACKE" with any
  of the following names:

    LAPACKEConfig.cmake
    lapacke-config.cmake

  Add the installation prefix of "LAPACKE" to CMAKE_PREFIX_PATH or set
  "LAPACKE_DIR" to a directory containing one of the above files.  If
  "LAPACKE" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!
See also "/home/alvaro/Investigacion/proxTVphcerdan/CMakeFiles/CMakeOutput.log".
See also "/home/alvaro/Investigacion/proxTVphcerdan/CMakeFiles/CMakeError.log".

I'm think I have the correct lapack and lapacke libraries installed, as the python installation works alright.

phcerdan · 2017-09-25T05:20:48Z

Hi @albarji, glad that you find it useful too!
Check my latest commit that solves the Lapacke issue and generates a proxTVConfig.cmake for external projects to use the generated c-library pretty easily.

The Lapacke issue (that I wasn't having in my system) is because only the latest versions of lapacke provide a lapackeConfig.cmake. For older versions, I have included a reliable FindLAPACKE.cmake from here in a new cmake folder, that handles finding lapacke. Please let me know if that solves the problem.

I have written a c-interface in the readme, let me know if I should put it after the python/matlab interfaces, or if you find a way to structure it better.

To run cmake, it is recommended to create a build folder in parallel to the project folder ( so not inside the proxTV, to avoid version-controlling it, and other issues). Check the Readme and let me know if that helps.

I have also created a dummy_project to test if the generated libproxTV (and its proxTVConfig.cmake) can be found from an external cmake project, and it does!. See this.

Also, I have reorganized the CMakeFiles, the main one is now living in the top folder (not in src).

albarji · 2017-10-01T09:20:41Z

Thanks @phcerdan ! I'm currently tangled up in other projects, but will take a more thorough look into this once I find the time.

phcerdan · 2017-11-12T22:01:41Z

@albarji I will use my branch to use the c-interface of proxTV to other cmake project, so no hurries.

phcerdan · 2018-05-02T14:35:59Z

Hi @albarji, I asked in the cpp-slang channel for a review of the code, to be in the safe side, and correcting minor things. I would appreciate if you test it with your Lapacke version (modern versions of that library generate cmake code, so FindLAPACKE is not needed).

albarji · 2018-05-20T10:22:24Z

Hey @phcerdan , I have to apologize for my lack of answers. In all honesty, I have been neglecting this project for all these months, in favor of other ones I'm developing right now. I hope I can find some time during the summer to properly test this and merge your additions!

phcerdan · 2018-05-24T20:54:07Z

@albarji I understand, I don't have much time either lately in the project I wanted to do using this library. But let's try to find some, from your side probably you only need to give a try and perform a couple of tests! I think the cmake files should be good enough, and if problems arise, we can tweak it. Let me know if I can help further.
Saludos!

albarji · 2018-07-31T10:51:10Z

Hi again @phcerdan , finally found time to check this out.

I have tried following the instructions you added to the README.md file, but still to no success. This is the error I get back when running cmake:

CMake Error: Could not find cmake module file: CMakeDetermineDESCRIPTIONCompiler.cmake
CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly.
Missing variable is:
CMAKE_DESCRIPTION_COMPILER_ENV_VAR
CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly.
Missing variable is:
CMAKE_DESCRIPTION_COMPILER
CMake Error: Could not find cmake module file: /home/alvaro/tmp/build/CMakeFiles/3.5.1/CMakeDESCRIPTIONCompiler.cmake
CMake Error: Could not find cmake module file: CMakeDetermineToolbox for fast Total Variation proximity operatorsCompiler.cmake
CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly.
Missing variable is:
CMAKE_Toolbox for fast Total Variation proximity operators_COMPILER_ENV_VAR
CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly.
Missing variable is:
CMAKE_Toolbox for fast Total Variation proximity operators_COMPILER
CMake Error: Could not find cmake module file: /home/alvaro/tmp/build/CMakeFiles/3.5.1/CMakeToolbox for fast Total Variation proximity operatorsCompiler.cmake
-- The C compiler identification is GNU 5.4.0
-- The CXX compiler identification is GNU 5.4.0
CMake Error at CMakeLists.txt:7 (project):
  No CMAKE_DESCRIPTION_COMPILER could be found.

  Tell CMake where to find the compiler by setting the CMake cache entry
  CMAKE_DESCRIPTION_COMPILER to the full path to the compiler, or to the
  compiler name if it is in the PATH.


CMake Error: Could not find cmake module file: CMakeDESCRIPTIONInformation.cmake
CMake Error at CMakeLists.txt:7 (project):
  No CMAKE_Toolbox for fast Total Variation proximity operators_COMPILER
  could be found.

  Tell CMake where to find the compiler by setting the CMake cache entry
  CMAKE_Toolbox for fast Total Variation proximity operators_COMPILER to the
  full path to the compiler, or to the compiler name if it is in the PATH.


CMake Error: Could not find cmake module file: CMakeToolbox for fast Total Variation proximity operatorsInformation.cmake
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error: CMAKE_DESCRIPTION_COMPILER not set, after EnableLanguage
CMake Error: CMAKE_Toolbox for fast Total Variation proximity operators_COMPILER not set, after EnableLanguage
-- Configuring incomplete, errors occurred!
See also "/home/alvaro/tmp/build/CMakeFiles/CMakeOutput.log".

I have been googling for the errors appearing on top of the trace, but I only found answers regarding badly configured CMakeLists.txt. This does not seem to be the case, but then again I have no prior experience with cmake. Any idea what could be wrong?

phcerdan · 2018-07-31T11:35:36Z

Wierd error indeed. I will test it again once I am on a computer. But meanwhile, from my prior experience with CMake, I have to ask: are you testing it in a new build folder or using the same folder you were using months ago? If the second, try to clean the build the folder and start again.

albarji · 2018-07-31T11:51:29Z

I started from a new checkout from github, and the build folder I'm using is a new one.

phcerdan · 2018-07-31T13:09:25Z

I cannot reproduce it, I can configure without problems. It might be related to the cmake version, and the LANGUAGES option in project. I have removed that line, because C and CXX are enabled by default anyway. Please try again!

Related: jasper-software/jasper#161

albarji · 2018-07-31T16:49:47Z

No luck, I'm afraid

CMake Error at CMakeLists.txt:7 (project):
  project with VERSION must use LANGUAGES before language names.


-- Configuring incomplete, errors occurred!

But I did a little bit more testing and it seems it's a problem with my environment. I tried in a fresh Ubuntu container and after installing build-essential, libopenblas-dev, liblapack-dev and liblapacke-dev I get the following

-- proxTV version: 3.2.1
-- LAPACKE_SEARCH_PATHS: LAPACKE_DIR-NOTFOUND;/usr;/usr/local;/usr/local/opt/lapack;/opt/LAPACKE
CMake Error at /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find LAPACKE (missing: CBLAS_LIB)
Call Stack (most recent call first):
  /usr/share/cmake-3.10/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  cmake/FindLAPACKE.cmake:119 (find_package_handle_standard_args)
  CMakeLists.txt:21 (find_package)


-- Configuring incomplete, errors occurred!
See also "/build/CMakeFiles/CMakeOutput.log".
See also "/build/CMakeFiles/CMakeError.log".

So for some reason cmake can't find LAPACKE.

To avoid this kind of environment-dependent issues I implemented in the past some automated tests in Travis, which you can check here. I think the easiest way to sort this is to add some tests there that run the cmake compilation and the C test program you prepared. This way we will be sure that your additions work as expected.

I just modified my Travis configuration so that further commits into this push request will go through the tests automatically. Could you add the tests to your branch? I guess modifying .travis.yml to look as follows should work:

language: python
python:
  - "2.6"
  - "2.7"
  - "3.3"
  - "3.4"
  - "3.5"
  - "3.6"
os: linux
dist: trusty
addons:
    apt:
        packages:
        - libopenblas-dev
        - liblapack-dev
        - liblapacke-dev
        - cmake
install:
    - pip install coveralls
    - python setup.py install
    - mkdir build && cd build && cmake ../  # THIS SHOULD COMPILE YOUR C API
script: 
    - nosetests --with-coverage --cover-package=prox_tv
    - # WHATEVER YOU NEED TO COMPILE AND RUN YOUR C EXAMPLE
after_success:
    - coveralls

phcerdan · 2018-07-31T17:04:27Z

That's probably a Ubuntu bug: https://bugs.launchpad.net/ubuntu/+source/openblas/+bug/1728068

But should we able to work-around it, related: opencv/opencv#9953 (comment)

phcerdan · 2018-07-31T23:10:26Z

Umm, I think the best would be to require a modern lapacke version (3.6.1 at least), so we can forget about maintaining the homemade FIND_LAPACKE we were using.

I will try to set-up the travis as you suggested.

coveralls · 2018-07-31T23:17:36Z

Coverage remained the same at 93.353% when pulling e988a70 on phcerdan:cmake_support into 15b57a8 on albarji:master.

coveralls · 2018-07-31T23:17:37Z

Coverage remained the same at 93.353% when pulling e988a70 on phcerdan:cmake_support into 15b57a8 on albarji:master.

phcerdan · 2018-08-01T00:41:58Z

Uff, travis use trusty (2014) and it is just getting dated for the lapacke purposes.
The current Find_LAPACKE tries to find cblas.h... but trusty libopenblas-dev doesn't incude it. In xenial, it does.

I'll keep trying soon, maybe even with a docker version for travis.

albarji · 2018-08-01T15:08:12Z

I have been checking the travis docs, and indeed the "newest" version supported is trusty. But I can change the travis script to run the tests inside a docker container with xenial. Let me try that and I will tell you whether it works out.

albarji · 2018-08-01T18:15:07Z

@phcerdan I created a branch with a tuned .travis.yml that makes use of Docker containers to run under xenial. It's not pretty, and I had to drop support for the oldest python versions, but it works. Check it out at https://github.com/albarji/proxTV/blob/feature/travis-xenial/.travis.yml . If this works for you too then I will merge it to master.

albarji · 2018-10-20T10:42:04Z

@phcerdan I just merged a big change in the way the library is built and tested. Now I create precompiled python wheels for Linux and Mac, and upload them to PyPI for easier install. For building the Linux wheels in a way that is compatible with all linux distributions I need to run the tests inside a CentOS 5 container, so now I'm running all tests there. I'm unsure whether this makes things easier or harder for the C compilation and testing.

phcerdan · 2018-10-20T13:04:43Z

@albarji that's great! Thanks for letting me know. I will try to update this branch accordingly.
By the way, not sure if it is intended to show this line or not.

albarji · 2018-10-21T08:34:36Z

That line is the RSA-encrypted password used to upload the package to PyPI. The encryption is made using Travis public key for this repository, so only Travis jobs on this very same repo can access the private key to unencrypt the password and make use of it. This seems to be the standard way of dealing with secrets in public repositories: https://docs.travis-ci.com/user/environment-variables#defining-encrypted-variables-in-travisyml

phcerdan · 2018-10-21T22:47:14Z

Cool, it was the first time I was seeing that in a travis file, so, better to check with you it wasn't sensible info. Thanks for the explanation!

@jcfr

Add dummy test checking if linking works properly. Add FindLAPACKE.cmake and export proxTVconfig.cmake. FindLAPACKE.cmake is used to handle lapacke installations with no export targets. Also complete the generation of proxTVconfig.cmake Add a dummy project to test usage of proxTV from external project. Add C Interface to Readme Remove Foo_FOUND in the case FOO is REQUIRED Update cmake required version to 3.5 This simplifies policy and version set up. Require a modern lapacke and remove FIND_LAPACKE This will look for a `lapacke-config.cmake`, that is generated since lapacke version 3.6. And allow us to remove the buggy FIND_LAPACKE. Add test for cmake in travis. Restore FindLAPACKE.cmake Because travis still uses ubuntu 14.04, with an old lapacke which does not provide config.cmake. Fix CMake Added FindLAPACK FindLAPACKE CONFIG is not populating LAPACK. From @jcfr branch: https://github.com/jcfr/proxTV/tree/cmake_support_squashed Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]> WIP: Avoid deploy in .travis Also install cmake with pip (3.12) Also use find_package(LAPACKE NOCONFIG) for old librart versions Error: ``` +twine upload --repository-url https://test.pypi.org/legacy/ --username albarji dist/prox_tv-3.3.0-cp27-cp27mu-manylinux1_x86_64.whl Enter your password: No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself. Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received ```

dzenanz · 2019-07-15T19:11:35Z

What is the status of this PR?

phcerdan · 2019-07-16T06:43:46Z

Development of this feature (and the option to use Eigen instead of Blas/Lapack) is going on in my fork on this branch: https://github.com/phcerdan/proxTV/tree/use_eigen

dzenanz · 2019-07-16T13:00:09Z

Maybe I should rephrase the question: @albarji do you have any intention of merging this PR?

phcerdan force-pushed the cmake_support branch 2 times, most recently from 7661cd0 to aeaa2c6 Compare September 25, 2017 05:18

phcerdan force-pushed the cmake_support branch from aeaa2c6 to c02cfe1 Compare September 25, 2017 08:44

phcerdan force-pushed the cmake_support branch from 6b171a2 to 671889a Compare July 31, 2018 13:08

phcerdan force-pushed the cmake_support branch from 671889a to ca4fbba Compare July 31, 2018 13:17

phcerdan force-pushed the cmake_support branch 5 times, most recently from 600d2b9 to faf5c3a Compare August 1, 2018 00:32

phcerdan force-pushed the cmake_support branch from 92dcb04 to d4f6a77 Compare August 2, 2018 14:54

phcerdan force-pushed the cmake_support branch 3 times, most recently from 6147a8d to 7a82d57 Compare August 2, 2018 17:53

phcerdan force-pushed the cmake_support branch 3 times, most recently from 56d7260 to 96e4aa4 Compare November 3, 2018 03:21

phcerdan force-pushed the cmake_support branch 2 times, most recently from 2ed4393 to b0f34a2 Compare January 15, 2019 03:20

phcerdan mentioned this pull request Jan 15, 2019

Add Eigen3 as an optional alternative to Lapack #51

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cmake support for the c code. #38

Add cmake support for the c code. #38

phcerdan commented Sep 24, 2017

albarji commented Sep 24, 2017

phcerdan commented Sep 25, 2017 •

edited

Loading

albarji commented Oct 1, 2017

phcerdan commented Nov 12, 2017 •

edited

Loading

phcerdan commented May 2, 2018

albarji commented May 20, 2018

phcerdan commented May 24, 2018

albarji commented Jul 31, 2018

phcerdan commented Jul 31, 2018

albarji commented Jul 31, 2018

phcerdan commented Jul 31, 2018

albarji commented Jul 31, 2018

phcerdan commented Jul 31, 2018 •

edited

Loading

phcerdan commented Jul 31, 2018

coveralls commented Jul 31, 2018

coveralls commented Jul 31, 2018 •

edited

Loading

phcerdan commented Aug 1, 2018 •

edited

Loading

albarji commented Aug 1, 2018

albarji commented Aug 1, 2018

albarji commented Oct 20, 2018

phcerdan commented Oct 20, 2018

albarji commented Oct 21, 2018

phcerdan commented Oct 21, 2018

dzenanz commented Jul 15, 2019

phcerdan commented Jul 16, 2019

dzenanz commented Jul 16, 2019

Add cmake support for the c code. #38

Are you sure you want to change the base?

Add cmake support for the c code. #38

Conversation

phcerdan commented Sep 24, 2017

albarji commented Sep 24, 2017

phcerdan commented Sep 25, 2017 • edited Loading

albarji commented Oct 1, 2017

phcerdan commented Nov 12, 2017 • edited Loading

phcerdan commented May 2, 2018

albarji commented May 20, 2018

phcerdan commented May 24, 2018

albarji commented Jul 31, 2018

phcerdan commented Jul 31, 2018

albarji commented Jul 31, 2018

phcerdan commented Jul 31, 2018

albarji commented Jul 31, 2018

phcerdan commented Jul 31, 2018 • edited Loading

phcerdan commented Jul 31, 2018

coveralls commented Jul 31, 2018

coveralls commented Jul 31, 2018 • edited Loading

phcerdan commented Aug 1, 2018 • edited Loading

albarji commented Aug 1, 2018

albarji commented Aug 1, 2018

albarji commented Oct 20, 2018

phcerdan commented Oct 20, 2018

albarji commented Oct 21, 2018

phcerdan commented Oct 21, 2018

dzenanz commented Jul 15, 2019

phcerdan commented Jul 16, 2019

dzenanz commented Jul 16, 2019

phcerdan commented Sep 25, 2017 •

edited

Loading

phcerdan commented Nov 12, 2017 •

edited

Loading

phcerdan commented Jul 31, 2018 •

edited

Loading

coveralls commented Jul 31, 2018 •

edited

Loading

phcerdan commented Aug 1, 2018 •

edited

Loading