Skip to content

[blend2d] add port#6110

Merged
grdowns merged 1 commit intomicrosoft:masterfrom
ZeeWanderer:np_blend2d
May 20, 2019
Merged

[blend2d] add port#6110
grdowns merged 1 commit intomicrosoft:masterfrom
ZeeWanderer:np_blend2d

Conversation

@ZeeWanderer
Copy link
Contributor

@ZeeWanderer ZeeWanderer commented Apr 16, 2019

Fixes #6051

  • port version beta_2019-04-22
  • same versioning as in asmjit package with the addition of beta_ prefix

The decision to download and copy another library to source is questionable but:

  • This library depends on next-wip branch of asmjit and only asmjit master branch package is available at the moment.
  • This library depends on CmakeLists.txt from asmjit

@msftclas
Copy link

msftclas commented Apr 16, 2019

CLA assistant check
All CLA requirements met.

@ZeeWanderer ZeeWanderer changed the title [blend2d] add port [WIP][blend2d] add port Apr 16, 2019
@ZeeWanderer ZeeWanderer changed the title [WIP][blend2d] add port [blend2d] add port Apr 16, 2019
@grdowns grdowns self-assigned this Apr 16, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

string(COMPARE EQUAL "${VCPKG_LIBRARY_LINKAGE}" "static" BLEND2D_BUILD_STATIC)

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS
        -DBLEND2D_BUILD_STATIC=${BLEND2D_BUILD_STATIC}
)

@NancyLi1013
Copy link
Contributor

Hi @Maximilian578, thanks for your new port. Here are the test results from the current CI system:

x64-windows-static master test notes
blend2d Fail New Port
x64-osx master test notes
blend2d Fail New Port
arm64-windows master test notes
blend2d Fail New Port
x64-linux master test notes
blend2d Fail New Port
x64-uwp master test notes
blend2d Fail New Port
arm-uwp master test notes
blend2d Fail New Port

failureLogs.zip

@ZeeWanderer ZeeWanderer changed the title [blend2d] add port [WIP][blend2d] add port Apr 17, 2019
@grdowns grdowns added the wip label Apr 17, 2019
@ZeeWanderer
Copy link
Contributor Author

ZeeWanderer commented Apr 18, 2019

@grdowns , Hi.

  • I successfully installed blend2d for x64-linux triplet on Ubuntu 18.04.1 LTS VM
  • I successfully installed blend2d for x64-osx triplet on macOS Mojave 10.14.4 VM . (Had to install gcc6 because vcpkg failed to build with AppleClang 10.0.1.10010046)

Can you forward me anything more on the errors?
Also as far as i know, for now blend2d does not support arm and uwp so those will not work.

@NancyLi1013
Copy link
Contributor

@Maximilian578, thanks for your effort on this new port. I checked the test results from the current CI system again and all triplets build successfully except for arm64-windows, x64-uwp and arm-uwp. As blend2d does not support arm and uwp, it doesn't matter now.

@ZeeWanderer ZeeWanderer changed the title [WIP][blend2d] add port [blend2d] add port Apr 19, 2019
@ZeeWanderer
Copy link
Contributor Author

ZeeWanderer commented Apr 21, 2019

If the solution with asmjit sources and unavailability for arm and uwp is acceptable then this pr is ready for review and merge.

EDIT: Merging this pr is taking a lot longer then i expected. I fixed static builds upstream, so i will update the port. If it passes CI tests except for arm and uwp then it is ready to merge.

EDIT: Port updated.

@grdowns
Copy link
Contributor

grdowns commented Apr 29, 2019

Hey @Maximilian578! Sorry for taking so long to get to this, and thanks for the patience. The PR looks good, but I think that the dependency handling as far as asmjit is concerned could be improved to avoid splitting the build scenario. The correct behavior would be to expose the next-wip branch as a feature in the asmjit port itself, then modify the CONTROL file to depend on asmjit[next-wip]. Lastly if any patching is required to disable requiring the CMakeLists.txt we can do that too. I'll try to make some progress on this soon. Thanks for the PR!

@ZeeWanderer
Copy link
Contributor Author

ZeeWanderer commented Apr 29, 2019

@grdowns locally I added asmjit feature and tried to make a patch for asmjit's CmakeLists.txt dependency but blend2d has trouble linking agains asmjit.dll for x64-windows so only x64-windows-static works and i wonder if i really should use these changes for a few reasons:

  • According to asmjit/blend2d author, the preferred way is embedding asmjit/blend2d. Blend2d puts emphasis on performance.
  • Even if we do not embed asmjit it is advised to build it as static lib which is troublesome for x64-windows. The same is advised for blend2d
    • Should i build asmit and blend2d as static regardless of triplet?
  • Because of preferred asmjit embedding current dependency on asmjit's CmakeLists.txt is made in such a way that if removed would prove troublesome to maintain. Currently, i just embed relevant parts into blend2d's CmakeLists.txt.
  • Asmjit next-wip and master branches are mutually excluding so strictly speaking next-wip as a feature is not really a sane solution.

EDIT: I decided to push wip commit for review but i think that what i came up with is not a sane solution. While embedding looks like a crutch it is saner.

On a side note: Have embedding or dynamic-on-static dependencies been considered? Are those even feasible for a package manager?

@ZeeWanderer
Copy link
Contributor Author

I reverted to old port implementation since according to author clang compiled windows binaries will be packed with headers and hosted for windows users when library comes out of beta, probably (reason for clang). So then the port will be chaged to use archive instead of github repo, but for now this is sufficient.

 - port version `beta_2019-04-30`
 - same versioning as in asmjit pakage with addition of `beta_` prefix
@grdowns
Copy link
Contributor

grdowns commented May 20, 2019

Hey @Maximilian578, sorry for the long delay. Those are very good points with respect to maintenance and linking/performance concerns, and so I believe the best way way to add this port is as-is. Thanks for your patience and thanks again for the new port contribution!

@grdowns grdowns merged commit 3480a13 into microsoft:master May 20, 2019
@ZeeWanderer ZeeWanderer deleted the np_blend2d branch May 27, 2019 15:16
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.

[New port request] blend2d

5 participants