Conversation
edgchen1
reviewed
Dec 9, 2022
Contributor
Author
|
/azp run orttraining-linux-gpu-ci-pipeline |
|
Azure Pipelines successfully started running 1 pipeline(s). |
edgchen1
reviewed
Dec 9, 2022
| if(onnxruntime_external_lib_include_dir MATCHES "^\\$<BUILD_INTERFACE:([^>]+)>$") | ||
| string(REGEX REPLACE "^\\$<BUILD_INTERFACE:([^>]+)>$" "\\1" onnxruntime_external_lib_include_dir_cmake "${onnxruntime_external_lib_include_dir}") | ||
| cmake_path(NATIVE_PATH onnxruntime_external_lib_include_dir_cmake NORMALIZE onnxruntime_external_lib_include_dir_native) | ||
| target_compile_options(${target_name} PRIVATE "$<$<NOT:$<COMPILE_LANGUAGE:CUDA>>:/external:I${onnxruntime_external_lib_include_dir_native}>") |
Contributor
There was a problem hiding this comment.
is the problem with nested generator expressions? would something like https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:GENEX_EVAL help?
Contributor
Author
There was a problem hiding this comment.
Some of the include dirs are empty. For example, it may have things like "$<BUILD_INTERFACE:>", which will be evaluated as an empty string. Then this "/external:I" probably doesn't have a value, and it may misuse another thing after it as the value.
Contributor
There was a problem hiding this comment.
I see, thanks. might be useful to include a comment explaining why we need to do this
edgchen1
approved these changes
Dec 9, 2022
baijumeswani
pushed a commit
that referenced
this pull request
Dec 13, 2022
### Description Improve the handling of "/external:I". The "onnxruntime_external_lib_include_dir" variable may be: 1. A simple file path 2. A cmake generator expression like "$<INSTALL_INTERFACE:include>", "$<TARGET_PROPERTY:onnx_proto,INTERFACE_INCLUDE_DIRECTORIES>", "$<BUILD_INTERFACE:xxxx>". It seems that we can't simply put them in to the "target_compile_options" line. So this PR tries to parse the expression and extract the part we need out. ### Motivation and Context Resolve the Github issue: #13893
simon-moo
pushed a commit
to simon-moo/onnxruntime
that referenced
this pull request
Dec 26, 2022
### Description Improve the handling of "/external:I". The "onnxruntime_external_lib_include_dir" variable may be: 1. A simple file path 2. A cmake generator expression like "$<INSTALL_INTERFACE:include>", "$<TARGET_PROPERTY:onnx_proto,INTERFACE_INCLUDE_DIRECTORIES>", "$<BUILD_INTERFACE:xxxx>". It seems that we can't simply put them in to the "target_compile_options" line. So this PR tries to parse the expression and extract the part we need out. ### Motivation and Context Resolve the Github issue: microsoft#13893
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Improve the handling of "/external:I". The "onnxruntime_external_lib_include_dir" variable may be:
Motivation and Context