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

CMake: add option TBB_INSTALL #800

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Mar 7, 2022

Signed-off-by: Vladislav Shchapov [email protected]

Description

Make installation optional.

In the case of including oneTBB using add_subdirectory, it may require a custom installation rules.

Fixes # - issue number(s) if exists

  • - git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in this PR
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

List users with @ to send notifications

Other information

@AlexVeprev
Copy link
Contributor

Hi @phprus, I'd like to double check that I got the use case properly: you want to have oneTBB added into your project with add_subdirectory but don't want oneTBB to be installed when you run make install for your project. Right? And all you need - is to wrap all the installation instructions for oneTBB under condition with a new option TBB_INSTALL without other modifications of installation instructions. Right?

@phprus
Copy link
Contributor Author

phprus commented Mar 11, 2022

  1. Yes;
  2. Yes.

I use this patch (and set TBB_INSTALL to OFF) and custom install() commands (and some additional options for tbb* targets) for tbb, tbbmalloc, tbbmalloc_proxy targets in parent cmake project.

@phprus
Copy link
Contributor Author

phprus commented Jul 1, 2022

Ping?

@phprus phprus force-pushed the cmake-disable-install branch from f55b315 to a9742c0 Compare October 1, 2022 12:15
@phprus
Copy link
Contributor Author

phprus commented Oct 1, 2022

Rebased onto current master.

@phprus phprus force-pushed the cmake-disable-install branch from a9742c0 to 8043a0e Compare November 5, 2022 18:07
@phprus
Copy link
Contributor Author

phprus commented Nov 5, 2022

Rebased

@phprus phprus force-pushed the cmake-disable-install branch from 8043a0e to f93dc4e Compare December 18, 2022 18:52
@phprus
Copy link
Contributor Author

phprus commented Dec 19, 2022

@pavelkumbrasev, @isaevil what do you think about this PR?

@phprus phprus force-pushed the cmake-disable-install branch from f93dc4e to fc9b259 Compare February 5, 2023 10:01
@phprus
Copy link
Contributor Author

phprus commented Feb 5, 2023

@pavelkumbrasev, @isaevil, @kboyarinov
ping?

@phprus phprus force-pushed the cmake-disable-install branch from fc9b259 to e3664da Compare February 13, 2023 16:40
@phprus
Copy link
Contributor Author

phprus commented Feb 13, 2023

Rebased

@phprus phprus force-pushed the cmake-disable-install branch from e3664da to ac43785 Compare February 18, 2023 17:47
@phprus phprus force-pushed the cmake-disable-install branch from ac43785 to 891f63f Compare May 22, 2023 19:59
@phprus
Copy link
Contributor Author

phprus commented May 22, 2023

Rebased and ping.

@phprus phprus force-pushed the cmake-disable-install branch from 891f63f to 61518db Compare May 26, 2023 11:26
@phprus phprus force-pushed the cmake-disable-install branch from 61518db to 9bf5871 Compare June 8, 2023 12:33
Signed-off-by: Vladislav Shchapov <[email protected]>
@phprus phprus force-pushed the cmake-disable-install branch from 9bf5871 to 3f1f948 Compare October 28, 2023 14:45
@phprus
Copy link
Contributor Author

phprus commented Oct 28, 2023

Rebased to resolve conflicts with current master.

Request review @isaevil, @pavelkumbrasev, @dnmokhov, @ilya-lavrenov

@ilya-lavrenov
Copy link
Contributor

@phprus can you just use add_subdirectory with EXCLUDE_FROM_ALL option? It will ignore TBB install rules.

@phprus
Copy link
Contributor Author

phprus commented Oct 29, 2023

@ilya-lavrenov, Thank you for your reply!

EXCLUDE_FROM_ALL by default disables test compilation and does not allow to set custom installation rules for targets.

I need build/run all tests and install targets tbb, tbbmalloc, tbbmalloc_proxy with custom DESTINATIONs and COMPONENTs options.

Since the build system has the TBB_INSTALL_VARS option, I suggest adding a option to completely disable the installation.

@ilya-lavrenov
Copy link
Contributor

EXCLUDE_FROM_ALL by default disables test compilation

Why do you need to build tests when TBB (or other external project) is used as submodule and included into main project via add_subdirectory? It's a good practice to include external projects with EXCLUDE_FROM_ALL option

does not allow to set custom installation rules for targets

It allows. But there are some corner cases (like we had with pugixml zeux/pugixml#585 in OpenVINO).
So, in general I agree that it's better to disable install rules.

@phprus
Copy link
Contributor Author

phprus commented Oct 29, 2023

@ilya-lavrenov

Why do you need to build tests when TBB (or other external project) is used as submodule and included into main project via add_subdirectory?

We try to run tests for all dependencies. Sometimes this helps to find bugs in dependencies.
Sometimes we use our own patches for dependencies, which requires running tests. For example, for oneTBB: this PR, #1114 and #714.

Copy link
Contributor

@isaevil isaevil left a comment

Choose a reason for hiding this comment

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

LGTM

@isaevil isaevil merged commit f27eb14 into uxlfoundation:master Nov 6, 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.

4 participants