search

johnmcfarlane / cnl

A Compositional Numeric Library for C++
Boost Software License 1.0
644 stars 62 forks source link

Header only conan packaging and less warnings #917

Closed hbe72 closed 3 years ago

hbe72 commented 3 years ago

Hi John,

Few suggestions to clear warnings of missing dependencies, feel free to drop or use ;)

In addition I attach a suggestion to create conan-center-recipe for CNL. I think it would be worth while to have conan package for CNL available somewhere... conan-center-recipe-cnl.tar.gz

Attached also the patch and recipe I created for getting that one additional bit for negative maximum, for a public repo you may want to drop that patch as it is not tested and probably not compatible with full library.

johnmcfarlane commented 3 years ago

Hi Heikki, can you tell me what warnings you are experiencing?

hbe72 commented 3 years ago

I was experimenting with conan-center-index recipe (attached to the PR) for header only installation. The dependencies to gtest and benchmark are unnecessary just for using the library.

This is essentially how I have ended up using CNL in my experiments. I use this recipe to push header only installation of CNL to local artifactory instance and in my project I just add "cnl/v2.0.1-dev-nm" dependency to a conanfile.py.

In that recipe I dropped those dependencies. Then CMAKE will complains:

CMake Warning at test/CMakeLists.txt:36 (find_package):
  By not providing "Findbenchmark.cmake" in CMAKE_MODULE_PATH this project
  has asked CMake to find a package configuration file provided by
  "benchmark", but CMake did not find one.

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

    benchmarkConfig.cmake
    benchmark-config.cmake

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

It would be nice for the build to be clean of all warnings even though this really does not matter.

Therefore, the strategy should be adopted directly in CNL conanfile.py, as only if the tests are built these dependencies are required and only then the test subdirectory should included. Maybe the "CMAKE_SKIP_INSTALL_ALL_DEPENDENCY" flag does not have the "skip the tests" semantics, but it was already available and depends only on test option.

Everything else in the PR, as compiler version checking etc. can be useful to immediately stop the build or packaging if the requested settings are not compatible with library. CMake can do these checkings as well.

I see a lot broken builds now. At the time I just had quick look. I did not really understand why conan was not able to load gtest recipe as from the perspective of test build nothing really changed.

hbe72 commented 3 years ago

I could take the time to "finally" fix CDSP examples to use main of CNL. I would prefer to have the CNL to be available somewhere as a conan package. CMake external project as the fallback option.

johnmcfarlane commented 3 years ago

I'm not sure about pipeline breaks. The CI may have rotted. Does main still build OK?

Re. the test dependencies, they're a bother but do they block anything? Have you looked at #870? There should be a clean way to address this that doesn't add a layer of complexity over the build scripts. When asking the Conan devs, that's the direction they pointed me in.

On Mon 27 Sep 2021, 08:01 Heikki Berg, @.***> wrote:

I could take the time to "finally" fix CDSP examples to use main of CNL. I would prefer to have the CNL to be available somewhere as a conan package. CMake external project as the fallback option.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/johnmcfarlane/cnl/pull/917#issuecomment-927586674, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTGNYE2V3LBRQYGOBMDHTUEAJEFANCNFSM5EVO5SIQ .

hbe72 commented 3 years ago

Yes, main builds ok for me at home. In CI I suspect the main to have same problem as this build experiences.

Yes this PR addresses #870, thus drops the unnecessary dependencies from the build. Nothing is broken, but this PR just clears some warnings, so please consider this as suggestion.

johnmcfarlane commented 3 years ago

@hbe72 #919 removes the warnings and #921 fixes the pipeline. I don't think #870 is addressed yet but I'd like to explore a solution involving build requirements. (This avenue was suggested to me directly by JFrog developers during a meetup earlier this year.) I hope a simpler solution can be found which maintains the declarative style of conanfile.py.