Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CMakeLists.txt #142

Closed
wants to merge 1 commit into from
Closed

Add CMakeLists.txt #142

wants to merge 1 commit into from

Conversation

pdimov
Copy link
Member

@pdimov pdimov commented Oct 28, 2021

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

@github-actions
Copy link

This Pull request Passed all of clang-tidy tests. 👍

Copy link
Collaborator

@bassoy bassoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pdimov

I am fine with your addition. As already mentioned, I am not sure about the target_compile_features. See my comments.

Boost::typeof
)

target_compile_features(boost_numeric_ublas INTERFACE cxx_std_11)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be cxx_std_20 because of the tensor extension.
if only matrix and vector types are used cxx_std_11 is sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a deep knowledge of CMAKE. Therefore, correct me if I'm wrong. Why don't we put another cmake file inside the Tensor with the compiler feature set to c++20? Then we can include the directory.

This approach might fix the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit that I don't have that knowledge either. But as CMAKE specification should be made modular your hint might solve this issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we combine PR #94 with this one?
@coder3101 did a very good job.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that should be an issue, but it's better if @pdimov answers it.

@pdimov
Copy link
Member Author

pdimov commented Oct 29, 2021

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: boostorg/gil@85838cd.

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.)

@bassoy
Copy link
Collaborator

bassoy commented Oct 29, 2021

@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.

@pdimov
Copy link
Member Author

pdimov commented Oct 29, 2021

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?

@bassoy
Copy link
Collaborator

bassoy commented Oct 29, 2021

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.

@pdimov
Copy link
Member Author

pdimov commented Oct 29, 2021

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.

@bassoy
Copy link
Collaborator

bassoy commented Oct 29, 2021

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.

@lordjaxom
Copy link

Will this be merged anytime soon?

@greg7mdp
Copy link

greg7mdp commented May 7, 2023

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

@pdimov
Copy link
Member Author

pdimov commented Aug 1, 2023

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

What needs to be done to get this merged?

@q-ycong-p
Copy link

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!

@jdumas
Copy link

jdumas commented Sep 25, 2023

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

@pdimov
Copy link
Member Author

pdimov commented Nov 17, 2023

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. (boostorg/cmake#45 (comment))

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.

@greg7mdp
Copy link

haha, about time! Thanks Peter.

@jdumas
Copy link

jdumas commented Nov 17, 2023

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

@lordjaxom
Copy link

+1

@amitsingh19975
Copy link
Collaborator

amitsingh19975 commented Nov 17, 2023

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

Copy link
Collaborator

@amitsingh19975 amitsingh19975 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks.

@pdimov pdimov closed this in 8934b49 Nov 22, 2023
@pdimov pdimov deleted the pr/cmakelists branch November 22, 2023 00:19
pdimov added a commit that referenced this pull request Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants