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

add build option USE_SYSTEM_PUGIXML #2502

Merged
merged 1 commit into from
Oct 12, 2020
Merged

add build option USE_SYSTEM_PUGIXML #2502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions inference-engine/cmake/features_ie.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,5 @@ ie_option(ENABLE_CLANG_FORMAT "Enable clang-format checks during the build" ON)
set(IE_EXTRA_PLUGINS "" CACHE STRING "Extra paths for plugins to include into DLDT build tree")

ie_dependent_option(ENABLE_TBB_RELEASE_ONLY "Only Release TBB libraries are linked to the Inference Engine binaries" ON "THREADING MATCHES TBB;LINUX" OFF)

ie_option (USE_SYSTEM_PUGIXML "use the system copy of pugixml" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

all options in this file are named as ENABLE_*, don't you think we can use ENABLE_SYSTEM_PUGIXML?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not opposed to renaming it to ENABLE_*. I think there are two minor reasons not to do so:

  • "enable" sends the wrong signal. We don't enable pugixml, we use a different one.
  • Consistency with NGRAPH_USE_SYSTEM_PROTOBUF. If changing it to "enable" here, should I also change it there?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vinograd47 @ilyachur how do you think? USE_SYSTEM_PUGIXML looks good, but not consistent..

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it is not consistent.
All nGraph options are consistent because all options have a prefix NGRAPH_.
But I also agree that USE word is better for it.
I am ok with USE_SYSTEM_PUGIXML

25 changes: 17 additions & 8 deletions inference-engine/thirdparty/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,22 +55,31 @@ function(build_with_lto)
set(BUILD_TESTS ${BUILD_TESTS_current} CACHE BOOL "Build tests" FORCE)
endfunction()

ie_build_pugixml()
if (USE_SYSTEM_PUGIXML)
find_package(PugiXML REQUIRED)
set_property(TARGET pugixml PROPERTY IMPORTED_GLOBAL TRUE)
else()
ie_build_pugixml()
target_include_directories(pugixml INTERFACE "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/pugixml/src>")
endif()
add_subdirectory(stb_lib)
add_subdirectory(ade)
add_subdirectory(fluid/modules/gapi)

target_include_directories(pugixml INTERFACE "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/pugixml/src>")

set_target_properties(pugixml ade fluid stb_image
set_target_properties(ade fluid stb_image
PROPERTIES FOLDER thirdparty)

# developer package

ie_developer_export_targets(ade fluid pugixml)
if(TARGET pugixml_mt)
ie_developer_export_targets(pugixml_mt)
set_target_properties(pugixml_mt PROPERTIES FOLDER thirdparty)
ie_developer_export_targets(ade fluid)

if (NOT USE_SYSTEM_PUGIXML)
set_target_properties(pugixml PROPERTIES FOLDER thirdparty)
ie_developer_export_targets(pugixml)
if(TARGET pugixml_mt)
ie_developer_export_targets(pugixml_mt)
set_target_properties(pugixml_mt PROPERTIES FOLDER thirdparty)
endif()
endif()

if(ENABLE_MKL_DNN)
Expand Down