Last Comment Bug 748467 - mobile/android/installer: dependency build
: mobile/android/installer: dependency build
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: 748452
  Show dependency treegraph
 
Reported: 2012-04-24 11:20 PDT by Joey Armstrong [:joey]
Modified: 2012-05-16 03:33 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.65 KB, patch)
2012-05-15 12:02 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
ted: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-04-24 11:20:54 PDT
dir: mobile/android/installer

Multiple attempts at a dependency build in this directory will generate ~200k of log output.  Track down where time is being spent and add make dependencies so work will only be done when needed.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-14 10:29:01 PDT
This bug basically boils down to "make package" is slow.  There aren't any obvious wins here, and I don't see anything we can achieve in the timeframe we're looking for.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-15 12:02:13 PDT
Created attachment 624135 [details] [diff] [review]
Patch
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-05-15 12:39:14 PDT
Comment on attachment 624135 [details] [diff] [review]
Patch

Review of attachment 624135 [details] [diff] [review]:
-----------------------------------------------------------------

This is kind of ugly but the concept is sound.

::: mobile/android/installer/Makefile.in
@@ +95,5 @@
>  endif
>  DEFINES += -DBINPATH=$(BINPATH)
>  
>  ifdef MOZ_PKG_MANIFEST_P
> +$(MOZ_PKG_MANIFEST): $(MOZ_PKG_MANIFEST_P) $(GLOBAL_DEPS)

Did we ever find out if this was needed for multi-locale builds? If so, you might want to stick $(if $(IS_LANGUAGE_REPACK),FORCE) in the deps here.

::: toolkit/mozapps/installer/packager-deps.py
@@ +1,4 @@
> +import os, sys
> +
> +filename = sys.argv[1]
> +with open(filename, 'r') as f:

You could import fileinput and use for line in fileinput.input(). (Handy module that reads stdin/whatever you pass on the cmdline.)

@@ +2,5 @@
> +
> +filename = sys.argv[1]
> +with open(filename, 'r') as f:
> +    for l in f.readlines():
> +        l.strip()

l = l.strip() (strip doesn't modify the original)

@@ +3,5 @@
> +filename = sys.argv[1]
> +with open(filename, 'r') as f:
> +    for l in f.readlines():
> +        l.strip()
> +        if l[:4] != "bin/":

if not l.startswith("bin/"):

This will work for mobile, but it kind of makes me sad. It won't work on OS X, because @BINPATH@ gets turned into the bundle name there, so maybe you should ifdef out this stuff if someone happens to use it where $(OS_TARGET) is Darwin?

::: toolkit/mozapps/installer/packager.mk
@@ +887,5 @@
>  	@echo "Compressing..."
>  	cd $(DIST) && $(MAKE_PACKAGE)
>  
> +ifdef MOZ_FAST_PACKAGE
> +MAKE_PACKAGE_DEPS = $(wildcard $(subst * , ,$(addprefix $(DIST)/bin/,$(shell $(PYTHON) $(topsrcdir)/toolkit/mozapps/installer/packager-deps.py $(MOZ_PKG_MANIFEST)))))

Why are you substing out the *? $(wildcard) will expand it for you.

@@ +894,5 @@
> +endif
> +
> +make-package: $(MAKE_PACKAGE_DEPS)
> +	$(MAKE) make-package-internal
> +	@touch $@

$(TOUCH)
Comment 4 Ed Morley [:emorley] 2012-05-16 03:33:54 PDT
https://hg.mozilla.org/mozilla-central/rev/5b580c92e98a

Note You need to log in before you can comment on or make changes to this bug.