Closed Bug 609114 Opened 9 years ago Closed 9 years ago

use freetype's configure script configure its build rather than hard coding the makefile

Categories

(Firefox Build System :: General, defect)

ARM
Android
defect
Not set

Tracking

(fennec2.0b3+)

RESOLVED FIXED
mozilla2.0b8
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
I'm open to any suggestions for cleaning this up
Attachment #487694 - Flags: review?(ted.mielczarek)
Blocks: 608896
tracking-fennec: --- → 2.0b3+
OS: Linux → Android
Hardware: x86_64 → ARM
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #487694 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #487696 - Flags: review?(ted.mielczarek)
Attachment #487694 - Flags: review?(ted.mielczarek)
Comment on attachment 487696 [details] [diff] [review]
patch

>diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in
>--- a/toolkit/library/Makefile.in
>+++ b/toolkit/library/Makefile.in
>@@ -271,6 +271,11 @@ endif
> 
> include $(topsrcdir)/config/rules.mk
> 
>+ifdef MOZ_TREE_FREETYPE
>+-lfreetype:
>+	make -C ../../modules/freetype2
>+endif
>+
> export:: $(RDF_UTIL_SRC_CPPSRCS) $(INTL_UNICHARUTIL_UTIL_CPPSRCS)
> 	$(INSTALL) $^ .
> 

Why is this necessary?
(In reply to comment #2)
> Comment on attachment 487696 [details] [diff] [review]
> Why is this necessary?

I removed modules/freetype2 from tier_platform_dirs because we no longer need to create the Makefile from (the no deleted) Makefile.in. Unfortunately, this also removes the rule for making libfreetype.

As I said in comment 0, any suggestions for making this cleaner are welcome.
I believe you can add it to tier_platform_staticdirs, and it will do what you want.
Attached patch patch (obsolete) — Splinter Review
Attachment #487696 - Attachment is obsolete: true
Attachment #487885 - Flags: review?(khuey)
Attachment #487696 - Flags: review?(ted.mielczarek)
Whiteboard: [has-patch]
Comment on attachment 487885 [details] [diff] [review]
patch

Not too thrilled about exporting all those variables; bumping this to ted to see if he has a better idea.
Attachment #487885 - Flags: review?(khuey) → review?(ted.mielczarek)
A few concerns here:

1. It feels like a static library is being generated almost coincidentally as a result of a configure test failing due to quirks in our toolchain. Specifying --disable-shared explicitly would be nicer.
2. Freetype's build system uses libtool, which doesn't do -fPIC because shared libs are disabled. Right now, not using -fPIC causes more memory usage due to code that can't be shared between processes.
3. This makes it more difficult to do fakelibs. Fakelibs lets us lay out libxul better (bug 603370) and improve startup time and memory usage (via custom linker tricks).
Comment on attachment 487885 [details] [diff] [review]
patch

>diff --git a/configure.in b/configure.in
>+# Run freetype configure scrip

"script".

>+
>+if test "$MOZ_TREE_FREETYPE"; then
>+   export CFLAGS="$CFLAGS -std=c99"
>+   export CPPFLAGS="$CPPFLAGS"
>+   export CXXFLAGS="$CXXFLAGS"
>+   export LDFLAGS="$LDFLAGS"
>+   export CONFIG_FILES="unix-cc.mk:unix-cc.in unix-def.mk:unix-def.in freetype-config freetype2.pc:freetype2.in"
>+   ac_configure_args="$ac_configure_args --host=$host"

You may want to sanity-check this, since the meaning of --host and --target is different in our configure and every other configure. In our configure --host is the build machine, and --target is the thing you're compiling for, but in every other configure --build is the build machine, --host is the thing you're compiling for, and --target is the platform your binaries will generate code for (if you were building a compiler).

>+   AC_OUTPUT_SUBDIRS(modules/freetype2)
>+fi

I take it freetype's configure script doesn't know how to calculate these values correctly, which is why we have to pass them down from our configure?

Does the freetype configure work with MSVC? If not, please make --enable-tree-freetype fail when using MSVC.

r=me with those fixes.
Attachment #487885 - Flags: review?(ted.mielczarek) → review+
Before you check this in, *please* make sure the thing is building with -fPIC.
Attached patch patch for checkin (obsolete) — Splinter Review
carrying ted's review
Attachment #487885 - Attachment is obsolete: true
Attachment #489919 - Flags: review+
(In reply to comment #10)
> Created attachment 489919 [details] [diff] [review]
> patch for checkin
> 
> carrying ted's review

IIRC CPPFLAGS are for the c preprocessor and CXXFLAGS are for the c++ compiler so the -fPIC in CPPFLAGS isn't necessary, though I guess harmless if it still builds.
found a configure flag for -fPIC, which I like better
Attachment #489919 - Attachment is obsolete: true
pushed http://hg.mozilla.org/mozilla-central/rev/f57bef48f41f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 611845
Flags: in-testsuite-
Whiteboard: [has-patch]
Target Milestone: --- → mozilla2.0b8
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.