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 FindTBB.cmake module #13

Closed
wants to merge 3 commits into from
Closed

Conversation

rodgert
Copy link
Contributor

@rodgert rodgert commented Jun 26, 2018

Add module to locate TBB, update CMakeLists.txt to utilize project local CMake modules directory

Structure should conform to libc++ standards
Includes a simple CMake based test driver, based on previous (limited/broken) CMake support
@rodgert
Copy link
Contributor Author

rodgert commented Jun 27, 2018

I have now added a CMake/CTest structure and support for some (not all) of the existing tests under the new libc++ compatible testsuite/ dir

@MikeDvorskiy
Copy link
Contributor

Thomas, could you please split the pull request into two: "Add FindTBB.cmake module" and "Initial migration of test suite "?
And the second question - what's motivation for renaming the test files (using "pass" sub-string in filename)?

@rodgert
Copy link
Contributor Author

rodgert commented Jun 28, 2018

@MikeDvorskiy
1st - sure, I dislike that Github automatically lumps these things together
2nd - We had discussed this in the call we had a few weeks back w/Intel. The reason for the renaming is this -

  • The Standard Library has not only tests which must compile and run to completion, but also tests which are expected to fail, in particular to fail compilation, you will find in the Standard language like -
    "Must not participate in overload resolution unless..."
  • In discussions with @mclow, @jwakely and I have agreed the test suite should follow libc++ conventions
  • libc++ uses a test driver from LLVM called LIT that uses the .pass and .fail tags in the source file name

@mclow
Copy link

mclow commented Jun 28, 2018

Right - we need a way to specify tests that are expected not to compile.
In the libc++ test suite, we name them XXXX.fail.cpp.
The tests that should compile (and run, exiting with status 0) are named XXX.pass.cpp

@rodgert
Copy link
Contributor Author

rodgert commented Jun 28, 2018

@MikeDvorskiy I have, for now reverted the test suite changes, if you can accept this one, then I can push the remaining test reorg tomorrow as a single commit.

@jwakely
Copy link

jwakely commented Jun 28, 2018

In libstdc++ our convention for naming failing tests is is xxx_neg.cc but xxx.fail.cpp is OK too (our expectation of pass/fail is not based on the filename anyway, but on metadata in the file itself).

@AlexVeprev
Copy link
Contributor

AlexVeprev commented Jun 29, 2018

Hi @rodgert

The modern TBB supports CMake binary integration.
All you need is to download a released binary package for your system, unpack it and set (in CMake configuration) TBB_DIR as <tbbroot>/cmake or append <tbbroot> to CMAKE_PREFIX_PATH.

Example on Linux:

cd /tmp
git clone https://github.com/intel/parallelstl
wget https://github.com/01org/tbb/releases/download/2018_U5/tbb2018_20180618oss_lin.tgz
tar zxf tbb2018_20180618oss_lin.tgz
mkdir build && cd build
cmake -DTBB_DIR=/tmp/tbb2018_20180618oss/cmake /tmp/parallelstl 
# Output:
# ...
# -- Build files have been written to: /tmp/build

In that case you don't need FindTBB.cmake at all.
Is that approach suitable for you?

Additionally TBB provides CMake modules for downloading and building itself.

@rodgert
Copy link
Contributor Author

rodgert commented Jul 1, 2018

@AlexVeprev Ultimately my target is GCC/libstdc++ so the CMake build is more of a courtesy/convenience, so I don't have a lot of hard opinions on the "best way" to do this, but -

I already have TBB installed on my machine (Fedora, via DNF) and I want to stick with the versions which ship with a given Fedora or RHEL release from a trusted upstream, so I'd like to opt out of just downloading a given TBB version or making it incumbent on the user to do so in a similar situation. Can we have both?

@rodgert
Copy link
Contributor Author

rodgert commented Jul 1, 2018

@jwakely I could split the difference and name some of them .fail and some .neg

@MikeDvorskiy
Copy link
Contributor

Hi @rodgert,
Regarding
"...I can push the remaining test reorg tomorrow as a single commit."
Yes, sure.

@rodgert
Copy link
Contributor Author

rodgert commented Jul 2, 2018

@MikeDvorskiy Do you want it folded into this Pull Req, or should we accept this one and and open an new one?

@MikeDvorskiy
Copy link
Contributor

@rodgert,
I meant "Could you please a Pull Request that contains just the tests modification(file renames and others) which doesn't depend on "CMake" changes?"

@AlexVeprev
Copy link
Contributor

@rodgert, yes, this looks like a common case when user installs TBB using some package manager and wants to use this TBB as dependency for Parallel STL.

The most natural way is having a CMake configuration in this installed TBB package, but it is not supported for now.
Another way is having a script (e.g. the proposed FindTBB.cmake) on Parallel STL side which finds the installed TBB.
But how often will this script be useful? Parallel STL requires TBB 2018 or newer. Many package managers provide an older TBB by default (e.g. DNF on Fedora 27 installs TBB 2017 U7), so in that case even if you install TBB, Parallel STL won't be able to use it and you will have to download a newer TBB from GitHub, for example.

@rodgert
Copy link
Contributor Author

rodgert commented Jul 5, 2018

@MikeDvorskiy I'm not sure of the utility of that request, as I've stated (and was discussed in our meeting with @capatober) I'm not going to update your makefiles or your existing test suite. This is bootstrapping a new test suite to be used by both libstdc++ and libc++ from your existing test suite. Currently the CMake/CTest scripts are the only existing driver for that new test suite. I also don't intend to build a parallel Autotools build/DejaGnu test suite within the upstream, as I only need that functionality with GCC integration. I think libc++ would similarly integrate this into it's existing test hierarchy.

But, without a test driver in the upstream package, there's no way to run this testsuite against the standalone library.

Put another way, how would I/you verify just the test changes you request, without also means to build and run them?

@rodgert
Copy link
Contributor Author

rodgert commented Jul 11, 2018

@MikeDvorskiy @captober FWIW, I am proceeding with libstdc++ integration for an internal deadline based on the branch from which this pull request was issued from.

As noted, I don't need the CMake support, but I don't (yet) know of a way to allow somebody to verify the changes in the restructured testsuite/ dir for the standalone implementation without some form of test driver, and I don't really have any motivation to support anything other than a simple CMake based test suite here.

@tbbdev
Copy link
Contributor

tbbdev commented Dec 5, 2018

In the release 20181204 we added another implementation of FindTBB CMake module as a Preview Feature. Thank you all for the involving!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants