Skip to content

Handle build warnings Removing All#274

Open
essamROCm wants to merge 4 commits into
ROCm:developfrom
essamROCm:ea/Handle_All_Build_Warnnings
Open

Handle build warnings Removing All#274
essamROCm wants to merge 4 commits into
ROCm:developfrom
essamROCm:ea/Handle_All_Build_Warnnings

Conversation

@essamROCm
Copy link
Copy Markdown
Contributor

@essamROCm essamROCm commented Mar 31, 2026

Motivation

Enable build warnings and modify code to remove the warning causes.

Technical Details

Effect many source code files.

Test Plan

Same ctest
Tested on ROCm and TheRock environment.

Test Result

ALL PASS

@essamROCm essamROCm self-assigned this Mar 31, 2026
@essamROCm essamROCm added the enhancement New feature or request label Mar 31, 2026
@essamROCm essamROCm marked this pull request as ready for review April 6, 2026 23:58
@essamROCm
Copy link
Copy Markdown
Contributor Author

essamROCm commented Apr 6, 2026

@AryanSalmanpour @rrawther @LakshmiKumar23 @kiritigowda
All CI checks have passed successfully.

Comment thread CMakeLists.txt
find_package(HIP REQUIRED)
find_package(rocdecode 1.0.0 QUIET)
find_package(rocjpeg 1.0.0 QUIET)
# ROCm package versions are currently 0.x even though the APIs used here are stable.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this comment is incorrect, the version of rocDecode on TheRock now brings in 1.8.0 and rocjpeg is 1.4.0. Please keep the versioning check

(rock_0408) lakshmi@kapu:~/work/lk/rpp/build$ ls /home/lakshmi/rock_0408/lib/python3.10/site-packages/_rocm_sdk_devel/lib/librocdecode.so
librocdecode.so        librocdecode.so.1      librocdecode.so.1.8.0

(rock_0408) lakshmi@kapu:~/work/lk/rpp/build$ ls /home/lakshmi/rock_0408/lib/python3.10/site-packages/_rocm_sdk_devel/lib/librocjpeg.so
librocjpeg.so        librocjpeg.so.1      librocjpeg.so.1.4.0

Comment thread CMakeLists.txt

# Always build the GPU path
include_directories(${rocdecode_INCLUDE_DIR}
include_directories(SYSTEM
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does this SYSTEM do here?

Comment thread CMakeLists.txt
list(APPEND include ${include_ffmpeg})
list(APPEND sources ${sources_ffmpeg})
include_directories(${AVUTIL_INCLUDE_DIR} ${AVCODEC_INCLUDE_DIR} ${AVFORMAT_INCLUDE_DIR})
include_directories(SYSTEM ${AVUTIL_INCLUDE_DIR} ${AVCODEC_INCLUDE_DIR} ${AVFORMAT_INCLUDE_DIR})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above?

Comment thread CMakeLists.txt
# rocJPEG
if(rocjpeg_FOUND)
include_directories(${rocjpeg_INCLUDE_DIR} ${ROCM_PATH}/share/rocjpeg/samples)
include_directories(SYSTEM ${ROCM_PATH}/include ${rocjpeg_INCLUDE_DIR} ${ROCM_PATH}/share/rocjpeg/samples)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Comment thread tests/CMakeLists.txt
# find requirements (optional to allow skipping tests)
find_package(rocdecode 1.0.0 QUIET)
find_package(rocjpeg 1.0.0 QUIET)
find_package(rocdecode QUIET)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same comment as other. Why are we removing versioning check?

Comment thread CMakeLists.txt
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
list(APPEND ROCPYDECODE_WARNING_FLAGS
-Wall
-Wextra
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are not enabling extra and pedantic on our other libraries. Some of the numeric check with -Wextra are too strict. We should discuss this.

using namespace py::literals;

namespace {
template <typename Target, typename Source>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All this seems like some numeric checks needed by -Wextra. Not sure we want to enable it. We have only -Wall so far in other libraries. let us discuss

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

Labels

ci:precheckin enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants