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] Remove forge and graphics deps from port #15027

Merged
merged 1 commit into from Dec 12, 2020

Conversation

jacobkahn
Copy link
Contributor

@jacobkahn jacobkahn commented Dec 9, 2020

Removes the graphics library build from the port since it relies on forge which relies on fontconfig, which is currently broken on some platforms. cc @9prady9 @umar456

@JonLiu1993 this is a minor change, but I'll again need help testing on x64-windows. Thanks again.

  • What does your PR fix? Fixes #

Until #14864 is fixed, resolves build issues.

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

No changes. Tested x64-linux.

Yes

@JonLiu1993 JonLiu1993 added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Dec 10, 2020
@JonLiu1993
Copy link
Member

@jacobkahn ,All features test passed with x64-windows

@PhoebeHui PhoebeHui added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Dec 10, 2020
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!

@JonLiu1993 JonLiu1993 removed the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Dec 10, 2020
@jacobkahn
Copy link
Contributor Author

@strega-nil — can this be merged? :)

@strega-nil
Copy link
Contributor

LGTM, personally

@strega-nil strega-nil added the info:reviewed Pull Request changes follow basic guidelines label Dec 10, 2020
@BillyONeal
Copy link
Member

What effect does this have on someone who tries to install forge and arrayfire at the same time? Will they try to install the same things? (I tried to test this myself but failed because I don't have an intel mkl...)

@jacobkahn
Copy link
Contributor Author

jacobkahn commented Dec 12, 2020

@BillyONeal@9prady9 and @umar456 can provide more clarity and detail on this, but since Forge is runtime dependency and the Forge commit downloaded here mirrors the commit in the Forge vcpkg port, the library will be dlopened (or the platform equivalent) with symbols that are consistent with the headers used in the ArrayFire build. That is, the user can ./vcpkg install forge in conjunction with their installing ArrayFire, and as long as LD_LIBRARY_PATH-related things are correctly set, the graphics functionality will be enabled.

The only reason Forge is required in this case is because of the headers - those are used ahead of time to construct the particular symbols that ArrayFire will search for after opening libforge. That happens at compile time rather than runtime for efficiency.

The solution in this PR isn't ideal, but it at least gives users the option of not installing the graphics dependencies if they aren't as well-supported for their platform (see the issue I'm referencing above). The headers are needed to build even if libforge isn't going to be used. If #14864 is resolved and fontconfig is working in more places, we can easily revert this.

@BillyONeal
Copy link
Member

since Forge is runtime dependency and the Forge commit downloaded here mirrors the commit in the Forge vcpkg port

The problem is that if the port arrayfire tries to install libforge.so (I'm pulling this name out of my butt :) ) and forge also tries to install libforge.so that's not acceptable. Artifacts in an installed tree need unique ownership or we need to consider one the official provider of the thing and drop official support for the other. For example, boringssl and openssl, we only test openssl because boringssl cannot be concurrently installed, since they both want the same .so/.dll names.

@jacobkahn
Copy link
Contributor Author

@BillyONeal — ArrayFire never builds or installs libforge (or whatever it's called :) ) — it just needs the headers to construct some ahead-of-time symbol table (presumably this) which it uses to load libforge at runtime more efficiently if it can be located. The only thing shipping the libforge library bits is the vcpkg forge port, but the headers used in the vcpkg port are needed here so the ArrayFire loader can be populated ahead of time and know which symbols to look for in the library.

@BillyONeal
Copy link
Member

I see, sounds good then. Thanks!

@BillyONeal BillyONeal merged commit c77aad3 into microsoft:master Dec 12, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants