-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
[GHA] VS 2022 #28520
base: master
Are you sure you want to change the base?
[GHA] VS 2022 #28520
Conversation
uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 # v1.13.0 | ||
with: | ||
toolset: 14.29 | ||
toolset: 14.40 # v2022 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to ensure that places where OV provider is used, also use VS 2022, otherwise can have issues like in GenAI when Pybind11 code cannot recognize ov::Tensor compiled with different compiler version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right - openvinotoolkit/openvino.genai#1598
|
||
add_custom_target(ie_wheel ALL DEPENDS ${openvino_wheel_path}) | ||
|
||
if(NOT WIN32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it needed?
I suppose you can play with warning message to be more accurate or skipped per platform:
message(WARNING "Failed to find 'fdupes' tool, use 'sudo apt-get install fdupes' to install it")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to use fdupes on Windows? If no, we don't need to execute this custom command on Windows at all
@@ -147,7 +147,7 @@ add_custom_command(OUTPUT "${fdupes_report}" | |||
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" | |||
COMMENT "Run 'fdupes' checks for wheel ${openvino_wheel_name}" | |||
VERBATIM) | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, revert changes in this file
and configure your IDE to drop trailing spaces on file saving ;)
@@ -12,7 +12,13 @@ endforeach() | |||
|
|||
find_program(fdupes_PROGRAM NAMES fdupes DOC "Path to fdupes") | |||
if(NOT fdupes_PROGRAM) | |||
message(WARNING "Failed to find 'fdupes' tool, use 'sudo apt-get install fdupes' to install it") | |||
set(fdupes_install_msg "refer to your platform's package manager or install it manually.") | |||
if(Linux) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linux
is not cmake variable.
since src/bindings/python/wheel/fdupes_check.cmake
is a separate file, I am not sure that LINUX
(defined here https://github.com/openvinotoolkit/openvino/blob/master/cmake/developer_package/target_flags.cmake#L85-L87) is available in this file.
BTW, Linux
is a target platform, while you can compile from macOS to Linux.
So, you need to make condition depending on host platform:
Details:
Tickets: