Skip to content

Conversation

ebraminio
Copy link
Collaborator

@ebraminio ebraminio commented Apr 12, 2017

Is this logic sounds right to you or it should be better be like "if (HB_HAVE_GLIB OR HB_HAVE_ICU)" and maybe HB_HAVE_CORETEXT and other OS provided APIs also?

(With having this in mind that the goal of this cmake is I think to be a simple but correct (thats why I removed -DHB_NO_UNICODE_FUNCS) and easily embeddable harfbuzz distribution that maybe replaces third party distributions like these, and our internal nmake win32 port also entirely someday, if @fanc999 agrees also, other than my personal goal that is making harfbuzz development itself easier with OS provided Xcode or VS tools and extra APIs/SDKs.)

@ebraminio ebraminio requested a review from behdad April 12, 2017 09:45
@ebraminio
Copy link
Collaborator Author

ebraminio commented Apr 12, 2017

Also any other comment on the naming and flags would be very welcomed, especially before harfbuzz release so we can fix them before people rely on them.

@ebraminio ebraminio changed the title [cmake] Enable MT when builtin UCDN is disabled [cmake] Restrict NO_MT only when builtin UCDN is enabled Apr 12, 2017
@ebraminio ebraminio changed the title [cmake] Restrict NO_MT only when builtin UCDN is enabled [cmake] Have NO_MT only when builtin UCDN is enabled Apr 12, 2017
@ebraminio
Copy link
Collaborator Author

ebraminio commented Apr 12, 2017

(I rethought of this flag just when I saw this (and invited the maintainer to here) that tries hardly to port some of atomic operations to bsd/solaris derivatives that can be avoided if only thing one need is having a simple working harfbuzz without having high obsessions on binary size only some specific projects have)

@behdad
Copy link
Member

behdad commented Apr 12, 2017

Humm? Why?

Please don't make it easy to build with NO_MT. Definitely don't make it default in any situation. Those kinds of situations are why I was afraid of adding an alternate build system.

@behdad
Copy link
Member

behdad commented Apr 12, 2017

IMO don't add an option for NO_MT. It exists in the code for the likes of Firefox and Android who only use HB from one thread.

@ebraminio ebraminio changed the title [cmake] Have NO_MT only when builtin UCDN is enabled [cmake] Remove NO_MT flag Apr 12, 2017
@ebraminio
Copy link
Collaborator Author

No problem, removed.

Those kinds of situations are why I was afraid of adding an alternate build system.

Better to let third parties doing that I believe when can be done somewhere right, so quick question, do you see anything here worth to be merged back into harfbuzz itself? I don't know how they work and why they are there honestly.

@behdad
Copy link
Member

behdad commented Apr 12, 2017

do you see anything here worth to be merged back into harfbuzz itself? I don't know how they work and why they are there honestly.

They are ported from HB's configure.ac. They are feature-detection code to detect what atomic ops the compiler supports...

@behdad
Copy link
Member

behdad commented Apr 12, 2017

I've got to say. I find the cmake as unreadable as configure.ac :). Guess it's more portable though.

@ebraminio
Copy link
Collaborator Author

ebraminio commented Apr 12, 2017

Now without NO_MT I faced #345, hope to be fixed soon.

They are feature-detection code to detect what atomic ops the compiler supports...

So I have to study them.

I find the cmake as unreadable as configure.ac :).

Agreed :) Though now I am accustomed to it even the fact I learned and use it just time to time for harfbuzz.

@ebraminio
Copy link
Collaborator Author

(And just to make directwrite backend happen for kashida thing)

@ebraminio ebraminio merged commit 8568588 into harfbuzz:master Apr 12, 2017
fanc999 pushed a commit to fanc999/harfbuzz that referenced this pull request Jun 16, 2017
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.

2 participants