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

[openimageio] Skip building tests #30570

Merged
merged 2 commits into from
Apr 15, 2023

Conversation

MonicaLiu0311
Copy link
Contributor

@MonicaLiu0311 MonicaLiu0311 commented Mar 31, 2023

Disable the option OIIO_BUILD_TESTS to fix the following errors:

FAILED: bin/imagebufalgo_test 
/usr/bin/ld: /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_core.a(_avx_d__trmm_rut.o): in function `mkl_blas_avx_dtrmm_rut':
_trmm_rut.f:(.text+0x818f): undefined reference to `mkl_blas_daxpy'
/usr/bin/ld: /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_core.a(_avx_s__trmm_rut.o): in function `mkl_blas_avx_strmm_rut':
_trmm_rut.f:(.text+0x7c1c): undefined reference to `mkl_blas_saxpy'
/usr/bin/ld: /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_core.a(_avx512_domatcopy.o): in function `mkl_trans_avx512_mkl_domatcopy':
domatcopy.c:(.text+0x61): undefined reference to `mkl_trans_mkl_domatcopy2_par'
/usr/bin/ld: /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_core.a(_def_somatcopy.o): in function `mkl_trans_def_mkl_somatcopy':
somatcopy.c:(.text+0x61): undefined reference to `mkl_trans_mkl_somatcopy2_par'
collect2: error: ld returned 1 exit status

Repro steps:

  1. ./vcpkg install intel-mkl:x64-linux
  2. ./vcpkg install opencv4[lapack]:x64-linux
  3. ./vcpkg install openimageio[opencv]:x64-linux
  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@MonicaLiu0311 MonicaLiu0311 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Mar 31, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Mar 31, 2023

This may hide the problem when building openimageio. But at that point the intel-mlk dependency has already made its way into the binaries. So it will eventually hit downstreams.

In addition, the problem can be in another port. There is no hint that openimageio picks intel-mkl. The context points at opencv4.

@FrankXie05
Copy link
Contributor

Log from install-x64-linux-dbg-out.log

[272/294] : && /usr/bin/c++ -fPIC -g  src/libutil/CMakeFiles/strongparam_test.dir/strongparam_test_autogen/mocs_compilation.cpp.o src/libutil/CMakeFiles/strongparam_test.dir/strongparam_test.cpp.o -o bin/strongparam_test -L/mnt/vcpkg-ci/installed/x64-linux/debug/lib -Wl,-rpath,/mnt/vcpkg-ci/installed/x64-linux/debug/lib  
...
...
 /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_intel_lp64.a    
 /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_core.a  
 /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_intel_lp64.a  
 /mnt/vcpkg-ci/installed/x64-linux/lib/intel64/libmkl_sequential.a  

install-x64-linux-dbg-out.log

@MonicaLiu0311 MonicaLiu0311 marked this pull request as ready for review March 31, 2023 07:08
@dg0yt
Copy link
Contributor

dg0yt commented Mar 31, 2023

I already took my arguments from CI's set of openimageio logs. I checked the config and CMakeCache log files before posting. So let me repeat the conclusion:

There is no hint that openimageio picks intel-mkl. The context points at opencv4.

To clarify: There is no observable config step in openimageio that picks intel-mkl. It is just passed through as a transitive dependency, in non-cached variables. In the build step, the intel-mkl libs appear next to opencv4 libs.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 31, 2023

It is just passed through as a transitive dependency, in non-cached variables.

To verify that, I suggest running a build of openimageio with --trace-expand so that non-cached variables become observable.

@MonicaLiu0311
Copy link
Contributor Author

MonicaLiu0311 commented Apr 4, 2023

Run the command:
./vcpkg install openimageio:x64-windows --debug --debug-env --x-cmake-args=-DVCPKG_CMAKE_CONFIGURE_OPTIONS=--trace-expand

The log is as follows:
--debug.txt
config-x64-windows-out.log
config-x64-linux-out.log

@dg0yt Do you have any good suggestions? Thanks in advance.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 5, 2023

--trace-expand is a cmake parameter which can be added to vcpkg_cmake_configure(... OPTIONS ... ) in portfile.cmake.

@MonicaLiu0311
Copy link
Contributor Author

MonicaLiu0311 commented Apr 6, 2023

--trace-expand is a cmake parameter which can be added to vcpkg_cmake_configure(... OPTIONS ... ) in portfile.cmake.

@dg0yt Yes, I have done that, I found that both ways have the same effect, the log is as above.
Method 1:
./vcpkg install openimageio:x64-windows --x-cmake-args=-DVCPKG_CMAKE_CONFIGURE_OPTIONS=--trace-expand
Method 2:
--trace-expand is a cmake parameter which can be added to vcpkg_cmake_configure(... OPTIONS ... ) in portfile.cmake.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 7, 2023

the log is as above.

I only get an empty file when I try to download the config log linked in #30570 (comment).

@MonicaLiu0311
Copy link
Contributor Author

I only get an empty file when I try to download the config log linked in #30570 (comment).

! 😅 ...replaced by config-x64-windows-out.log, please check again

@dg0yt
Copy link
Contributor

dg0yt commented Apr 7, 2023

Hm, 12 MB, but no pointers to intel mkl.

@BillyONeal
Copy link
Member

There is no hint that openimageio picks intel-mkl. The context points at opencv4.

I think what is going on is that #29437 now installs and makes available intel-mkl on Linux boxes, which was not available there before. FindBLAS.cmake and FindLAPACK.cmake that come with cmake will pick mkl if it is available. This explains the 'nondeterminism' -- if intel-mkl installs second, a different BLAS/Lapack gets picked, but if it installs first, mkl is picked.

And once again everyone wanting to provide BLAS/Lapack curb stomps our face.

@Neumann-A
Copy link
Contributor

And once again everyone wanting to provide BLAS/Lapack curb stomps our face.

That happens if you dont lock-in the blas/lapack vendor although the dep logic requires it. There is a reason #24327 exists

@dg0yt
Copy link
Contributor

dg0yt commented Apr 11, 2023

FindBLAS.cmake and FindLAPACK.cmake that come with cmake will pick mkl if it is available. This explains the 'nondeterminism' -- if intel-mkl installs second, a different BLAS/Lapack gets picked, but if it installs first, mkl is picked.

Unless port blas is installed, too, #30421.
openimageio/opencv4 is hit due to opencv not using standard CMake find modules.

(vcpkg also can't wrap find_library. What could help is not putting alternatives in the same installation prefix, but using more individual prefixes, adding them as needed when declared as a dependency.)

@Cheney-W
Copy link
Contributor

@BillyONeal
The ci baseline issue: openimageio:x64-linux is blocking more and more PRs. Since the root cause of this issue is not on openimageio, can we merge this PR to eliminate the blocking?

@BillyONeal
Copy link
Member

I agree with @dg0yt that this is probably hiding a problem, but not building tests is consistently something we consider a good thing so I think we should take this regardless of how we slay the blas/lapack monster.

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Apr 14, 2023
BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Apr 15, 2023
@BillyONeal BillyONeal changed the title [baseline][openimageio] Fix CI Pipelines builds on linux [openimageio] Skip building tests Apr 15, 2023
@BillyONeal
Copy link
Member

Despite offering a competing resolution of the problem in #30883 I'm merging this because we generally consider not building tests something ports should be doing, and I think it has value from that aspect alone.

Thanks for digging and sorry running this down was so painful @MonicaLiu0311 :)

@BillyONeal BillyONeal merged commit 9530141 into microsoft:master Apr 15, 2023
@MonicaLiu0311 MonicaLiu0311 deleted the Dev/Monica/fix_openimageio branch April 17, 2023 02:27
BillyONeal added a commit that referenced this pull request Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants