Update ONNX installing script #21044
Merged
Conversation
This file contains 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
mszhanyi
approved these changes
Jun 14, 2024
yf711
pushed a commit
that referenced
this pull request
Jun 19, 2024
Avoid using command line flags to pass in CMAKE_PREFIX_PATH. Use environment variables instead. Because, otherwise the value of CMAKE_PREFIX_PATH could get encoded twice. For example, if the prefix is `C:\a\root`, then in tools/ci_build/github/windows/helpers.ps1 we set it in Env:CMAKE_ARGS which will be consumed by ONNX. Then when ONNX get it and decoded it, ONNX will get `C:aroot` instead. Then because the path doesn't exist, the CMAKE_PREFIX_PATH couldn't take effect when the script installs ONNX. This PR fixes the issue. The issue got discovered when I tried to upgrade cmake to a newer version. Now our Windows CPU CI build pipeline uses cmake 3.27. In the main branch even the CMAKE_PREFIX_PATH setting does not work, cmake still can find protoc.exe from the directories. However, starting from 3.28 cmake changed it. With the newer cmake versions the find_library(), find_path(), and find_file() cmake commands no longer search in installation prefixes derived from the PATH environment variable.
baijumeswani
pushed a commit
that referenced
this pull request
Jun 20, 2024
Avoid using command line flags to pass in CMAKE_PREFIX_PATH. Use environment variables instead. Because, otherwise the value of CMAKE_PREFIX_PATH could get encoded twice. For example, if the prefix is `C:\a\root`, then in tools/ci_build/github/windows/helpers.ps1 we set it in Env:CMAKE_ARGS which will be consumed by ONNX. Then when ONNX get it and decoded it, ONNX will get `C:aroot` instead. Then because the path doesn't exist, the CMAKE_PREFIX_PATH couldn't take effect when the script installs ONNX. This PR fixes the issue. The issue got discovered when I tried to upgrade cmake to a newer version. Now our Windows CPU CI build pipeline uses cmake 3.27. In the main branch even the CMAKE_PREFIX_PATH setting does not work, cmake still can find protoc.exe from the directories. However, starting from 3.28 cmake changed it. With the newer cmake versions the find_library(), find_path(), and find_file() cmake commands no longer search in installation prefixes derived from the PATH environment variable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Avoid using command line flags to pass in CMAKE_PREFIX_PATH. Use environment variables instead.
Because, otherwise the value of CMAKE_PREFIX_PATH could get encoded twice. For example, if the prefix is
C:\a\root, then in tools/ci_build/github/windows/helpers.ps1 we set it in Env:CMAKE_ARGS which will be consumed by ONNX. Then when ONNX get it and decoded it, ONNX will getC:arootinstead. Then because the path doesn't exist, the CMAKE_PREFIX_PATH couldn't take effect when the script installs ONNX. This PR fixes the issue.The issue got discovered when I tried to upgrade cmake to a newer version. Now our Windows CPU CI build pipeline uses cmake 3.27. In the main branch even the CMAKE_PREFIX_PATH setting does not work, cmake still can find protoc.exe from the directories. However, starting from 3.28 cmake changed it. With the newer cmake versions the find_library(), find_path(), and find_file() cmake commands no longer search in installation prefixes derived from the PATH environment variable.