Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

cmake: option to install thrust when used via add_subdirectory() #1297

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

germasch
Copy link
Contributor

This is about the case where a downstream library pulls in Thrust via add_subdirectory(). In this case, when the library is installed, it may want to also install Thrust together with it, as it's presumably needed to use the library (in particular, if the library's public header include thrust headers.)

This PR adds an option THRUST_INSTALL to Thrust. The default install behavior is unchanged, ie., when Thrust is built stand-alone, it will be installed on make install, and when Thrust is pulled in via add_subdirectory(), it won't install itself. But in the case described above, the downstream project can set(THRUST_INSTALL ON) before the add_subdirectory() so that the downstream library's install will contain Thrust.

@alliepiper alliepiper self-assigned this Sep 28, 2020
@alliepiper alliepiper added the only: cmake CMake changes only. Doesn't need internal NVIDIA CI. label Sep 28, 2020
@alliepiper alliepiper added this to the 1.11.0 milestone Sep 28, 2020
@alliepiper
Copy link
Collaborator

LGTM -- thanks for the PR, I'll merge this soon.

Heads up that I'll need to squash this down to a single commit before merging to simplify our internal integration process.

@germasch
Copy link
Contributor Author

Heads up that I'll need to squash this down to a single commit before merging to simplify our internal integration process.

Not a problem.

cmake/install: mv ThrustAddSubdir handling to after project()

We want to add the option to install in this case, which requires
the paths set by project()

thrust: separate out determination of whether Thrust is top-level project

This determines whether an install target will be provided. If Thrust is
built as top-level project, the install target will be provided by default,
as before.

If Thrust is built as a sub-project, the install target will, by default,
not be provided, again maintaining existing behavior.

So what's new here is that via this option a downstream project can enable
install of thrust if it is used via `add_subdirectory`.

Update CONTRIBUTING.md with new CMake option docs.
@alliepiper
Copy link
Collaborator

Tested locally, looks good. I changed the name of the variable to THRUST_ENABLE_INSTALL_RULES to better fit with the other THRUST_ENABLE options, and updated CONTRIBUTING.md to document the new var.

I'm going to hold off on merging this so I can make the same changes to CUB at the same time.

@alliepiper alliepiper added the blocked Cannot make progress. label Oct 5, 2020
@germasch
Copy link
Contributor Author

germasch commented Oct 6, 2020

Sounds fine to me. There doesn't seem to be a standard naming convention for the install functionality, so there's no harm in adding yet another one ;)

(some selection: SPDLOG_INSTALL , BENCHMARK_ENABLE_INSTALL, INSTALL_GTEST)

@alliepiper alliepiper merged commit e288eaf into NVIDIA:main Oct 15, 2020
alliepiper added a commit to alliepiper/cub that referenced this pull request Oct 15, 2020
Specifically, this ports the following PRs to CUB:

- NVIDIA/thrust#1297 Add install rule option for add_subdirectory users.
- NVIDIA/thrust#1298 Enforce semantic versioning in CMake version configs.
- NVIDIA/thrust#1300 Use FindPackageHandleStandardArgs in CMake config.
alliepiper added a commit to NVIDIA/cub that referenced this pull request Oct 19, 2020
Specifically, this ports the following PRs to CUB:

- NVIDIA/thrust#1297 Add install rule option for add_subdirectory users.
- NVIDIA/thrust#1298 Enforce semantic versioning in CMake version configs.
- NVIDIA/thrust#1300 Use FindPackageHandleStandardArgs in CMake config.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked Cannot make progress. only: cmake CMake changes only. Doesn't need internal NVIDIA CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants