Skip to content

Conversation

olpipi
Copy link
Collaborator

@olpipi olpipi commented Aug 14, 2025

  • Isolate include of openvino cmakes via find_package. Wrap this find_package into func to not pollute outer cmake environment.
  • Write target creation helpers to define building dirs correctly and eliminate extra boilerplate

CVS-146355

@olpipi olpipi requested review from Copilot and Wovchena and removed request for Copilot August 14, 2025 14:48
@olpipi olpipi force-pushed the build_samples_dir branch from 194a52d to 23b87b5 Compare August 14, 2025 14:51
@Copilot Copilot AI review requested due to automatic review settings August 14, 2025 14:51
Copilot

This comment was marked as outdated.

@github-actions github-actions bot added the category: Python API Python API for GenAI label Aug 19, 2025
@olpipi olpipi requested a review from Wovchena August 19, 2025 12:11
@Wovchena Wovchena requested a review from Copilot August 19, 2025 14:49
Copilot

This comment was marked as outdated.

@olpipi olpipi force-pushed the build_samples_dir branch 5 times, most recently from 75b44cc to 028f3b1 Compare August 21, 2025 16:33
@olpipi olpipi requested review from Wovchena and Copilot August 21, 2025 16:34
Copilot

This comment was marked as outdated.

@Wovchena Wovchena requested a review from Copilot September 10, 2025 10:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces CMake target creation helpers to streamline build system configuration and eliminate boilerplate code. It encapsulates OpenVINO find_package calls in a function to prevent cmake environment pollution and centralizes output directory management.

  • Added helper functions genai_add_target, genai_add_tool_target, and ov_add_sample for declarative target creation
  • Centralized output directory definitions in global_properties.cmake with configurable variables
  • Replaced repetitive CMakeLists.txt patterns across samples and tools with function calls

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
cmake/add_target_helpers.cmake Implements new target creation helper functions with declarative API
cmake/global_properties.cmake Defines centralized output directory variables for different target types
CMakeLists.txt Wraps OpenVINO find_package in load_openvino() function and includes new helpers
samples/CMakeLists.txt Adds ov_add_sample function and output directory configuration
tools/continuous_batching/*/CMakeLists.txt Migrates to genai_add_tool_target helper function
samples/*/CMakeLists.txt Migrates sample targets to use ov_add_sample helper function
src/*/CMakeLists.txt Updates to use centralized output directory variables
tests/cpp/CMakeLists.txt Adds output directory configuration for test targets

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/cpp/CMakeLists.txt:1

  • [nitpick] Missing trailing whitespace was removed which improves formatting consistency.
# Copyright (C) 2018-2025 Intel Corporation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
target_link_options(${TEST_TARGET_NAME} PRIVATE /IGNORE:4207,4286)
endif()

Copy link
Preview

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

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

[nitpick] Trailing whitespace was removed which improves formatting consistency.

Copilot uses AI. Check for mistakes.

Comment on lines +194 to +196
ARCHIVE_OUTPUT_DIRECTORY "${GENAI_ARCHIVE_OUTPUT_DIRECTORY}"
LIBRARY_OUTPUT_DIRECTORY "${GENAI_LIBRARY_OUTPUT_DIRECTORY}"
RUNTIME_OUTPUT_DIRECTORY "${GENAI_RUNTIME_OUTPUT_DIRECTORY}"
Copy link
Preview

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

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

The generator expressions $<1:...> were removed from these properties. This could cause issues if these variables don't contain generator expressions themselves, as the original code used generator expressions consistently throughout the project.

Suggested change
ARCHIVE_OUTPUT_DIRECTORY "${GENAI_ARCHIVE_OUTPUT_DIRECTORY}"
LIBRARY_OUTPUT_DIRECTORY "${GENAI_LIBRARY_OUTPUT_DIRECTORY}"
RUNTIME_OUTPUT_DIRECTORY "${GENAI_RUNTIME_OUTPUT_DIRECTORY}"
ARCHIVE_OUTPUT_DIRECTORY "$<1:${GENAI_ARCHIVE_OUTPUT_DIRECTORY}>"
LIBRARY_OUTPUT_DIRECTORY "$<1:${GENAI_LIBRARY_OUTPUT_DIRECTORY}>"
RUNTIME_OUTPUT_DIRECTORY "$<1:${GENAI_RUNTIME_OUTPUT_DIRECTORY}>"

Copilot uses AI. Check for mistakes.

Comment on lines +43 to +49
add_custom_command(
TARGET ${TARGET_NAME} POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy
"${CMAKE_CURRENT_SOURCE_DIR}/openvino_genai/__init__.py"
"${CMAKE_CURRENT_SOURCE_DIR}/openvino_genai/__init__.pyi"
"${CMAKE_CURRENT_SOURCE_DIR}/openvino_genai/py_openvino_genai.pyi"
"${GENAI_LIBRARY_OUTPUT_DIRECTORY}")
Copy link
Preview

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

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

The file copy operation has been changed from a configure-time file(COPY ...) to a build-time add_custom_command. This could cause build failures if GENAI_LIBRARY_OUTPUT_DIRECTORY doesn't exist at build time, since the custom command won't create the destination directory.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

@Wovchena Wovchena left a comment

Choose a reason for hiding this comment

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

Please verify after couple of days that the nightly archive samples build isn't broken

@Wovchena Wovchena enabled auto-merge September 11, 2025 07:17
@Wovchena Wovchena added this pull request to the merge queue Sep 11, 2025
auto-merge was automatically disabled September 11, 2025 09:48

Pull Request is not mergeable

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 11, 2025
@olpipi olpipi added this pull request to the merge queue Sep 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 12, 2025
@olpipi olpipi added this pull request to the merge queue Sep 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 12, 2025
PATHS "${OpenVINO_DIR_PY}")
endif()
endfunction()
load_openvino()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any other places where this function is used?

set_target_properties(${TARGET_NAME} PROPERTIES
ARCHIVE_OUTPUT_DIRECTORY "$<1:${CMAKE_BINARY_DIR}/openvino_genai/>"
LIBRARY_OUTPUT_DIRECTORY "$<1:${CMAKE_BINARY_DIR}/openvino_genai/>"
ARCHIVE_OUTPUT_DIRECTORY "$<1:${GENAI_ARCHIVE_OUTPUT_DIRECTORY}>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we can't use PROJECT_<>_DIR (BINARY, ARCHIVE etc) here instead?

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.

3 participants