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

[harfbuzz] Update to 4.2.0 #24144

Merged
merged 12 commits into from
Apr 28, 2022
Merged

[harfbuzz] Update to 4.2.0 #24144

merged 12 commits into from
Apr 28, 2022

Conversation

bold84
Copy link
Contributor

@bold84 bold84 commented Apr 14, 2022

Describe the pull request

  • What does your PR fix?

    Fixes #...

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

    all

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/harfbuzz/vcpkg.json

Valid values for the license field can be found in the documentation

@JonLiu1993 JonLiu1993 self-assigned this Apr 15, 2022
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Apr 15, 2022
@JonLiu1993
Copy link
Member

It looks like ci fails the test on triples x64-windows-static and x64-windows-static-md , you can get the error log as suggested in the image below:
https://dev.azure.com/vcpkg/public/_build/results?buildId=70674&view=artifacts&pathAsName=false&type=publishedArtifacts
image

Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 99346bb6926e85d93e4aad330bf28cce4a18051b -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 9f5739a..cffe65d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2722,7 +2722,7 @@
     },
     "harfbuzz": {
       "baseline": "4.2.0",
-      "port-version": 3
+      "port-version": 0
     },
     "hayai": {
       "baseline": "2019-08-10",
diff --git a/versions/h-/harfbuzz.json b/versions/h-/harfbuzz.json
index b318da4..0444e35 100644
--- a/versions/h-/harfbuzz.json
+++ b/versions/h-/harfbuzz.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "75697d7c43494eeb0699d372f41853cc937f7a78",
+      "version-semver": "4.2.0",
+      "port-version": 0
+    },
     {
       "git-tree": "a36602c632157bbf3260c196a90172c61e8f859b",
       "version-semver": "4.2.0",

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/harfbuzz/vcpkg.json

Valid values for the license field can be found in the documentation

@bold84
Copy link
Contributor Author

bold84 commented Apr 15, 2022

Okay, I see... other ports (aubio, opencv4, pangolin) that depend on harfbuzz fail to build.
I realized now that I actually don't need the harfbuzz update.... But I started already. I hope it's not going to end in dependency hell. Haha..

So, similar to the gn,skia,* ports, I can add them all in one PR, right?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 5d66a98f64188bbd806b8e62e66c097d90eb3224 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 9f5739a..cffe65d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2722,7 +2722,7 @@
     },
     "harfbuzz": {
       "baseline": "4.2.0",
-      "port-version": 3
+      "port-version": 0
     },
     "hayai": {
       "baseline": "2019-08-10",
diff --git a/versions/h-/harfbuzz.json b/versions/h-/harfbuzz.json
index b318da4..0444e35 100644
--- a/versions/h-/harfbuzz.json
+++ b/versions/h-/harfbuzz.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "75697d7c43494eeb0699d372f41853cc937f7a78",
+      "version-semver": "4.2.0",
+      "port-version": 0
+    },
     {
       "git-tree": "a36602c632157bbf3260c196a90172c61e8f859b",
       "version-semver": "4.2.0",

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/harfbuzz/vcpkg.json

Valid values for the license field can be found in the documentation

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/harfbuzz/vcpkg.json

Valid values for the license field can be found in the documentation

@bold84 bold84 marked this pull request as draft April 15, 2022 14:27
@bold84
Copy link
Contributor Author

bold84 commented Apr 15, 2022

Screen Shot 2022-04-15 at 21 58 08

I can not reproduce these errors 😐

@dg0yt
Copy link
Contributor

dg0yt commented Apr 15, 2022

For pangolin:x64-windows-static:

CMake Error at D:/installed/x64-windows-static/share/ffmpeg/FindFFMPEG.cmake:68 (find_library):
  Could not find
  FFMPEG_DEPENDENCY_/d/installed/x64-windows-static/lib/pkgconfig/../../lib/freetype_RELEASE
  using the following names:
  /d/installed/x64-windows-static/lib/pkgconfig/../../lib/freetype
Call Stack (most recent call first):
  D:/installed/x64-windows-static/share/ffmpeg/FindFFMPEG.cmake:148 (append_dependencies)
  D:/installed/x64-windows-static/share/ffmpeg/vcpkg-cmake-wrapper.cmake:6 (_find_package)
  C:/a/1/s/scripts/buildsystems/vcpkg.cmake:770 (include)
  src/CMakeLists.txt:388 (find_package)

This looks like a path (/d/installed/x64-windows-static/lib/pkgconfig/../../lib/freetype) is used where a plain name (e.g. harfbuzz) is expected. Perhaps wrong export in lib/pkgconfig?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/harfbuzz/vcpkg.json

Valid values for the license field can be found in the documentation

@bold84
Copy link
Contributor Author

bold84 commented Apr 17, 2022

For pangolin:x64-windows-static:

CMake Error at D:/installed/x64-windows-static/share/ffmpeg/FindFFMPEG.cmake:68 (find_library):
  Could not find
  FFMPEG_DEPENDENCY_/d/installed/x64-windows-static/lib/pkgconfig/../../lib/freetype_RELEASE
  using the following names:
  /d/installed/x64-windows-static/lib/pkgconfig/../../lib/freetype
Call Stack (most recent call first):
  D:/installed/x64-windows-static/share/ffmpeg/FindFFMPEG.cmake:148 (append_dependencies)
  D:/installed/x64-windows-static/share/ffmpeg/vcpkg-cmake-wrapper.cmake:6 (_find_package)
  C:/a/1/s/scripts/buildsystems/vcpkg.cmake:770 (include)
  src/CMakeLists.txt:388 (find_package)

This looks like a path (/d/installed/x64-windows-static/lib/pkgconfig/../../lib/freetype) is used where a plain name (e.g. harfbuzz) is expected. Perhaps wrong export in lib/pkgconfig?

Screen Shot 2022-04-17 at 23 00 57

Screen Shot 2022-04-17 at 23 30 13

I still can't reproduce the problem. I started off with a virgin vcpkg clone.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 17, 2022

I still can't reproduce the problem. I started off with a virgin vcpkg clone.

You may not build the same feature configuation as CI does, for ports in the dependency chains. ffmpeg burns some wrong/unexpected data into share/ffmpeg. You probably need ./vcpk install pangolin[ffmpeg] ffmpeg[freetype,ass]. Feature ass pulls harfbuzz via libass into ffmpeg. So in this chain, the harfbuzz update could impact fffmpeg output to share/ffmpeg, which blows up when used by pangolin[ffmpeg].

@bold84
Copy link
Contributor Author

bold84 commented Apr 17, 2022

I still can't reproduce the problem. I started off with a virgin vcpkg clone.

You may not build the same feature configuation as CI does, for ports in the dependency chains. ffmpeg burns some wrong/unexpected data into share/ffmpeg. You probably need ./vcpk install pangolin[ffmpeg] ffmpeg[freetype,ass]. Feature ass pulls harfbuzz via libass into ffmpeg. So in this chain, the harfbuzz update could impact fffmpeg output to share/ffmpeg, which blows up when used by pangolin[ffmpeg].

That makes sense...

@bold84
Copy link
Contributor Author

bold84 commented Apr 17, 2022

Nope...

Screen Shot 2022-04-18 at 06 22 15

@bold84
Copy link
Contributor Author

bold84 commented Apr 18, 2022

I still can't reproduce the problem. I started off with a virgin vcpkg clone.

You may not build the same feature configuation as CI does, for ports in the dependency chains. ffmpeg burns some wrong/unexpected data into share/ffmpeg. You probably need ./vcpk install pangolin[ffmpeg] ffmpeg[freetype,ass]. Feature ass pulls harfbuzz via libass into ffmpeg. So in this chain, the harfbuzz update could impact fffmpeg output to share/ffmpeg, which blows up when used by pangolin[ffmpeg].

That makes sense...

Okay... I figured out that I can run

.\vcpkg.exe ci --triplet=x64-windows-static

So, let's see...

@dg0yt
Copy link
Contributor

dg0yt commented Apr 19, 2022

Maybe you lack the overlay ports in scripts/test-ports. They pull in additional features.

@bold84
Copy link
Contributor Author

bold84 commented Apr 19, 2022

Maybe you lack the overlay ports in scripts/test-ports. They pull in additional features.

Thank you. I already started one build a few hours ago. 1871 packages.
I will see if the problem occurs. If the issue doesn't occur, I will add the overlay ports, as you suggested.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 19, 2022

If you restart the CI build, you may also want to add skipped ports, based on ci.baseline.txt and manual additions. In particular, you proabably want to skip tensorflow.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 19, 2022

Other hints to mitigate vcpkg ci build time:

  • use parent hashes. (vcpkg ci --dry-run --export-hashes for a parent commit to get these hashes.) Removes unaffected ports from the build, as in vcpkg PR CI.
  • Let vcpkg's PR do the work, but add a message(FATAL_ERROR STOP) at the end of the ports you need to examine. For example, copy relevant config files to the port's log directory before the stop, so that they are included in the failure logs.

# Conflicts:
#	ports/harfbuzz/portfile.cmake
#	ports/harfbuzz/vcpkg.json
#	versions/baseline.json
#	versions/h-/harfbuzz.json
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for harfbuzz but no changes to version or port version.
-- Version: 4.2.0
-- Old SHA: 1f02c7e1133785d12121cf295009465706cc5dd7
-- New SHA: 62b08c05899c0e8eefcf5880d1e9f8336a284e26
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@bold84
Copy link
Contributor Author

bold84 commented Apr 23, 2022

In was able to reproduce this problem. The .pc file provided by harfbuzz 4.2.0 contains absolute paths to the libraries it depends on.

I assume that's because installation directory (packages) differs from the directory the dependencies are installed to.

The FindFFMPEG.cmake file can't handle paths. It expects the library names.

There are several ways to solve this problem. Currently, I intend to solve it in vcpkg_fixup_pkgconfig.cmake, as I expect this problem to occur with other ports in the future (if the projects switch to meson, which seems to be in fashion).

What do you think?

@dg0yt
Copy link
Contributor

dg0yt commented Apr 23, 2022

Changing vcpkg_fixup_pkgconfig.cmake will trigger the next world rebuild (> 10h, cf. #23898).
And vcpkg_fixup_pkgconfig.cmake already does a lot to avoid absolute paths.
So better make add a minimal local fixup (patch, or explicit replacement) in port harfbuzz now, unless you have evidence of a common pattern.

@bold84
Copy link
Contributor Author

bold84 commented Apr 23, 2022

Thank you for your response, @dg0yt .

I'm aware of that.
I wouldn't mess things up the way I did with vcpkg-tool-gn again. 😂
Now, I have the resources and knowledge to build locally in the same way as the CI does (the parts I need).

But I think you're right. Let's see if it happens again with other ports.
So, I will solve the problem for this particular case in the port files.

@JonLiu1993
Copy link
Member

@bold84 , Thanks for your pr, if this pr is ready for review, please let me know

@bold84 bold84 marked this pull request as ready for review April 25, 2022 07:27
@bold84
Copy link
Contributor Author

bold84 commented Apr 25, 2022

@JonLiu1993 Ready :-)

@JonLiu1993
Copy link
Member

All features are tested successfully in the following triplet:

  • x64-windows
  • x64-windows-static

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Apr 27, 2022
@BillyONeal BillyONeal merged commit 0d71120 into microsoft:master Apr 28, 2022
@BillyONeal
Copy link
Member

Thanks for the update!

@bold84 bold84 deleted the update_harfbuzz branch April 29, 2022 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants