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

Correct issues with CMake install rules #331

Merged
merged 4 commits into from
Nov 15, 2022

Conversation

robertmaynard
Copy link
Collaborator

@robertmaynard robertmaynard commented Nov 8, 2022

This corrects three issues found by rapids-cmake and others when trying to install libcudacxx.

  1. The install commands need to come after project. The install commands aren't allowed when in CMake scripting mode which is the mode you are in before the first call to project()
  2. The install(DIRECTORY PATTERN) matches the end of a file name, not the front so we need to use *.cmake.in to not
    install the template files
  3. We incorrectly used libcudacxx_TOPLEVEL_PROJECT instead of LIBCUDACXX_TOPLEVEL_PROJECT so we never defaulted to have install rules enabled when building libcudacxx itself.
  4. Don't install any CMakeLists.txt files that are inside include/cuda' or include/nv'

The install commands in cmake are only supported in project mode
and therefore need to come after the `project` call.
@robertmaynard robertmaynard added only: cmake CMake changes only. bug: functional Does not work as intended. labels Nov 8, 2022
Copy link
Collaborator

@miscco miscco left a comment

Choose a reason for hiding this comment

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

LGTM but I feel like a 3-year old whenever I see cmake, so I would love some feedback from more knowledgeable folks

These CMakeLists.txt files aren't needed by consumers, and therefore
shouldn't be installed.
@wmaxey
Copy link
Member

wmaxey commented Nov 8, 2022

Pipeline has been launched on Blossom. We'll know the results soon.

@robertmaynard
Copy link
Collaborator Author

Pipeline has been launched on Blossom. We'll know the results soon.

Any update on this?

Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

lgtm

@wmaxey
Copy link
Member

wmaxey commented Nov 15, 2022

Pipeline has been launched on Blossom. We'll know the results soon.

Any update on this?

Pipeline was having issues over the weekend and earlier. Tests are passing. LGTM.

@wmaxey wmaxey merged commit 9610390 into NVIDIA:main Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug: functional Does not work as intended. only: cmake CMake changes only.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants