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.

Sign up for GitHub

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

Jump to bottom

Add CMake for documentation #442

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add CMake for documentation #442

wants to merge 1 commit into from
+122 −46

Conversation

matyas-streamhpc
Copy link

@matyas-streamhpc matyas-streamhpc commented Nov 21, 2023 •
edited
Loading

Summary of proposed changes:
Add CMake support for documentation.

@saadrahim
Copy link
Member

@feizheng10 @malcolmroberts Can I get a review for this PR?

feizheng10
feizheng10 reviewed Nov 28, 2023
View reviewed changes
CMakeLists.txt Outdated
@@ -137,6 +139,9 @@ if(BUILD_FILE_REORG_BACKWARD_COMPATIBILITY AND NOT WIN32)
)
endif()

# Enable documentation build
option(BUILD_DOCS "Build documentation" ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall the standard... but all rocFFT specific build option should start with ROCFFT_, do we? Unless it is a rocm cmake level stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think we want to make similar changes to all math libs, not just rocFFT. So IMO the generic name makes more sense.

Copy link
Member

Choose a reason for hiding this comment

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

This will be a ROCm wide build option.

Copy link
Member

Choose a reason for hiding this comment

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

BUILD_DOCS should be OFF by default

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

feizheng10
feizheng10 approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@feizheng10 feizheng10 left a comment

Choose a reason for hiding this comment

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

LG

@evetsso
Copy link
Contributor

evetsso commented Nov 28, 2023

The general idea here is definitely good - when I think ahead to wanting to making a similar change for all math libs, I'm wondering whether at least some of this should be moved into rocm-cmake so it doesn't have to be redone for each library.

The root CMakeLists.txt also is prepared to fetch rocm-cmake, so doing it again in the docs dir seems redundant. @lawruble13, can you think of a better way to do this?

cgmb
cgmb reviewed Nov 29, 2023
View reviewed changes
cmake/Dependencies.cmake Outdated
rocm-cmake
GIT_REPOSITORY https://github.com/RadeonOpenCompute/rocm-cmake.git
GIT_TAG ${rocm_cmake_tag}
SOURCE_SUBDIR "DISABLE ADDING TO BUILD" # We don't really want to consume the build and test targets of ROCm CMake.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we should consider doing something to better support this usage in rocm-cmake. Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Let's standardize all rocm projects to have this cmake/Dependencies file that will download rocm-cmake.

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

saadrahim
saadrahim requested changes Nov 29, 2023
View reviewed changes
CMakeLists.txt Outdated
@@ -137,6 +139,9 @@ if(BUILD_FILE_REORG_BACKWARD_COMPATIBILITY AND NOT WIN32)
)
endif()

# Enable documentation build
option(BUILD_DOCS "Build documentation" ON)
Copy link
Member

Choose a reason for hiding this comment

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

BUILD_DOCS should be OFF by default

CMakeLists.txt Outdated
@@ -47,6 +47,8 @@ endif()

set( ROCFFT_BUILD_SCOPE ON )

include(GNUInstallDirs)

project( rocfft LANGUAGES CXX C )

# This finds the rocm-cmake project, and installs it if not found
Copy link
Member

Choose a reason for hiding this comment

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

We need to have only one place that rocm-cmake is downloaded

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

@saadrahim
Copy link
Member

@evetsso and @cgmb can you please take a look at this again?

saadrahim
saadrahim reviewed Dec 18, 2023
View reviewed changes
cmake/Dependencies.cmake Outdated
if(NOT ROCM_FOUND)
message(STATUS "ROCm CMake not found. Fetching...")
set(rocm_cmake_tag
"c044bb52ba85058d28afe2313be98d9fed02e293" # [email protected]. (move to 6.0 tag when released)
Copy link
Member

Choose a reason for hiding this comment

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

update to tag now

Copy link
Author

Choose a reason for hiding this comment

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

Updated

@saadrahim
Copy link
Member

@malcolmroberts shall I merge the PR into this repository?

@matyas-streamhpc
Copy link
Author

Rebased.

@matyas-streamhpc
Copy link
Author

Rebased.

amd-jnovotny
amd-jnovotny approved these changes Oct 13, 2024
View reviewed changes
Copy link
Contributor

@amd-jnovotny amd-jnovotny left a comment

Choose a reason for hiding this comment

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

Docs component looks good

CHANGELOG.md
@@ -11,6 +11,10 @@ Documentation for rocFFT is available at

## rocFFT 1.0.26 for ROCm 6.1.0

### Additions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Additions
### Added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saadrahim saadrahim saadrahim approved these changes

@evetsso evetsso evetsso left review comments

@cgmb cgmb cgmb left review comments

@amd-jnovotny amd-jnovotny amd-jnovotny approved these changes

@feizheng10 feizheng10 feizheng10 approved these changes

@malcolmroberts malcolmroberts malcolmroberts approved these changes

@lawruble13 lawruble13 Awaiting requested review from lawruble13

@af-ayala af-ayala Awaiting requested review from af-ayala af-ayala is a code owner

@eng-flavio-teixeira eng-flavio-teixeira Awaiting requested review from eng-flavio-teixeira eng-flavio-teixeira is a code owner

Assignees
No one assigned
Labels
ci:smoke
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@matyas-streamhpc @saadrahim @evetsso @malcolmroberts @cgmb @feizheng10 @amd-jnovotny @samjwu