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

[arrayfire] New Port #14240

Merged
merged 28 commits into from Nov 19, 2020
Merged

[arrayfire] New Port #14240

merged 28 commits into from Nov 19, 2020

Conversation

jacobkahn
Copy link
Contributor

@jacobkahn jacobkahn commented Oct 26, 2020

Adds a new port for ArrayFire. Ports all backends -- CUDA, OpenCL, and CPU.

cc @9prady9 @umar456 to join the discussion here.

cc @padentomasello, @tlikhomanenko, @vineelpratap, @syhw, @andresy

Some things to note:

  1. Depends on CUDA, cuDNN (soft) and MKL -- uses the corresponding vcpkg ports. Not sure how this affects CI here.
  2. ArrayFire relies on several submodules.
    a) Some of these (e.g. those for testing) aren't required and aren't present in this port.
    b) Most of the submodules that are required don't have ports and probably won't (these are ArrayFire specific projects).
    c) Some things do have ports, e.g. NVIDIA Cub. The solution here is probably to ultimately change the ArrayFire build to look for Cub on the existing system rather than downloading it to prevent potential conflicts with other downstream dependencies that require Cub, although in my experience, this hasn't been an issue.
    d) As a stopgap measure to solve these issues, the vcpkg config uses vcpkg_from_github to clone submodules as needed them moves them into their expected directories via CMake so that they can be built from source normally with calls to add_subdirectory from ArrayFire CMake things.
  3. This is a port of 3.7.1, a recent stable release. We can update the port to 3.7.3/3.8 once those releases are out.
  4. Backends are features, as in ./vcpkg install arrayfire[cuda]. Multiple backends/features can be installed at once.
  5. Only installs libafcuda, libafcpu, or libafopencl, as specified. Doesn't install examples, tests, or other things, although this might be useful down the road.
  6. Doesn't build with forge/AF graphics features. This is probably something that can be added or changed later given that most of the needed ports are here and the forge submodule is manually cloned and moved to the right place.

Which triplets are supported/not supported? Have you updated the CI baseline?

Have tested x64-linux. Might want some help testing the others.

Does your PR follow the maintainer guide?

Yes

@PhoebeHui PhoebeHui self-assigned this Oct 27, 2020
@PhoebeHui PhoebeHui added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:author-response labels Oct 27, 2020
@PhoebeHui
Copy link
Contributor

@jacobkahn, could you format vcpkg.json file firstly?

See github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#manifest for solution.

ports/arrayfire/portfile.cmake Outdated Show resolved Hide resolved
@jacobkahn
Copy link
Contributor Author

jacobkahn commented Oct 28, 2020

@JackBoosY @PhoebeHui — I've removed all of the submodule dependencies as mentioned (besides of arrayfire/threads, which is a standalone library with dependencies and has no vcpkg port).

A patch is needed for now to update the build config to search for these libraries rather than relying on their existing as submodules. Hopefully there can be more flexibility with the build in future ArrayFire releases such that we don't need the patch, cc @umar456 @9prady9.

I'm having trouble parsing the CI failures — if you have any idea what's causing them, some help would be appreciated.

@PhoebeHui
Copy link
Contributor

@jacobkahn, here is CI test failure:

x64-windows and x64-windows-static:

-- Performing post-build validation
The following cmake files were found outside /share/arrayfire. Please place cmake files in /share/arrayfire.

    D:/packages/arrayfire_x64-windows/cmake/ArrayFireConfig.cmake
    D:/packages/arrayfire_x64-windows/cmake/ArrayFireConfigVersion.cmake
    D:/packages/arrayfire_x64-windows/cmake/ArrayFireUnifiedTargets-release.cmake
    D:/packages/arrayfire_x64-windows/cmake/ArrayFireUnifiedTargets.cmake
    D:/packages/arrayfire_x64-windows/debug/cmake/ArrayFireConfig.cmake
    D:/packages/arrayfire_x64-windows/debug/cmake/ArrayFireConfigVersion.cmake
    D:/packages/arrayfire_x64-windows/debug/cmake/ArrayFireUnifiedTargets-debug.cmake
    D:/packages/arrayfire_x64-windows/debug/cmake/ArrayFireUnifiedTargets.cmake

The following dlls were found in /lib or /debug/lib. Please move them to /bin or /debug/bin, respectively.
    D:/packages/arrayfire_x64-windows/lib/af.dll
The following dlls were found in /lib or /debug/lib. Please move them to /bin or /debug/bin, respectively.
    D:/packages/arrayfire_x64-windows/debug/lib/af.dll
Found 3 error(s). Please correct the portfile:
    C:\a\1\s\ports\arrayfire\portfile.cmake

x64-osx:

In file included from /Users/vagrant/Data/installed/x64-osx/include/boost/stacktrace/safe_dump_to.hpp:217:
/Users/vagrant/Data/installed/x64-osx/include/boost/stacktrace/detail/collect_unwind.ipp:33:2: error: "Boost.Stacktrace requires `_Unwind_Backtrace` function. Define `_GNU_SOURCE` macro or `BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED` if _Unwind_Backtrace is available without `_GNU_SOURCE`."
#error "Boost.Stacktrace requires `_Unwind_Backtrace` function. Define `_GNU_SOURCE` macro or `BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED` if _Unwind_Backtrace is available without `_GNU_SOURCE`."
 ^
1 error generated.

failure logs for x64-osx (13).zip

@PhoebeHui
Copy link
Contributor

PhoebeHui commented Oct 29, 2020

For the post build check failrues, you can use the vcpkg_fixup_cmake_targets(CONFIG_PATH cmake) to fix the cmake targets issue.
For the af.dll issue, you need to patch the CMakeLists file to install the dll file to bin folder, it looks AF_INSTALL_BIN_DIR not defined so it moves the dll to the lib folder.

ports/arrayfire/portfile.cmake Outdated Show resolved Hide resolved
ports/arrayfire/portfile.cmake Outdated Show resolved Hide resolved
@PhoebeHui
Copy link
Contributor

The latest CI test failures:
x64-windows: you need to check why it doesn't generate for debug configuration.

-- Performing post-build validation
Mismatching number of debug and release binaries. Found 0 for debug but 1 for release.
Debug binaries
Release binaries
    D:/packages/arrayfire_x64-windows/bin/af.dll
Debug binaries were not found
Found 1 error(s). Please correct the portfile

x64-windows-static: you need to check why it generated in a static build, and patch the CMakeLists.txt file to fix this issue, rather that removing the bin folder.

DLLs should not be present in a static build, but the following DLLs were found:
    D:/packages/arrayfire_x64-windows-static/bin/af.dll

There should be no bin\ directory in a static build, but D:\packages\arrayfire_x64-windows-static\bin is present.
If the creation of bin\ and/or debug\bin\ cannot be disabled, use this in the portfile to remove them
    if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
        file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/bin" "${CURRENT_PACKAGES_DIR}/debug/bin")
    endif()
Found 2 error(s). Please correct the portfile

- Make the unified backend its own feature
- Make the unified and CPU backend default features
- Conditionally create a bin directory if a Windows non-static build for the unified dll
- Remove fftw dependency (relies on MKL)
- Only support x64
@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Nov 18, 2020
@PhoebeHui PhoebeHui assigned strega-nil and unassigned JackBoosY Nov 18, 2020
@PhoebeHui
Copy link
Contributor

Thanks all for making this possible!

@strega-nil, could you please help review and merge this PR?

All features test passed with x64-windows, tested by @JonLiu1993
All features test passed with x64-linux, tested by @jacobkahn

ports/arrayfire/portfile.cmake Outdated Show resolved Hide resolved
Comment on lines 61 to 62
endif()
if(EXISTS ${CURRENT_PACKAGES_DIR}/share/ArrayFire/cmake)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an elseif()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @strega-nil — this can be elseif(), sure, since this condition won't be met on Windows.

ports/arrayfire/portfile.cmake Outdated Show resolved Hide resolved
ports/arrayfire/portfile.cmake Outdated Show resolved Hide resolved
ports/arrayfire/portfile.cmake Outdated Show resolved Hide resolved
@strega-nil
Copy link
Contributor

@jacobkahn could you respond to whether the endif();if() pair should be an elseif()? then I'll merge :)

@jacobkahn
Copy link
Contributor Author

jacobkahn commented Nov 18, 2020

@strega-nil — just responded. Thanks for the review and those fixes!

Looks like the osx CI is hanging again...

@jacobkahn
Copy link
Contributor Author

jacobkahn commented Nov 18, 2020

@strega-nil — it looks like that fixup targets call isn't going to work after all — sending a fix in a moment.

@strega-nil
Copy link
Contributor

@jacobkahn alright, cool, thanks!

@jacobkahn
Copy link
Contributor Author

jacobkahn commented Nov 19, 2020

Looks like CI failed because of a transitive error in the qt5 package, cc @strega-nil ?

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 14240 in repo microsoft/vcpkg

@azure-pipelines
Copy link

Command 'run

Looks' is not supported by Azure Pipelines.



Supported commands

  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

The v3.7 branch tip was still outputting shared objects with 3.7.2 versions attached to them. It's now fixed
@strega-nil
Copy link
Contributor

Alright, this looks great :) Thanks @jacobkahn :)

@strega-nil strega-nil merged commit 4297ade into microsoft:master Nov 19, 2020
@jacobkahn jacobkahn deleted the arrayfire branch November 19, 2020 22:28
@jinggaizi
Copy link

when i install arrayfire with vcpkg ;
CMake Error at scripts/cmake/vcpkg_execute_build_process.cmake:145 (message):
Command failed: vcpkg/downloads/tools/cmake-3.18.4-linux/cmake-3.18.4-Linux-x86_64/bin/cmake --build . --config Debug --target install -- -v -j41
Working Directory:cpkg/buildtrees/arrayfire/x64-linux-dbg
See logs for more information:
vcpkg/buildtrees/arrayfire/install-x64-linux-dbg-out.log

i check the install-x64-linux-dbg-out.log ,it appear "ninja: build stopped: subcommand failed" end of this file

@JackBoosY
Copy link
Contributor

@jinggaizi Can you open a new issue to report that?
Note you also should provide the full error logs.

Thanks.

@jinggaizi
Copy link

@jinggaizi Can you open a new issue to report that?
Note you also should provide the full error logs.

Thanks.

thanks for your reply , i use gcc version 6.3.1 and fix the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants