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.

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

[ROCm][MIGraphX] for googletest dep, set OVERRIDE_FIND_PACKAGE #16715

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

jeffdaily
Copy link
Contributor

Otherwise, an unsupported version of gtest/gmock will be found at /opt/conda/include for ROCm builds. Though this issue was initially found for ROCm builds, the issue is generic. onnxruntime requires a specific version of googletest and should not rely on locating googletest using find_package.

The ROCm error was:

In file included from /opt/conda/include/gmock/gmock-spec-builders.h:75,
                 from /opt/conda/include/gmock/gmock-generated-function-mockers.h:47,
                 from /opt/conda/include/gmock/gmock-function-mocker.h:39,
                 from /opt/conda/include/gmock/gmock.h:61,
                 from /stage/onnxruntime/onnxruntime/test/util/test_utils.cc:17:
/opt/conda/include/gmock/gmock-matchers.h: In instantiation of ‘bool testing::internal::PointwiseMatcher<TupleMatcher, RhsContainer>::Impl<LhsContainer>::
MatchAndExplain(LhsContainer, testing::MatchResultListener*) const [with LhsContainer = const gsl::span<const float>&; TupleMatcher = testing::internal::
FloatingEq2Matcher<float>; RhsContainer = gsl::span<const float>]’:
/opt/conda/include/gmock/gmock-matchers.h:2303:10:   required from here
/opt/conda/include/gmock/gmock-matchers.h:2312:48: error: no type named ‘const_iterator’ in ‘testing::internal::PointwiseMatcher<testing::internal::
FloatingEq2Matcher<float>, gsl::span<const float> >::Impl<const gsl::span<const float>&>::LhsStlContainer’ {aka ‘class gsl::span<const float>’}

Otherwise, an unsupported version of gtest/gmock will be found at
/opt/conda/include for ROCm builds. Though this issue was initially
found for ROCm builds, the issue is generic. onnxruntime requires a
specific version of googletest and should not rely on locating
googletest using find_package.

The ROCm error was:

```
In file included from /opt/conda/include/gmock/gmock-spec-builders.h:75,
                 from /opt/conda/include/gmock/gmock-generated-function-mockers.h:47,
                 from /opt/conda/include/gmock/gmock-function-mocker.h:39,
                 from /opt/conda/include/gmock/gmock.h:61,
                 from /stage/onnxruntime/onnxruntime/test/util/test_utils.cc:17:
/opt/conda/include/gmock/gmock-matchers.h: In instantiation of ‘bool testing::internal::PointwiseMatcher<TupleMatcher, RhsContainer>::Impl<LhsContainer>::
MatchAndExplain(LhsContainer, testing::MatchResultListener*) const [with LhsContainer = const gsl::span<const float>&; TupleMatcher = testing::internal::
FloatingEq2Matcher<float>; RhsContainer = gsl::span<const float>]’:
/opt/conda/include/gmock/gmock-matchers.h:2303:10:   required from here
/opt/conda/include/gmock/gmock-matchers.h:2312:48: error: no type named ‘const_iterator’ in ‘testing::internal::PointwiseMatcher<testing::internal::
FloatingEq2Matcher<float>, gsl::span<const float> >::Impl<const gsl::span<const float>&>::LhsStlContainer’ {aka ‘class gsl::span<const float>’}
```
@TedThemistokleous
Copy link
Contributor

ping @PeixuanZuo @cloudhan we need this for both ROCm and MIGraphX EPs

@jeffdaily
Copy link
Contributor Author

@cloudhan and @PeixuanZuo please review. This issue will soon affect ROCm azure pipelines, if not already.

@jeffdaily jeffdaily changed the title for googletest dep, set OVERRIDE_FIND_PACKAGE [ROCmEP/MIGraphXEP] for googletest dep, set OVERRIDE_FIND_PACKAGE Jul 18, 2023
@cloudhan cloudhan requested review from snnn and mszhanyi July 18, 2023 23:33
@cloudhan
Copy link
Contributor

/azp run Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@cloudhan
Copy link
Contributor

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@microsoft microsoft deleted a comment from azure-pipelines bot Jul 19, 2023
@cloudhan cloudhan changed the title [ROCmEP/MIGraphXEP] for googletest dep, set OVERRIDE_FIND_PACKAGE [ROCm][MIGraphX] for googletest dep, set OVERRIDE_FIND_PACKAGE Jul 19, 2023
@cloudhan cloudhan merged commit bb136f8 into microsoft:main Jul 20, 2023
@snnn
Copy link
Member

snnn commented Jul 24, 2023

Generally speaking, building onnxruntime from source on Linux should be done in docker containers which doesn't have extra packages, and you should not use conda unless that you are building a package that is just for conda(won't be compatible with CPython and packages in pypi.org). Otherwise you should disable the find package feature from build command line for all such external projects, not just gtest.
We need to reconsider it. Because:
1.This change is too specific for RoCM, but it impacts everyone who integrates ORT to their package management system(like the Linux distros)
2. Googletest depends on abseil and many other things. They should all come from the same source. Either you disable find_package for all of them, or none. Otherwise the inconsistence would cause incompatible package versions being used.

Because not every OSS's cmake file could do version matching correctly and exactly, the best practice is to make the build environment clean, not have extra packages, minimize dependencies. Compared to the other build systems like Bazel, CMake is easy to use and but it is not as powerful as you may wish.

mszhanyi added a commit that referenced this pull request Jul 31, 2023
mszhanyi added a commit that referenced this pull request Aug 1, 2023
jchen351 pushed a commit that referenced this pull request Aug 12, 2023
Otherwise, an unsupported version of gtest/gmock will be found at
/opt/conda/include for ROCm builds. Though this issue was initially
found for ROCm builds, the issue is generic. onnxruntime requires a
specific version of googletest and should not rely on locating
googletest using find_package.

The ROCm error was:

```
In file included from /opt/conda/include/gmock/gmock-spec-builders.h:75,
                 from /opt/conda/include/gmock/gmock-generated-function-mockers.h:47,
                 from /opt/conda/include/gmock/gmock-function-mocker.h:39,
                 from /opt/conda/include/gmock/gmock.h:61,
                 from /stage/onnxruntime/onnxruntime/test/util/test_utils.cc:17:
/opt/conda/include/gmock/gmock-matchers.h: In instantiation of ‘bool testing::internal::PointwiseMatcher<TupleMatcher, RhsContainer>::Impl<LhsContainer>::
MatchAndExplain(LhsContainer, testing::MatchResultListener*) const [with LhsContainer = const gsl::span<const float>&; TupleMatcher = testing::internal::
FloatingEq2Matcher<float>; RhsContainer = gsl::span<const float>]’:
/opt/conda/include/gmock/gmock-matchers.h:2303:10:   required from here
/opt/conda/include/gmock/gmock-matchers.h:2312:48: error: no type named ‘const_iterator’ in ‘testing::internal::PointwiseMatcher<testing::internal::
FloatingEq2Matcher<float>, gsl::span<const float> >::Impl<const gsl::span<const float>&>::LhsStlContainer’ {aka ‘class gsl::span<const float>’}
```
prathikr pushed a commit that referenced this pull request Aug 21, 2023
Otherwise, an unsupported version of gtest/gmock will be found at
/opt/conda/include for ROCm builds. Though this issue was initially
found for ROCm builds, the issue is generic. onnxruntime requires a
specific version of googletest and should not rely on locating
googletest using find_package.

The ROCm error was:

```
In file included from /opt/conda/include/gmock/gmock-spec-builders.h:75,
                 from /opt/conda/include/gmock/gmock-generated-function-mockers.h:47,
                 from /opt/conda/include/gmock/gmock-function-mocker.h:39,
                 from /opt/conda/include/gmock/gmock.h:61,
                 from /stage/onnxruntime/onnxruntime/test/util/test_utils.cc:17:
/opt/conda/include/gmock/gmock-matchers.h: In instantiation of ‘bool testing::internal::PointwiseMatcher<TupleMatcher, RhsContainer>::Impl<LhsContainer>::
MatchAndExplain(LhsContainer, testing::MatchResultListener*) const [with LhsContainer = const gsl::span<const float>&; TupleMatcher = testing::internal::
FloatingEq2Matcher<float>; RhsContainer = gsl::span<const float>]’:
/opt/conda/include/gmock/gmock-matchers.h:2303:10:   required from here
/opt/conda/include/gmock/gmock-matchers.h:2312:48: error: no type named ‘const_iterator’ in ‘testing::internal::PointwiseMatcher<testing::internal::
FloatingEq2Matcher<float>, gsl::span<const float> >::Impl<const gsl::span<const float>&>::LhsStlContainer’ {aka ‘class gsl::span<const float>’}
```
@cbourjau
Copy link
Contributor

@jeffdaily Do you have more information about the build failure?

@snnn I noticed that this change made it into the 1.16.0 release candidate. This change seems pretty heavy-handed and breaks or at least further complicates packaging for nixpkgs and probably many other package managers. Things used to work with the same gtest version for the 1.15.1 release without the change of this PR. Is it really necessary or do yo maybe see some way to work around it?

@snnn
Copy link
Member

snnn commented Sep 12, 2023

From my perspective, it is not needed and harmful. If someone has the need to disable find_package, he/she can do it globally for all dependencies we have.

snnn added a commit that referenced this pull request Sep 12, 2023
@snnn
Copy link
Member

snnn commented Sep 12, 2023

#17523

snnn added a commit that referenced this pull request Sep 13, 2023
#16715)" (#17523)

This reverts commit bb136f8, then
re-implement it in a different way.
I reverted the original change, then added a version constraint to the
find_package args.

If you still found it picks up wrong gtest version after this change,
you may disable `find_package` by setting
'FETCHCONTENT_TRY_FIND_PACKAGE_MODE' to NEVER. For example, the latest
gtest version is 1.14.0. If at a later time Google releases a new
version of gtest and that one is incompatible with the ONNX Runtime
source code you get today and your dev environment already installed the
new version and you do not want to create a new clean build environment
that is without the package, you can add `--cmake_extra_defines
FETCHCONTENT_TRY_FIND_PACKAGE_MODE=NEVER` to your build command to solve
the problem.
centwang pushed a commit that referenced this pull request Sep 13, 2023
#16715)" (#17523)

This reverts commit bb136f8, then
re-implement it in a different way.
I reverted the original change, then added a version constraint to the
find_package args.

If you still found it picks up wrong gtest version after this change,
you may disable `find_package` by setting
'FETCHCONTENT_TRY_FIND_PACKAGE_MODE' to NEVER. For example, the latest
gtest version is 1.14.0. If at a later time Google releases a new
version of gtest and that one is incompatible with the ONNX Runtime
source code you get today and your dev environment already installed the
new version and you do not want to create a new clean build environment
that is without the package, you can add `--cmake_extra_defines
FETCHCONTENT_TRY_FIND_PACKAGE_MODE=NEVER` to your build command to solve
the problem.
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
microsoft#16715)" (microsoft#17523)

This reverts commit bb136f8, then
re-implement it in a different way.
I reverted the original change, then added a version constraint to the
find_package args.

If you still found it picks up wrong gtest version after this change,
you may disable `find_package` by setting
'FETCHCONTENT_TRY_FIND_PACKAGE_MODE' to NEVER. For example, the latest
gtest version is 1.14.0. If at a later time Google releases a new
version of gtest and that one is incompatible with the ONNX Runtime
source code you get today and your dev environment already installed the
new version and you do not want to create a new clean build environment
that is without the package, you can add `--cmake_extra_defines
FETCHCONTENT_TRY_FIND_PACKAGE_MODE=NEVER` to your build command to solve
the problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants