ublas icon indicating copy to clipboard operation
ublas copied to clipboard

Add CMakeLists.txt

Open pdimov opened this issue 2 years ago • 7 comments

uBLAS is one of the two remaining Boost libraries still missing CMake support. This adds it.

pdimov avatar Oct 28 '21 19:10 pdimov

This Pull request Passed all of clang-tidy tests. :+1:

github-actions[bot] avatar Oct 28 '21 20:10 github-actions[bot]

The CMakeLists.txt is automatically generated by boostdep --cmake numeric/ublas; boostdep takes the requirement from your meta/libraries.json.

I would recommend leaving it as-is, because (a) C++20 is too high a requirement to impose today and (b) using cxx_std_20 requires CMake 3.12, which will mean that building Boost with CMake will fail with an earlier CMake. We haven't yet set a firm minimum CMake version which to support. (It's possible that we'll end up requiring 3.12 or higher, of course.)

I have to wonder, what C++20 features are you using?

PR #94 is incompatible with the Boost CMake infrastructure and targets the use case where uBLAS is the root project. In such cases what we do is combine the two using an if statement - when not root project, use the Boost-compatible CML, if root project, use whatever the project maintainers like. We did this for several libraries, such as f.ex. GIL: https://github.com/boostorg/gil/pull/614/commits/85838cdc4e46e861e15eb9e608a6ca3f7f0147e3.

If you don't insist on having your own CML file, it's more convenient to use the boostdep output as-is, without any changes, because it makes it easier to just regenerate it if changes need to be done (which happens f.ex. when your dependency list changes.)

pdimov avatar Oct 29 '21 13:10 pdimov

@pdimov

The CMakeLists.txt is automatically generated by boostdep --cmake numeric/ublas; boostdep takes the requirement from your meta/libraries.json.

I would recommend leaving it as-is, because (a) C++20 is too high a requirement to impose today and (b) using cxx_std_20 requires CMake 3.12, which will mean that building Boost with CMake will fail with an earlier CMake. We haven't yet set a firm minimum CMake version which to support. (It's possible that we'll end up requiring 3.12 or higher, of course.)

As long as the tensor can still be compiled, I am fine with this PR.

I have to wonder, what C++20 features are you using?

We were already using C++17:

  • (not important) [[nodiscard]]
  • (not important) static_assert without additional message
  • (important) if constexpr because we are evaluating the parameter packs
  • (not important) fold expressions
  • etc...

We were already using C++20:

  • (already using) mainly concepts and related language features (SFINAE....)
  • (thinking about) consteval

PR #94 is incompatible with the Boost CMake infrastructure and targets the use case where uBLAS is the root project. In such cases what we do is combine the two using an if statement - when not root project, use the Boost-compatible CML, if root project, use whatever the project maintainers like. We did this for several libraries, such as f.ex. GIL: boostorg/gil@85838cd.

I guess that this would be a good solution for us too.

If you don't insist on having your own CML file, it's more convenient to use the boostdep output as-is, without any changes, because it makes it easier to just regenerate it if changes need to be done (which happens f.ex. when your dependency list changes.)

That's fine with me as long as we can do something similar as done for GIL. Maybe you could help us here too.

bassoy avatar Oct 29 '21 14:10 bassoy

Unrelated to this PR, but are you aware that Boost releases are done from the master branch, and you haven't touched it since 2019? That is, none of the changes you've made to develop have been part of a Boost release.

How are people supposed to use the new features? Checkout the develop branch directly?

pdimov avatar Oct 29 '21 14:10 pdimov

Unrelated to this PR, but are you aware that Boost releases are done from the master branch, and you haven't touched it since 2019? That is, none of the changes you've made to develop have been part of a Boost release.

How are people supposed to use the new features? Checkout the develop branch directly?

Yes this a different topic and I would like to discuss this in the GitHub discussion section.

Short answer: Code that uses C++17 features has been merged in 2018. Code using C++20 features is still not ready to be merged.

bassoy avatar Oct 29 '21 16:10 bassoy

It's relevant here because I want to figure out what CMakeLists.txt file we want on the master branch.

On develop, if I understand correctly, you want a combination of this and #94, possibly with cxx_std_20 in target_compile_features.

pdimov avatar Oct 29 '21 16:10 pdimov

It's relevant here because I want to figure out what CMakeLists.txt file we want on the master branch.

On develop, if I understand correctly, you want a combination of this and #94, possibly with cxx_std_20 in target_compile_features.

Got it. The master branch needs the cxx_std_17 if we want to include tests and examples. The develop branch requires the cxx_20_std.

bassoy avatar Oct 29 '21 17:10 bassoy

Will this be merged anytime soon?

lordjaxom avatar Apr 11 '23 15:04 lordjaxom

Please merge this PR. It is required for ublas to be usable when boost is added as a submodule to a project.

greg7mdp avatar May 07 '23 14:05 greg7mdp

At the moment ublas is the only Boost library lacking a CMakeLists.txt, which creates problem for Boost users who use CMake (https://github.com/boostorg/cmake/issues/41 is the latest instance of this being brought up.)

What needs to be done to get this merged?

pdimov avatar Aug 01 '23 17:08 pdimov

Would really appreciate if reviewer(s) can take a look at this PR! Checking out this patch locally unblocks me from using CMake with Boost. It would saved much trouble if merged. Ty!

q-ycong-p avatar Aug 01 '23 20:08 q-ycong-p

@bassoy any update on this PR? I am also running into this issue when trying to build Boost from source via CMake.

jdumas avatar Sep 25 '23 17:09 jdumas

I've just received another complaint that Boost::accumulators isn't functional because it depends on Boost::numeric_ublas, and uBLAS does not have a CMakeLists.txt. (https://github.com/boostorg/cmake/issues/45#issuecomment-1815966205)

Since I see no activity here, if I receive no complaints, I will go ahead and add the CMakeLists.txt file to both master and develop, so that CMake users of Accumulators (and uBLAS itself, for that matter) can be unblocked.

pdimov avatar Nov 17 '23 11:11 pdimov

haha, about time! Thanks Peter.

greg7mdp avatar Nov 17 '23 12:11 greg7mdp

I think the CMakeLists looks good and I'd be happy to see it merged as well.

jdumas avatar Nov 17 '23 15:11 jdumas

+1

lordjaxom avatar Nov 17 '23 15:11 lordjaxom

@bassoy should I merge this, or does anyone else want to do? If anyone has any objection against me merging, say no.

amitsingh19975 avatar Nov 17 '23 17:11 amitsingh19975