Closed Bug 463532 Opened 11 years ago Closed 11 years ago

import freetype into the tree

Categories

(Core :: General, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch build system bits (obsolete) — Splinter Review
Ted, can you make sure I've got the build system bits right?

vlad, can you sr putting the contents of http://download.savannah.gnu.org/releases/freetype/freetype-2.3.7.tar.gz in modules/freetype2?  (its too big to attach)
Attachment #346801 - Flags: superreview?(vladimir)
Attachment #346801 - Flags: review?(ted.mielczarek)
the license was approved in bug 461491
Depends on: 461491
For some reason, I'm not authorized to access that bug. I'm logged in.

/be
Blocks: 462908
Attached patch additional makefile change (obsolete) — Splinter Review
Attachment #346906 - Flags: review?(ted.mielczarek)
As noted in bug 463531, ftconfig.h expects CHAR_BITS to be defined, which it is not on windows ce
Comment on attachment 346801 [details] [diff] [review]
build system bits

+if [ "WIN_FREETYPE" ]; then
+ add_makefiles "
+   $MAKEFILES_freetype2
+ "

Missing the $ in front of the WIN_FREETYPE variable name. Also, the convention seems to be to name vars MOZ_FOO, might make sense to have this be MOZ_FREETYPE, or MOZ_WIN_FREETYPE.

+CSRCS = \
+	src/autofit/afangles.c	\

Instead of listing full paths here, set VPATH = src/autofit src/base etc, then just set CSRCS = afangles.c ftbase.c etc. Using full paths breaks mkdepend last I checked. In addition, it will save you from doing all the "nsinstall -D" down below, as all the object files will wind up in this directory.

+ifeq ($(OS_ARCH), Darwin)
+CSRCS	+=		src/base/ftmac.c
+endif

Does this actually compile on Mac, or is this just future-proofing or what?

+export::
+	nsinstall -D $(DIST)/include/freetype2
+	cp -r $(srcdir)/include/* $(DIST)/include/freetype2

This is wrong. List all the headers in EXPORTS, and they'll automatically get put in dist/include/$MODULE.
Attachment #346801 - Flags: review?(ted.mielczarek) → review-
Attachment #346906 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #6)
> (From update of attachment 346801 [details] [diff] [review])
> +if [ "WIN_FREETYPE" ]; then
> + add_makefiles "
> +   $MAKEFILES_freetype2
> + "
> 
> Missing the $ in front of the WIN_FREETYPE variable name. Also, the convention
> seems to be to name vars MOZ_FOO, might make sense to have this be
> MOZ_FREETYPE, or MOZ_WIN_FREETYPE.

I suspect this may become CAIRO_WIN_FREETYPE, would that work here or should I create a second variable?

> 

> +ifeq ($(OS_ARCH), Darwin)
> +CSRCS    +=        src/base/ftmac.c
> +endif
> 
> Does this actually compile on Mac, or is this just future-proofing or what?
> 

future proofing, should I remove it?

> +export::
> +    nsinstall -D $(DIST)/include/freetype2
> +    cp -r $(srcdir)/include/* $(DIST)/include/freetype2
> 
> This is wrong. List all the headers in EXPORTS, and they'll automatically get
> put in dist/include/$MODULE.

both freetype ad cario expect a certain directly structure, which is why I did the copy.  Is there a way to get $(DIST)/include/freetype2/ft2build.h, $(DIST)/include/freetype2/freetype/freetype.h, $(DIST)/include/freetype2 and /freetype/config/ftconfig.h?  Or should LOCAL_INCLUDES just point to $(topsrcdir)/modules/freetype2/include ?
Could we please get bug 461491 opened to the public?
I don't have permission to do that, but I've asked on the bug for someone who does to do so.  In the mean time, I'll add you to the cc.
(In reply to comment #7)
> I suspect this may become CAIRO_WIN_FREETYPE, would that work here or should I
> create a second variable?

Sounds fine.

> future proofing, should I remove it?

Yeah, it gives the impression that this has been tested on Mac, I'd say leave it out.

> both freetype ad cario expect a certain directly structure, which is why I did
> the copy.  Is there a way to get $(DIST)/include/freetype2/ft2build.h,
> $(DIST)/include/freetype2/freetype/freetype.h, $(DIST)/include/freetype2 and
> /freetype/config/ftconfig.h?  Or should LOCAL_INCLUDES just point to
> $(topsrcdir)/modules/freetype2/include ?

I think you should just use LOCAL_INCLUDES += $(topsrcdir)/... (but maybe bsmedberg will correct me here.)
LOCAL_INCLUDES sounds better/simpler.
Attached patch updated based on ted's comments (obsolete) — Splinter Review
I'll rename WIN_FREETYPE based on the feed back in the other bug.

Also, I'm not exporting any headers since we don't need them.
Attachment #346801 - Attachment is obsolete: true
Attachment #346906 - Attachment is obsolete: true
Attachment #347720 - Flags: superreview?(vladimir)
Attachment #347720 - Flags: review?(ted.mielczarek)
Attachment #346801 - Flags: superreview?(vladimir)
Interesting (and frustrating), that you can easily do this kind of stuff when you are working for Mozilla while as an outside contributor one waits weeks without getting an answer from responsible people and eventually gives up...
(In reply to comment #13)
> Interesting (and frustrating), that you can easily do this kind of stuff when
> you are working for Mozilla while as an outside contributor one waits weeks
> without getting an answer from responsible people and eventually gives up...

What incident are you referring to here?

/be
I wouldn't call it "incident" but I was mildly annoyed by not getting a response for Bug 411578 (which is now marked fixed because the minor part was indeed checked into the tree).
I saw your comment in that bug. I'm sorry I didn't reply right away; I lost track of that bugmail.

Here's something else to consider: OS2 is not a tier 1 platform, while mobile platforms are targeted to be tier 1. This means OS2 might have to carry an external dependency that will be internalized if it becomes necessary for mobile target platforms. We have to prioritize, including effort bringing source from an upstream repository into the tree.

/be
Also, when there are multiple "parts" to a bug, its often good practice to file sub-bugs for each and mark them as dependencies of the "super" bug.  That will help to avoid the whole bug being marked as fixed when the whole problem isn't resolved.
Comment on attachment 347720 [details] [diff] [review]
updated based on ted's comments

+VPATH += @srcdir@/src/autofit 	\

It's a bit unconventional to use @srcdir@ here, could you use $(srcdir) instead? (Should be no functional difference.)
Attachment #347720 - Flags: review?(ted.mielczarek) → review+
vlad, review ping?
this also conditionally adds modules/freetype2 to tier_exteral_dirs as ted suggested on irc
Attachment #347720 - Attachment is obsolete: true
Attachment #351235 - Flags: superreview?(vladimir)
Attachment #351235 - Flags: review+
Attachment #347720 - Flags: superreview?(vladimir)
Attachment #351235 - Flags: superreview?(vladimir) → superreview+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.