Skip to content

Migrated build process from Makefiles to CMake #5

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

Closed
wants to merge 11 commits into from
Closed

Migrated build process from Makefiles to CMake #5

wants to merge 11 commits into from

Conversation

ambasta
Copy link

@ambasta ambasta commented Jan 2, 2018

Hi,

I've migrated current build process from raw Makefiles to CMake instead. This allows for easier configurable releases and the standard configure/make/make install process across platforms for easier packaging across linux distributions and otherwise.

Additionally, it also exports projectConfig.cmake files for other projects to detect pstl and specify it as a dependency!

@tbbdev
Copy link
Contributor

tbbdev commented Jan 15, 2018

Hello, ambasta.
Thank you for your contribution.
We've started reviewing it.

@ambasta
Copy link
Author

ambasta commented Feb 26, 2018

Hi @tbbdev

Any updates on the PR?

@tbbdev
Copy link
Contributor

tbbdev commented Feb 28, 2018

Hi @ambasta,

We are interested in developing CMake infrastructure for Parallel STL and appreciate your efforts, but unfortunately cannot accept the PR as is.

There are several reasons for this, for example:

  • We support many platforms (see Release Notes for details), but the proposed CMakeLists.txt is mainly oriented on Linux.
  • TBB dependency should be implemented via native CMake find_package as it is supported by TBB.
  • CMake support for tests and examples needs to be considered separately.
  • Many (or even all) used macros are internal, applicable only for tests and should not be included to CMAKE_CXX_FLAGS, e.g. __PSTL_TEST_SUCCESSFUL_KEYWORD.
  • The Parallel STL version should be retrieved from include/pstl/internal/pstl_config.h instead of Git tag.
  • Our internal infrastructure uses Makefiles, so we need to keep them.

Our primary CMake goal is providing the simple integration of Parallel STL into client CMake projects.
CMake support for examples, tests, installation and packaging needs to be considered separately.

@ambasta
Copy link
Author

ambasta commented Feb 28, 2018

Noted, I'll update the PR will recommended changes!

@henryiii
Copy link

I've addressed most (all?) of the review points and have made a PR to the fork ambasta/parallelstl#1 ; if @ambasta accepts it, that will keep everything contained in this PR. (Otherwise, I can make a new PR).

@tbbdev
Copy link
Contributor

tbbdev commented Apr 17, 2018

@henryiii, @ambasta, thank you for your efforts!

There are detailed comments about some things that have already been mentioned above:

We're working on integration of Parallel STL into standard C++ libraries libc++ and libstdc++.
So we don’t see the value of the installation and packaging parts of the proposed CMake project in future: you will get Parallel STL either with other Intel(R) products or with standard libraries, and having the custom installation may lead to conflicts.

About examples: we would prefer to have examples as separate projects with an external dependency on Parallel STL to demonstrate the integration. Let's work on it out of this PR.

About tests: we would prefer to have the test part which doesn’t require regular support, i.e. without hardcoded test names. Let's work on it out of this PR too.

Summary: we are ready to integrate the core part and continue to work on test and examples, but installation and packaging parts are not in long term strategy for Parallel STL.

If you agree we will merge the PR internally and it will be available in some new version.

Thank you.

@henryiii
Copy link

That sounds like a reasonable path forward. +1

@ambasta
Copy link
Author

ambasta commented Apr 18, 2018

I've been following the proposals to add pstl to libstdc++/libcxx. Does this mean the merged PSTL won't depend on tbb as per the proposed spec?

@akukanov
Copy link
Contributor

@ambasta, we expect Parallel STL to be extended with more parallel backends over time, and programmers will be able to choose which one to use.

@rodgert
Copy link
Contributor

rodgert commented Apr 27, 2018

@ambasta I expect that I will be adding a couple of backend options over the next few months as I move toward a final version to ship as part of libstdc++, but we will still support TBB as a backend option in GCC9.

@akukanov I've revisited this pull request for reference. A few notable points -

  • I understand that your internal build system uses makefiles, however...

  • libc++ uses cmake and libstdc++ uses Autotools for builds

  • libc++ uses LLVM's LIT test driver

  • libstdc++ uses DejaGnu as it's test driver

  • I have yet to push an update which includes significant test reorganization to make the PSTL tests conform to libc++'s testing standard, but I've discussed with both @mclow and @jwakely and we've agreed to follow libc++'s lead here.

  • We need these tests suites to both be buildable, and runnable via both CMake and Autotools. My pull req for the test reorganization will include support for both (drawing from the work done in this pull req for the CMake parts).

  • If adopted, this test reorganization update coupled with the fact that libc++ uses cmake and libstdc++ uses Autotools/DejaGnu, in my opinion makes it difficult to continue to provide corresponding support in the existing makefiles. In particular; we will eventually add tests where we intend to fail compilation (libc++'s .fail.cpp tests), this will require some other test driver logic besides a successful completion via make.

  • I am trying to preserve as much of the existing makefile's support for other target platforms as possible, but I don't have the ability to fully test that.

@akukanov
Copy link
Contributor

@rodgert, we are fully open to reorganize and expand the build and test systems in the way that it works for libc++ and libstdc++. We will have to figure out how much of it will still work via makefiles. The problem of negative tests might possibly be addressed with some test launcher script.

@rodgert
Copy link
Contributor

rodgert commented Apr 28, 2018

@akukanov Agreed

I'm not going to turn this into a formal pull req until we finalize the other reorganization work, but I will be committing my changes on https://github.com/rodgert/parallelstl/tree/stdlib-test-reorg and I'd appreciate any feedback you might have on how best to address the existing makefiles with this set of changes

tbbdev added a commit that referenced this pull request May 8, 2018
Updates in comparison with PR #5:
- Applied changes according to comment: #5 (comment)
- Re-worked logic of PARALLELSTL_USE_PARALLEL_POLICIES and PARALLELSTL_BACKEND interaction.
- Used more precise dependency on TBB.
- Added Apache 2.0 copyright notices.
@@ -0,0 +1,94 @@
cmake_minimum_required(VERSION 3.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "VERSION 3.1" is a minimal requirement?
Are any features not available in earlier versions?

Copy link

Choose a reason for hiding this comment

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

CMake selects behaviors based on the minimum version; the wrong behavior will be selected if this is too low. CMake is on version 3.11 now, so this is a very old (5 year old) version. You should never have an older version of CMake than your compiler. And it's trivial to install. See https://cliutils.gitlab.io/modern-cmake/.

And, yes, C++11 support (CMAKE_CXX_STANDARD) was introduced in CMake 3.1. Interface targets had lots of fixes, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

@henryiii , thank you for explanation

tbbdev pushed a commit that referenced this pull request Jun 20, 2018
Updates in comparison with PR #5:
- Applied changes according to comment: #5 (comment)
- Re-worked logic of PARALLELSTL_USE_PARALLEL_POLICIES and PARALLELSTL_BACKEND interaction.
- Used more precise dependency on TBB.
- Added Apache 2.0 copyright notices.
@tbbdev
Copy link
Contributor

tbbdev commented Jun 20, 2018

PR #10 based on this PR has been merged into master and included into Parallel STL 20180619 release (see CHANGES).

@ambasta, do you agree to close this PR?

@ambasta ambasta closed this Jun 20, 2018
SergeyKopienko added a commit that referenced this pull request Jun 13, 2023
SergeyKopienko added a commit that referenced this pull request Jun 19, 2024
…__any_of_group for __parallel_find_any #5

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
SergeyKopienko added a commit that referenced this pull request Jun 19, 2024
…__any_of_group for __parallel_find_any #5

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
SergeyKopienko added a commit that referenced this pull request Jun 19, 2024
…__any_of_group for __parallel_find_any #5

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
SergeyKopienko added a commit that referenced this pull request Jun 19, 2024
…__any_of_group for __parallel_find_any #5.1

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
SergeyKopienko added a commit that referenced this pull request Jun 19, 2024
…__any_of_group for __parallel_find_any #5

Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
SergeyKopienko added a commit that referenced this pull request Jul 15, 2024
Signed-off-by: Sergey Kopienko <sergey.kopienko@intel.com>
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