Skip to content

Provide onecore compliance#2794

Closed
pshemoth wants to merge 3 commits intoopenvinotoolkit:masterfrom
pshemoth:provide_onecore_compliance
Closed

Provide onecore compliance#2794
pshemoth wants to merge 3 commits intoopenvinotoolkit:masterfrom
pshemoth:provide_onecore_compliance

Conversation

@pshemoth
Copy link
Copy Markdown

No description provided.

@pshemoth pshemoth requested a review from a team October 23, 2020 09:49
@openvino-pushbot openvino-pushbot added ExternalPR External contributor category: inference OpenVINO Runtime library - Inference labels Oct 23, 2020
set(CMAKE_CXX_STANDARD_LIBRARIES "\$\(UCRTContentRoot\)lib/\$\(TargetUniversalCRTVersion\)/um/x86/OneCoreUap.lib" CACHE STRING "" FORCE)
add_link_options("/LIBPATH:\"\$\(VCInstallDir\)lib/onecore\"")
add_link_options("/LIBPATH:\"\$\(VC_LibraryPath_VC_x86_OneCore\)\"")
endif()
Copy link
Copy Markdown
Contributor

@ilya-lavrenov ilya-lavrenov Oct 23, 2020

Choose a reason for hiding this comment

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

Current OpenVINO binaries are compliant as well. We just need to ignore msvcp140.dll, vcruntime140.dll errors when run apiValidator, is not it? what is different with and without this code?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In the project I'm participating right now there is an assumption that apiValidator doesn't take any exclusion list. t means that all errors caused by dynamic linking msvcp140 adn vcruntime make the validation failed.

CMAKE_CXX_FLAGS_MINSIZEREL
CMAKE_CXX_FLAGS_RELEASE
CMAKE_CXX_FLAGS_RELWITHDEBINFO)
string(REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}")
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.

why do we need static runtime for onecore? Where is it written?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is workaround to remove msvcp140.dll, vcruntime140.dll from one core compliance validation.

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.

Does it mean that we should use static runtime only for Windows SDK version older than "X" version, where "X" version has BinaryExclusionlist.xml list for apiValidator? As far as I can see versions older "10.0.20000.0" does not have BinaryExclusionlist.xml, but newer versions have

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.

BTW, in the current master branch we have apiValidator run as POST_BUILD step for some OpenVINO dlls. But this run takes into account that msvcp140.dll, vcruntime140.dll libraries are exclusions even for 10.0.1XYZW.0 versions

if(MSVC_ONE_CORE_COMPLIANCE)
message([STATUS] "Building OneCore Compliant Binaries")
if(DEFINED ENV{varBuildWdkVer})
# For QB or manual setting - QB sets varBuildWdkVer before build
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 is QB?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

CI Software - in fact the approach can be used locally as well.

@@ -0,0 +1,36 @@
#has to be set before project
if(MSVC_ONE_CORE_COMPLIANCE)
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.

can we move content of this script to cmake/onecoreuap.toolchain.cmake? In this case we can pass -DCMAKE_TOOLCHAIN_FILE=cmake/onecoreuap.toolchain.cmake and don't need MSVC_ONE_CORE_COMPLIANCE variable, don't need to set with force CMAKE_C_STANDARD_LIBRARIES and CMAKE_CXX_STANDARD_LIBRARIES

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

very likely - yes. I need to check side effects

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.

I have created toolchain based on these changes #3044. @pshemoth could you please try and give feedback?

set(CMAKE_C_STANDARD_LIBRARIES "\$\(UCRTContentRoot\)lib/\$\(TargetUniversalCRTVersion\)/um/x86/OneCoreUap.lib" CACHE STRING "" FORCE)
set(CMAKE_CXX_STANDARD_LIBRARIES "\$\(UCRTContentRoot\)lib/\$\(TargetUniversalCRTVersion\)/um/x86/OneCoreUap.lib" CACHE STRING "" FORCE)
add_link_options("/LIBPATH:\"\$\(VCInstallDir\)lib/onecore\"")
add_link_options("/LIBPATH:\"\$\(VC_LibraryPath_VC_x86_OneCore\)\"")
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.

do we need the same for cmake/uwp.toolchain.cmake?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

as above - very likely yes.

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.

It would be very good, if you create a new onecoreuap.toochain.cmake with proposed changes and update existing uwp.toolchain.cmake as well.

Thanks for your effort.

@ilya-lavrenov ilya-lavrenov self-assigned this Nov 2, 2020
@ilya-lavrenov ilya-lavrenov mentioned this pull request Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: inference OpenVINO Runtime library - Inference ExternalPR External contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants