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 2.3.0 #3398

Closed
wants to merge 1 commit into from
Closed

Conversation

dgilman
Copy link
Contributor

@dgilman dgilman commented Jan 11, 2019

Description

I know the version number is scary, but I don't think everything needs to be revbumped. Note that when Debian went from 1.9.0 in https://tracker.debian.org/news/989152/accepted-harfbuzz-190-1-source-into-unstable/ to 2.1.1 in https://tracker.debian.org/news/1002774/accepted-harfbuzz-211-1-source-into-unstable/ the SONAME did not change. Digging into the source, the upstream harfbuzz author has merely wrapped depreciated functions in a compile-time depreciation warning and not actually removed anything yet, so it is safe to upgrade without revbumps. And for what it's worth, macports didn't see any need to recompile pango on my machine.

Testing: I did port -d test harfbuzz and that works.

I also exercised harfbuzz and pango/cairo using graphviz. Here's my example dotfile, with arabic generated from a lorem ipsum arabic text generator I found on the internet:

digraph example {
   A [label="yolo"];
   B [label="swag"];
   C [label="غريمه الأول العالمي عن تلك, تحرّكت والروسية لم الى. دار لم والمانيا المتساقطة،. يكن ثم هُزم وترك لإعادة, جُل رئيس ماليزيا، عل. حادثة بتخصيص ليرتفع يكن قد, في التكاليف العمليات الى. كل على الثانية والروسية, انتهت الجنود نفس مع, تلك سقوط وبريطانيا كل. فقد في أوزار مكثّفة البشريةً."];

   A -> B
   B -> C
   C -> A
}

And here's how I invoked graphviz, forcing PDF output as this appears to require pango/cairo/harfbuzz:

DYLD_PRINT_LIBRARIES=yes dot -Tpdf -O ./arabic.dot

I verified that harfbuzz was being loaded by the linker and the PDF's text looked nice to me.

Type(s)
  • enhancement
Tested on

macOS 10.13.6 17G4015
Xcode 10.1 10B61

Verification

Have you

  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

@macportsbot
Copy link

Notifying maintainers:
@ryandesign for port harfbuzz.

@mojca
Copy link
Member

mojca commented Jan 11, 2019

You said you referenced trac tickets, but I fail to see https://trac.macports.org/ticket/57840.

@macportsbot
Copy link

Travis Build #4859 Passed.

Lint results
--->  Verifying Portfile for harfbuzz
--->  0 errors and 0 warnings found.

Port harfbuzz success on xcode9.4. Log
Port harfbuzz-icu success on xcode9.4. Log
Port harfbuzz success on xcode8.3. Log
Port harfbuzz-icu success on xcode8.3. Log
Port harfbuzz success on xcode7.3. Log
Port harfbuzz-icu success on xcode7.3. Log
Port harfbuzz success on xcode10.1. Log

@dgilman
Copy link
Contributor Author

dgilman commented Jan 11, 2019

I think you are confusing me with l2dy? I didn't know about trac

@mf2k
Copy link
Contributor

mf2k commented Jan 11, 2019

@dgilman: You checked the box that you did. It doesn't matter that you didn't open the ticket.

You need to add (something like) the following to the end of your commit message on a new line.

Closes: https://trac.macports.org/ticket/57840

@mojca
Copy link
Member

mojca commented Jan 11, 2019

No, I'm not confusing you with l2dy.

The checklist contains an item:

[ ] referenced existing tickets on Trac with full URL?

which is used to remind you to check whether there's a relevant existing ticket about this port when opening a PR. There are various reasons why we put that checkbox there. Very often a change in a port might fix an existing bug from our tracker, in which case we would want to add Closes: https://trac.macports.org/ticket/57840 and then the ticket would be closed automatically. Sometimes some incomplete or not-yet-committed patches may already exist on Trac, in which case you would want to compare them. In this case there was a reason why the port wasn't updated before (I believe that wine would break, for example).

It is not absolutely necessary to do all the steps listed, but it's helpful to know what steps the contributor did or did not do. It is highly advisable to always check for open tickets on Trac, but in case you don't do it, at least don't mark the checkbox.

I hope that makes it slightly more clear?

Copy link
Contributor

@ryandesign ryandesign left a comment

Choose a reason for hiding this comment

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

Please do not merge this as it does not contain a solution for the issues mentioned in https://trac.macports.org/ticket/57840.

@dgilman
Copy link
Contributor Author

dgilman commented Jan 14, 2019

I apologize for checking the box there - I was clearly in the wrong, and didn't do the trac search. I hope you can also see that I was opening this PR in good faith, doing what integration tests I could on my machine to validate the changes in a maintaner reproducible way.

@pmetzger
Copy link
Member

What do we do next to move this towards being merged?

@dgilman
Copy link
Contributor Author

dgilman commented Jan 14, 2019

Either macports or upstream needs to patch harfbuzz to work around bugs in Apple's header file. FWIW, the author of kitty did some research in kovidgoyal/kitty#1187 and has a suggested patch. But I totally agree with Ryan's comments elsewhere that this is a very hairy issue and I totally understand if he wants to wait for upstream to make a decision.

@ryandesign
Copy link
Contributor

Upstream has clearly already made the decision that the CoreText backend is no longer of interest to them. They've closed all my bug reports about it without fixing them. And yet programs like Kitty are using it. So what we're waiting on is for me (or anyone else with interest) to find the time to figure out what changed between harfbuzz 1 and 2 to cause this problem and to fix it, or else to patch kitty to no longer use the HarfBuzz CoreText backend; the HarfBuzz developers have said that it should be a simple matter of a few lines of code which one might be able to copy out of the HarfBuzz CoreText backend into the Kitty code, but I have not attempted to identify those lines yet.

@pmetzger
Copy link
Member

I'll leave this to you guys to settle, then.

@dgilman dgilman deleted the harfbuzz branch November 20, 2020 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6 participants