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

[freetype] Make zlib and brotli features #14917

Merged
merged 3 commits into from
Dec 10, 2020

Conversation

LRFLEW
Copy link
Contributor

@LRFLEW LRFLEW commented Dec 3, 2020

I was looking to make a build of Freetype without brotli (as I don't need WOFF2, and the extra dependency increased build sizes), but it wasn't made a feature for some reason. This makes the use of zlib and brotli in freetype info features that can be excluded by users. The new features are added to the defaults so that the default build does not change. The only optional dependency not a feature now is harfbuzz, but that's because there's a circular dependency issue.

I checked to see if there was a reason why brotli wasn't made a feature before, but I couldn't find any explanation for it in #12380 or #12405.

EDIT: I see the CI errors. I'll look into them. My guess is that port dependencies need to be updated for the new features.

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

LRFLEW commented Dec 3, 2020

So the CI error seems unrelated to the PR... again. (What are the odds that two PRs of mine assigned to you in a row have that issue? 😛)

-- Downloading https://gitlab.kitware.com/vtk/vtk-m/-/archive/f2aa6ad5be1a97e3fb41ef4680ee2c76c3434ac0/vtk-m-f2aa6ad5be1a97e3fb41ef4680ee2c76c3434ac0.tar.gz...
CMake Error at scripts/cmake/vcpkg_download_distfile.cmake:105 (message):

  File does not have expected hash:

          File path: [ D:/downloads/temp/vtk-vtk-m-f2aa6ad5be1a97e3fb41ef4680ee2c76c3434ac0.tar.gz ]
      Expected hash: [ 2f2a273f74d9a583df9e25a4792440d8d89652fa14b3153f2ea5afbd329b50970e7b9bd68e0ccd036baf5c1f3ad7a8302d95c01dbb30d9a46c045987eebf5370 ]
        Actual hash: [ 35e8a2c0ad6cd3c1f02a71a50d781c89f93909ad27030b406fd69f4fea5c1862c48a6e541fd07562947322c3a69bdfdb54206ae51bb86ef7a710f9e9898e9638 ]

  The file may have been corrupted in transit.  This can be caused by
  proxies.  If you use a proxy, please set the HTTPS_PROXY and HTTP_PROXY
  environment variables to
  "https://user:password@your-proxy-ip-address:port/".

Call Stack (most recent call first):
  scripts/cmake/vcpkg_download_distfile.cmake:191 (test_hash)
  scripts/cmake/vcpkg_from_gitlab.cmake:123 (vcpkg_download_distfile)
  ports/vtk-m/portfile.cmake:40 (vcpkg_from_gitlab)
  scripts/ports.cmake:136 (include)

(empty lines were removed for brevity)

Looks like the upstream source for vtk-m silently changed the file. I can confirm the "Actual hash" is what I got downloading the file on my machine. Someone with the original file should confirm what changed and update the expected hash in the port.

@PhoebeHui
Copy link
Contributor

@LRFLEW , please ignore it, the issue would be fixed by #14884

@PhoebeHui
Copy link
Contributor

If we decide to move the dependency to default features, the cmake config file and vcpkg-cmake-wrapper.cmake need to update as well, since when users built the port with core, the dependency port will not found, and it would fail when use the port.

https://github.com/microsoft/vcpkg/blob/master/ports/freetype/fix-exports.patch
https://github.com/microsoft/vcpkg/blob/master/ports/freetype/vcpkg-cmake-wrapper.cmake

cc @petersteneteg @Neumann-A

@petersteneteg
Copy link
Contributor

Sounds good to me!

@LRFLEW LRFLEW force-pushed the freetype-features branch from b383bec to 1b44431 Compare December 3, 2020 22:38
@LRFLEW
Copy link
Contributor Author

LRFLEW commented Dec 3, 2020

I updated vcpkg-cmake-wrapper.cmake and its related configure_file call. Thanks for catching that.

I have no real idea what's going on with fix-exports.patch, though, so I'm not 100% sure how best to approach it. From what I can tell it's two changes: One replacing ${ZLIB_LIBRARIES} that the build gets from find_package(ZLIB) with the direct ZLIB::ZLIB, and the other replacing freetype-config.cmake with a stub. I don't know exactly why either change was initially made, so I'm worried about touching either change. I'll try looking through vcpkg's commit history to see if I can better understand it.

EDIT: Found #14499 and #9952

@PhoebeHui
Copy link
Contributor

@LRFLEW, the patch https://github.com/microsoft/vcpkg/blob/master/ports/freetype/fix-exports.patch write
'find_dependency(ZLIB)' in freetype-config.cmake file, so it will fail when use it as well, it also need update.

@LRFLEW
Copy link
Contributor Author

LRFLEW commented Dec 9, 2020

@PhoebeHui I added a fix for fix-exports.patch. It should be working correctly if I understand it correctly. I could probably skip the separate freetype-config.cmake and freetype-targets.cmake when Zlib is not used, but it's simpler to just not use find_dependency(ZLIB) instead.

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 updates!

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Dec 10, 2020
@BillyONeal BillyONeal merged commit c309037 into microsoft:master Dec 10, 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.

4 participants