776783 - B2G Desktop Builds: Package profile into the build process

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Tony Chung [:tchung]]

We need to pre-build profiles into the Desktop Build process.  It will help make setup so much more easier.

Recommendation, integrate git clone gaia repo, make -C profile, and ./b2g -profile commands into the package

Repro:
1) install 7-23-2012 desktop OS build on Mac
2) verify setting up the builds takes a few extra steps as i outlined above.  It would be great to get this up and running.

Expected:
- Have a simple setup process to extract, and load profile in 1 step.  preferably no command line even

Actual:
- there's involved steps to get desktop builds up and running on win/mac/linux.  it's really developer only right now.

Comment 1

[Dietrich Ayala (:dietrich)]

Not technically blocking Basecamp, but we need this fixed regardless, in order to have proper test environment available.

Comment 2

[Tony Chung [:tchung]]

using whiteboard "b2g-desktop-builds" for ease of tracking

Comment 3

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: package gaia into B2G desktop builds

This patch adds the ability to invoke the gaia build system and collect its results.  It also builds a small program that replaces the actual b2g binary in the App Bundle info.plist file.  This patch has only been tested on Mac, and the run-b2g program needs changes for Windows.

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: updated patch that works on Linux

Updated patch that works on Linux as well as OS X

The wrap ldflags portion is based on bug 781868 and is so we don't depend on mozglue for the simple program.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86980fdd59dc

Comment 6

[Dietrich Ayala (:dietrich)]

What was checked-in and why was this marked fixed? Looks like there are two reviews still pending?

Comment 7

[(not currently active) Ted Mielczarek]

Looks like an unrelated patch. If you land a patch on inbound, and don't mark your bug as "leave open", the inbound merge fairies will auto-close it for you.

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Ted Mielczarek [:ted] from comment #7)
> Looks like an unrelated patch. If you land a patch on inbound, and don't
> mark your bug as "leave open", the inbound merge fairies will auto-close it
> for you.

oh!  thanks.

Comment 9

[Dietrich Ayala (:dietrich)]

Hi Ted & Kyle - do you have an ETA for reviewing this? This is extremely important for getting B2G into the hands of testers, and is blocking that effort.

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: patch for review

Here is an updated patch.  This patch does the following:

1) add --with-gaia= to configure.  This value points to where the gaia source check out is located.  This is stored in the build system as GAIADIR.  When GAIADIR is defined as a non-empty string, it AC_DEFINES(PACKAGE_GAIA)

2) adds b2g/gaia/Makefile.in.  This directory is optionally included in the b2g/Makefile.in DIRS variable if GAIADIR is non-empty.  On Linux and Mac OS X, this makefile will also build run-b2g, which is used as a wrapper to B2G to make launching

3) adds b2g/gaia/run-b2g.c.  This is a posix application that is build on non-windows platforms when packaing Gaia.  It makes it easier to run B2G with a Gaia profile.  I don't have a Windows version yet, but it'll probably use a different source file.

4) Change the Mac B2G App bundle to launch run-b2g instead of plain b2g.  This means that you can double click on the app and immediately start B2G with Gaia.  This removes the document type handlers and document image information from the Info.plist file because run-b2g doesn't know how to deal with them
5) Packages all of Gaia and run-b2g in the output of the packager

Issues:
1) I should either write a Windows run-b2g, or exclude @BINPATH@/run-b2g from the packaging manifest.  I was hoping to land this with packaging enabled on Linux and OS X and writing the Windows wrapper before enabling this for Windows.

2) For obvious reasons, opening B2G.app with a baked in profile on the DMG doesn't work.  The issue here is that the error message doesn't really explain what's actually wrong or how to fix it.  I'd like to add a more useful (and graphical, if appropriate) error message that explains what to do.

3) Currently, run-b2g doesn't interpret any of it's extra command line arguments.  I'd like to make it so that run-b2g does this (in psuedo-python):

exec([argv[0], '-profile', profile_path] + argv[1:])


Questions:
1) Firefox has a firefox shell script wrapper and firefox-bin binary.  Do you think it'd be a good idea to follow a similar pattern with the wrapper being called "b2g" and the binary being called "b2g-bin" for desktop B2G builds?

2) I plan to rewrite run-b2g in native windows C/C++, would using raw CRT functions be OK or should I use Win32?

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to John Ford [:jhford] from comment #10)
> 1) Firefox has a firefox shell script wrapper and firefox-bin binary.  Do
> you think it'd be a good idea to follow a similar pattern with the wrapper
> being called "b2g" and the binary being called "b2g-bin" for desktop B2G
> builds?


This hasn't been true for quite a while. 'firefox' and 'firefox-bin' are the same binary now. It loads the rest of Gecko using the standalone glue.

Comment 12

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Ted Mielczarek [:ted] from comment #11)
> This hasn't been true for quite a while. 'firefox' and 'firefox-bin' are the
> same binary now. It loads the rest of Gecko using the standalone glue.

I didn't know, that's really cool!

Comment 13

[Dietrich Ayala (:dietrich)]

These patches are awaiting review for almost 5 days. Is a new patch needed, or are the review requests still validly open?

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

Unless there are review comments, I don't have any changes planned.

Comment 15

[(not currently active) Ted Mielczarek]

Sorry, it wasn't clear that the work here was actually finished.

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: windows wrapper program

Just throwing this up so I have a copy of it somewhere.  This is not intended to be a part of the initial review.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 654902 [details] [diff] [review]
patch for review

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

r- for a few things. I wouldn't hold this patch up for the suggestion about folding this into nsBrowserApp, but I think that's ripe for a followup (and perhaps some forward-thinking about the binary names here so that we can make that swap in the future without breaking everything).

::: b2g/app/Makefile.in
@@ +103,4 @@
>  libs repackage:: $(libs-preqs)
>  	rsync -a --exclude "*.in" $(srcdir)/macbuild/Contents $(DIST)/$(APP_NAME).app --exclude English.lproj
>  	rsync -a --exclude "*.in" $(srcdir)/macbuild/Contents/Resources/English.lproj/ $(DIST)/$(APP_NAME).app/Contents/Resources/$(AB).lproj
> +	sed -e "s/%MOZ_APP_VERSION%/$(MOZ_APP_VERSION)/" -e "s/%MOZ_APP_NAME%/$(MOZ_APP_NAME)/" -e "s/%APP_VERSION%/$(APP_VERSION)/" -e "s/%APP_NAME%/$(APP_NAME)/" -e "s/%APP_BINARY%/$(APP_BINARY)/" -e "s/%EXECUTABLE_NAME%/run-b2g/" $(srcdir)/macbuild/Contents/Info.plist.in > $(DIST)/$(APP_NAME).app/Contents/Info.plist

If you're just going to hardcode run-b2g you might as well just hardcode it in Info.plist.

::: b2g/gaia/Makefile.in
@@ +33,5 @@
> +	$(MAKE) profile GAIA_DOMAIN=desktop-builds.$(MOZ_APP_NAME).mozilla.org && \
> +	mkdir -p $(ABS_DIST)/bin/$(GAIA_PATH) && \
> +	(cd $(GAIADIR)/profile && tar $(TAR_CREATE_FLAGS) - .) | \
> +	(cd $(ABS_DIST)/bin/$(GAIA_PATH) && tar -xf -) \
> +	$(NULL)

This kind of sucks. I don't really have a great suggestion for how to do this in a cleaner fashion though. I can make one suggestion:
Get rid of the continuations, just run one command per line. (make will stop if it errors on a line anyway). Use $(MAKE) -C $(GAIADIR) instead of having the initial cd.
Also, you can use GENERATED_DIRS for that dist/bin directory instead of having an explicit mkdir there.

The make {clean,profile} step there kind of sucks. It means we're going to be doing a lot of unnecessary shuffling every time. Is that just for sanity?

::: b2g/gaia/run-b2g.c
@@ +42,5 @@
> +    error("unable to start");
> +    perror(argv[0]);
> +    free(full_path);
> +    free(full_profile_path);
> +    return -1;

I don't know why you wrote this in C and fiddled with all the memory management instead of writing it in C++ and using the STL, but it's already written so I'm not going to really worry about it.

It seems like this is something we could just fold into b2g/app/nsBrowserApp.cpp with some conditionals. It would be easy enough to just add some defines when we're building for a desktop platform, and make the app look for a hardcoded profile name.

::: configure.in
@@ +156,5 @@
> +MOZ_ARG_WITH_STRING(gaia,
> +[  --with-gaia=DIR
> +               location of gaia dir],
> +    GAIADIR=$withval)
> +AC_SUBST(GAIADIR)

I'm not wild about adding a new configure option for this one specific application and build configuration. Can you drop the ARG_WITH_STRING and just keep the AC_SUBST/AC_DEFINE? That way you can just put:
GAIADIR=whatever
in the mozconfigs, but it won't show up in `configure --help`.

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Ted Mielczarek [:ted] from comment #17)
> Comment on attachment 654902 [details] [diff] [review]
> patch for review
> If you're just going to hardcode run-b2g you might as well just hardcode it
> in Info.plist.

I was trying to have as much of the information in a single source, but this seems reasonable.

> ::: b2g/gaia/Makefile.in
> This kind of sucks. I don't really have a great suggestion for how to do
> this in a cleaner fashion though. I can make one suggestion:
> Get rid of the continuations, just run one command per line. (make will stop
> if it errors on a line anyway). Use $(MAKE) -C $(GAIADIR) instead of having
> the initial cd.

That's a good idea.  That should also make figuring out the absolute path to the dist dir unnecessary.

> Also, you can use GENERATED_DIRS for that dist/bin directory instead of
> having an explicit mkdir there.

I didn't know that existed.  I'll look into using it.

> The make {clean,profile} step there kind of sucks. It means we're going to
> be doing a lot of unnecessary shuffling every time. Is that just for sanity?

Yes, mainly for sanity for two reasons:

1) There is a xulrunner sdk that gets fetched to build Gaia.  When I wrote this patch, Gaia wouldn't grab a new Xulrunner SDK if the one specified in the makefile changed.  This would've required manual interaction.  I've patched Gaia's build system to be intelligent about when it needs to download a new xulrunner, which removes this part of the concern.

2) Gaia currently only knows how to build with products inside of the gaia source tree.  This means that once a build is done, the original tree will have build products in it.  The call to make clean is needed if we want to start from a valid state.  Since the changes to the Xulrunner SDK above, we only really need to delete the profile that is built in the gaia tree.

> ::: b2g/gaia/run-b2g.c
> I don't know why you wrote this in C and fiddled with all the memory
> management instead of writing it in C++ and using the STL, but it's already
> written so I'm not going to really worry about it.

I'm not familiar with the STL, but know how to do memory management in C for a simple program like this.

> It seems like this is something we could just fold into
> b2g/app/nsBrowserApp.cpp with some conditionals. It would be easy enough to
> just add some defines when we're building for a desktop platform, and make
> the app look for a hardcoded profile name.

I tried baking this into the XRE stuff and that just failed spectacularly.  I'll take a look at nsBrowserApp.cpp, but as suggested in a follow up.

Further to your point about future binary naming issues, I think if I were to make the b2g binary on desktop be called "b2g-bin", and the wrapper named "b2g", it would allow the same scripts/docs to work with the current wrapper and when we roll the functionality into the actual b2g binary, we could change "b2g-bin" back to just "b2g".

> ::: configure.in
> @@ +156,5 @@
> > +MOZ_ARG_WITH_STRING(gaia,
> > +[  --with-gaia=DIR
> > +               location of gaia dir],
> > +    GAIADIR=$withval)
> > +AC_SUBST(GAIADIR)
> 
> I'm not wild about adding a new configure option for this one specific
> application and build configuration. Can you drop the ARG_WITH_STRING and
> just keep the AC_SUBST/AC_DEFINE? That way you can just put:
> GAIADIR=whatever
> in the mozconfigs, but it won't show up in `configure --help`.

Sure can!

Comment 19

[(not currently active) Ted Mielczarek]

Your replies all sound fine, so if you get a patch up with those things addressed I can r+ it.

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: updated patch

I've made the changes discussed.

Comment 21

[Alex Keybl [:akeybl]]

Please try to target this fix for before the end of the week - desktop builds will be a session at MozCamp, and the installation instructions would benefit from having this fixed.

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8c83c4ea731a

Please leave this bug open

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c83c4ea731a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8253d8caa1b8
https://hg.mozilla.org/mozilla-central/rev/611507d8da4c
https://hg.mozilla.org/mozilla-central/rev/0afb00f99006
https://hg.mozilla.org/mozilla-central/rev/257489870be6

Comment 25

[Dietrich Ayala (:dietrich)]

What's left to do here?

Comment 26

[John Ford [:jhford] CET/CEST Berlin Time]

I thought this bug was already blocked by bug 789614 already, but it wasn't.

We are blocked on having gaia sources available on the builders.

The only mozilla-central code issue left is testing the win32 wrapper and its integration with the build system but win32 profile packaging and builds should work now.

Comment 27

[Tony Chung [:tchung]]

There's a awful log of work being done here where blocking-basecamp flag is minus'd.   can we re-evaluate and + it again?

Comment 28

[Dietrich Ayala (:dietrich)]

It's a parallel track of work. It does not block the release of V1 in the target market. However, it's project that is important for many other reasons.

Just because something got minused doesn't mean it's work that doesn't need to be done by people who have multiple projects of which Basecamp is one.

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: Add windows support

This patch changes the makefile to understand the differences between the wrapper program on posix machines and on win32 machines.  There is a Win32 version of the wrapper program added.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 665657 [details] [diff] [review]
Add windows support

>diff --git a/b2g/gaia/Makefile.in b/b2g/gaia/Makefile.in

>+ifeq ($(OS_ARCH),WINNT)
>+CPPSRCS = run-b2g.cpp
>+DEFINES += \
>+  -DB2G_NAME=L\"$(MOZ_APP_NAME)-bin$(BIN_SUFFIX)\" \
>+  -DGAIA_PATH=L\"$(subst /,\\\\,$(GAIA_PATH))\" \
>+  $(NULL)
>+GAIA_MAKE=make

Are you sure this is right?  Why don't we use $(MAKE)?

>diff --git a/b2g/gaia/run-b2g.cpp b/b2g/gaia/run-b2g.cpp

>+#define BUFSIZE sizeof(wchar_t) * MAX_PATH + 1

sizeof(wchar_t) * (MAX_PATH + 1)

Let's call this NUM_MAX_PATH_BYTES.

>+
>+// Options that can be overridden on the command line with -D

s/on the command line/at build time/

>+wchar_t* replace_file(wchar_t* orig, wchar_t* file){

Let's call this |make_path_with_leaf_file_name()|.  It needs to be
documented, and note the caller has to free the returned string.

>+int wmain(int argc, wchar_t *argv[], wchar_t *envp[]){

>+    wchar_t* args = (wchar_t*) malloc(BUFSIZE);

You have two file paths here, and some extra characters, so this needs
to be at least 2 * BUFSIZE + (extra).

>+    free(profile_path);
>+    profile_path = NULL;

Let's free both strings in the same place below to make the mental
model here simpler.

I'm not a win32 API ninja but your usage looks fine.

r=me with comments addressed (answer to question above, and requested
changes).

Comment 31

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #30)
> >+GAIA_MAKE=make
> 
> Are you sure this is right?  Why don't we use $(MAKE)?

Yep, windows uses pymake by default now, which cannot run the Gaia makefile properly.  We want to use the correct value for $(MAKE) everywhere else though, since we use GNU Make elsewhere.

> Let's call this NUM_MAX_PATH_BYTES.

Changed

> s/on the command line/at build time/

fixed

> Let's call this |make_path_with_leaf_file_name()|.  It needs to be
> documented, and note the caller has to free the returned string.

Renamed, documented and noted

> >+int wmain(int argc, wchar_t *argv[], wchar_t *envp[]){
> 
> >+    wchar_t* args = (wchar_t*) malloc(BUFSIZE);
> 
> You have two file paths here, and some extra characters, so this needs
> to be at least 2 * BUFSIZE + (extra).

You're right.  Fixed by making the extra chars a macro and using wcslen when allocating memory
 
> >+    free(profile_path);
> >+    profile_path = NULL;
> 
> Let's free both strings in the same place below to make the mental
> model here simpler.

Done.
 
> I'm not a win32 API ninja but your usage looks fine.

Great!

> r=me with comments addressed (answer to question above, and requested
> changes).

Comment 32

[John Ford [:jhford] CET/CEST Berlin Time]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c1eccb338932

This should be the last landind for this bug!

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c1eccb338932