Closed Bug 544540 Opened 14 years ago Closed 14 years ago

the cross platform build scripts should be updated to build android based on platform/android/Makefile

Categories

(Tamarin Graveyard :: Build Config, defect, P1)

Product:
Tamarin Graveyard
Component:
Build Config
Platform:
ARM
All
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Q3 11 - Serrano

People

(Reporter: dschaffe, Assigned: jsudduth)

References

Details

Attachments

(2 files, 7 obsolete files)

Android test failures list.
6.81 KB, text/plain
Details
Removed features list from android compile_generic calls in tamarinredux.py.
31.72 KB, patch
Details | Diff | Splinter Review 
The cross platform build scripts should be updated to include android.  Currently android builds ok using files in platform/android.

In the platform/android directories see the file Makefile, make.avm, make.mmgc, make.zlib
Assignee: nobody → dschaffe
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+
Target Milestone: --- → flash10.2
actually, it doesn't; see https://bugzilla.mozilla.org/show_bug.cgi?id=544973

(but yeah, we should switch to the xplat in any event)
Raising the priority on this; its a continuous pain point.
Priority: -- → P1
Assignee: dschaffe → cpeyer
bump
Assigning to Jim.  See Dan Sc. for clarification.
Assignee: cpeyer → jsudduth
I believe that the jist of this bug is to:

1) pare down the current android Makefile to just contain what is necessary to build the shell, I believe that it currently contains additional things that are not necessary for building the shell

2) update the configure script to generate a Makefile that contains what is needed to compile the android shell
How's this coming?
OS: Mac OS X → Android
Hardware: x86 → ARM
It's coming slowly. I'm able to cross-compile a shell but it's smaller than the one we currently build (1.6 Mb vs. 2 Mb for the Release target); I'm investigating why there's a size difference.
I've acquired a Nexus One finally and have been able to push the release android shell to it and run the acceptance tests using several different command line params, including the -Ojit and -Dinterp flags. They all pass. When I do 'adb shell /data/app/avmshell -Dversion' I get:

AVMSYSTEM_32BIT;AVMSYSTEM_UNALIGNED_INT_ACCESS;AVMSYSTEM_LITTLE_ENDIAN;
AVMSYSTEM_ARM;AVMSYSTEM_UNIX;AVMFEATURE_JIT;AVMFEATURE_ABC_INTERP;
AVMFEATURE_SELFTEST;AVMFEATURE_EVAL;AVMFEATURE_PROTECT_JITMEM;
AVMFEATURE_SHARED_GCHEAP;AVMFEATURE_STATIC_FUNCTION_PTRS;
AVMFEATURE_INDIRECT_NATIVE_THUNKS;AVMFEATURE_CACHE_GQCN;

The only difference between this and what I get running the same command on the buildbot release android shell is 'AVMSYSTEM_UNALIGNED_INT_ACCESS'. I think, from Googling around, that I'm getting that because I'm building for ARMv7 in this test shell; if anyone knows to the contrary please speak up. I doubt this would cause a size difference of almost 25% in the executables but I don't know for sure. I'll try a build for ARMv5 and see if the feature goes away and the size gets closer to 2Mb. Any other ideas on the size difference?
Update: building for ARMv5 gets rid of 'AVMSYSTEM_UNALIGNED_INT_ACCESS' but the executable is still 1.6Mb. The tests continue to pass with the new build.
We should be building for ARMv7 as the default and perhaps ARMv5 as well if we want to test soft FP support on this platform.
Note that ARMv5 support might matter for Android emulator builds, at least according to this page: 
  https://zerowing.corp.adobe.com/display/FlashPlayer/Android+-+For+Dummies

(or maybe that information is out of date)
This patch includes the changes necessary to create an android Makefile using the cross-compile system (configure.py and friends). An example command is:
../configure.py --enable-shell --target=android-linux --arm-arch=armv7-a

The changes necessary to wire this up in the build system (tamarin-redux.py and the shell scripts) aren't included in this patch.

Known issue:
- When building for armv5 there is a link warnings about enum sizes:
/Volumes/android/device/prebuilt/darwin-x86/toolchain/arm-eabi-4.4.0/bin/../lib/gcc/arm-eabi/4.4.0/../../../../arm-eabi/bin/ld: warning: /Volumes/android/device/prebuilt/darwin-x86/toolchain/arm-eabi-4.4.0/bin/../lib/gcc/arm-eabi/4.4.0/libgcc.a(_ctzsi2.o) uses 32-bit enums yet the output is to use variable-size enums; use of enum values across objects may fail
Attachment #486691 - Flags: review?(edwsmith)
Attachment #486691 - Flags: review?(brbaker)
Attachment #486691 - Flags: review?(edwsmith) → review?(fklockii)
Comment on attachment 486691 [details] [diff] [review]
Preliminary patch to enable building android with the cross-compile system.

I would like to see this patch reworked so that android is actually the OS type and not the CPU type. I feel that the current format of the patch will cause pain in the future when/if we need to start compiling android for x86. The oddness of declaring android as the CPU type can be seen by the fact that the CPU type is reset to the correct value of "ARM" in build/configuration.py

I did not do a complete check of this patch but will once a new patch is available. However I did note a couple of things that can possibly be addressed:

- Should the build default to armv7-a if no arm-arch switch is provided? I think we are starting to come to consensus that armv7-a is what we should be targeting. (see comment #10) 

- Comments around setting YS_LIB and FLASHRUNTIME_CODE_ROOT should be addressed. I assume that this does not need to be set for compiling the shell and should be removed.

- Is the *OPENGL* code necessary. There is nothing in the shell that interacts with the GPU. Can it be removed?

- In build/configuration.py is there any reason not to pull vars from the environ if they are set? (For example using os.environ.get('CPPFLAGS', ''))
Attachment #486691 - Flags: review?(brbaker) → review-
Comment on attachment 486691 [details] [diff] [review]
Preliminary patch to enable building android with the cross-compile system.

Also can you double check how the patch was created. For some reason the patch for build/configuration.py is missing an "@@section" for the block of code starting with:

 
         elif self._target[0] == 'linux':
-            self._acvars.update({

For some reason patch did not notice this and did not complain. It took me a couple of seconds to figure why I did not see that additional code change after I applied the patch.
Comment on attachment 486691 [details] [diff] [review]
Preliminary patch to enable building android with the cross-compile system.

I have not reviewed the patch carefully; I've read enough to agree with Brent's comments, but not enough to add more feedback.  So I am clearing the "R?" to remove myself from the review queue, but I would like to be part of the re-review post reworking.

I hypothesize (but have not checked) that some of the artifacts, like the OPENGL stuff, are adaptations of the old android build infrastructure in platform/android/ that this is supposed to replace.  Brent's idea of removing things like the OPENGL aspects from the shell builds makes sense to me; but I think we should first double-check how/why those bits got introduced in the first place, if possible, to guard against throwing away useful information.
Attachment #486691 - Flags: review?(fklockii)
(In reply to comment #13)
> Comment on attachment 486691 [details] [diff] [review]
> Preliminary patch to enable building android with the cross-compile system.
> 
> I would like to see this patch reworked so that android is actually the OS type
> and not the CPU type. I feel that the current format of the patch will cause
> pain in the future when/if we need to start compiling android for x86. The
> oddness of declaring android as the CPU type can be seen by the fact that the
> CPU type is reset to the correct value of "ARM" in build/configuration.py
> 

I changed this several times while working on the patch. The difficulty comes with how the --target parameter is parsed in getopts.py and configuration.py and later picked up in configure.py; you end up having to 'adjust' parameters later either way, but some further discussion of this is warranted to see what we want to live with. This all goes back to the need to redesign how the x-compile files are structured, a non-trivial project IMO.

> I did not do a complete check of this patch but will once a new patch is
> available. However I did note a couple of things that can possibly be
> addressed:
> 
> - Should the build default to armv7-a if no arm-arch switch is provided? I
> think we are starting to come to consensus that armv7-a is what we should be
> targeting. (see comment #10) 
> 

Yes, we should probably go to armv7-a.

> - Comments around setting YS_LIB and FLASHRUNTIME_CODE_ROOT should be
> addressed. I assume that this does not need to be set for compiling the shell
> and should be removed.
> 

I found very little to no info on libys through Googling so just pulled the code over as it was in the original make file to get other eyes on it. After talking with a Flash developer here in S.F. who knows the android make files I think we can remove the YS_LIB code.

> - Is the *OPENGL* code necessary. There is nothing in the shell that interacts
> with the GPU. Can it be removed?
> 

I think it's probably safe to remove. Again, it was brought over to this patch like the YS_LIB stuff to elicit comments.

> - In build/configuration.py is there any reason not to pull vars from the
> environ if they are set? (For example using os.environ.get('CPPFLAGS', ''))

It's a bit orthogonal to the bug but would probably be a good idea. We started doing that recently in configure.py.
(In reply to comment #14)
> Comment on attachment 486691 [details] [diff] [review]
> Preliminary patch to enable building android with the cross-compile system.
> 
> Also can you double check how the patch was created. For some reason the patch
> for build/configuration.py is missing an "@@section" for the block of code
> starting with:
> 
> 
>          elif self._target[0] == 'linux':
> -            self._acvars.update({
> 
> For some reason patch did not notice this and did not complain. It took me a
> couple of seconds to figure why I did not see that additional code change after
> I applied the patch.

My bad - will be fixed in the new patch.
This patch addresses these issues:

1. The OpenGL and YS (Yellowstone) parameters have been removed since they're no longer needed (they were being set but not used by the original make file). I also found and removed INCLUDE_ARM_ASM_FILES and ANDR_OUT_SYS_LIB_DIR because they were also set but not used by the original make file. The path that ANDR_OUT_SYS_LIB_DIR pointed to is already set in LFLAGS_HEADLESS.

2. The handling of the --target switch has been changed to avoid having to reset the TARGET_CPU parameter to "arm" as before. The TARGET_OS, however, has to remain "linux" in order for the build to succeed. If it's necessary to change this to "android" then there will be some work to do in avmfeatures.

3. Added -Wl,--no-enum-size-warning to get rid of enum warnings with the armv5 and armv6 builds; the original make file used this switch also.

4. The patch sets armv7-a as the default arm architecture if no --arm-arch switch is passed.

When I run the test on the build I get some failures and unexpected passes. I've attached the list of these in case anyone wants to look at them.
Attachment #486691 - Attachment is obsolete: true
Attachment #488069 - Flags: review?(fklockii)
Attachment #488069 - Flags: review?(brbaker)
These are the test failures and unexpected passes for the android cross-compile build (armv7-a).
I saw "regress/bug_555568.abc_" fail for me today with a debug ARMv5 build.  And the shell tests passed with a build using the platform/android/Makefile and switching it to ARMv7-A as the default target.
Comment on attachment 488069 [details] [diff] [review]
Changes to address comments 13-15 of the first patch.

Something is wrong with the patch, please fix and repost

patch: **** malformed patch at line 149:      config.subst("ENABLE_TAMARIN", 1)
Attachment #488069 - Flags: review?(brbaker)
(In reply to comment #21)
> Comment on attachment 488069 [details] [diff] [review]
> Changes to address comments 13-15 of the first patch.
> 
> Something is wrong with the patch, please fix and repost
> 
> patch: **** malformed patch at line 149:      config.subst("ENABLE_TAMARIN", 1)

If I remove line #149 from the patch file then it will apply.
Attachment #488069 - Flags: review?(brbaker)
Attached patch Based on patch #488069 (obsolete) — — Splinter Review 
This patch is based on patch #488069 (Changes to address comments 13-15 of the first patch.)

I have NOT confirmed that this generates the exact same Makefile but just want to show the way that I would like to see the patch working.

This basically gives us the separation of "android" as the OS which will give us the flexibility in the future to have additional sections within "android" that would check the CPU type for 'arm' or 'i686'

../configure.py --enable-shell --target=arm-android
Attachment #488178 - Flags: feedback?(jsudduth)
Comment on attachment 488069 [details] [diff] [review]
Changes to address comments 13-15 of the first patch.

This patch needs a little help to apply and work properly.

There are indentation issues after applying the patch:
- build/configuration.py block @@ -276,9 +281,24 @@ does not properly indent after adding the new if block

- configure.py block @@ -324,6 +423,23 @@ does not properly indent after adding the new if block
Attached patch Based on patch #488069 (obsolete) — — Splinter Review 
This is the same as patch 488178 except that is resets the TARGET_OS to linux. 

This patch generates the same Makefile as patch "488069: Changes to address comments 13-15 of the first patch." The difference is that the configure call is different:

../configure.py --enable-shell --target=arm-android
vs
../configure.py --enable-shell --target=android-linux

I still think that this makes more sense since the CPU is not android and using android as the CPU assumes that we will only ever compile android for ARM and never for i686
Attachment #488178 - Attachment is obsolete: true
Attachment #488183 - Flags: feedback?(jsudduth)
Attachment #488178 - Flags: feedback?(jsudduth)
(In reply to comment #25)
> Created attachment 488183 [details] [diff] [review]
> Based on patch #488069
> 
> This is the same as patch 488178 except that is resets the TARGET_OS to linux. 
> 
> This patch generates the same Makefile as patch "488069: Changes to address
> comments 13-15 of the first patch." The difference is that the configure call
> is different:
> 
> ../configure.py --enable-shell --target=arm-android
> vs
> ../configure.py --enable-shell --target=android-linux
> 
> I still think that this makes more sense since the CPU is not android and using
> android as the CPU assumes that we will only ever compile android for ARM and
> never for i686

This looks fine to me. It does need to reset TARGET_OS to 'linux' in configuration.py but that's easy to live with.
(In reply to comment #26)
> (In reply to comment #25)
> > Created attachment 488183 [details] [diff] [review] [details]
> > Based on patch #488069
> > 
> > This is the same as patch 488178 except that is resets the TARGET_OS to linux. 
> > 
> > This patch generates the same Makefile as patch "488069: Changes to address
> > comments 13-15 of the first patch." The difference is that the configure call
> > is different:
> > 
> > ../configure.py --enable-shell --target=arm-android
> > vs
> > ../configure.py --enable-shell --target=android-linux
> > 
> > I still think that this makes more sense since the CPU is not android and using
> > android as the CPU assumes that we will only ever compile android for ARM and
> > never for i686
> 
> This looks fine to me. It does need to reset TARGET_OS to 'linux' in
> configuration.py but that's easy to live with.

Ok, spending more time with this code... There is no need to reset the TARGET_OS back to linux, what _should_ be done is that the "android" os should be introduced in the manifest files:

VMPI/manifest.mk
manifest.mk
shell/manifest.mk
As I am working through this patch I want to know which Android Makefile was used as the baseline.

[1] tamarin-redux/platform/android/Makefile
[2] flash/products/android/make.avm + make.common

I think that it probably should have been [2] since it is the current working files that flash is using and [1] is an old Makefile that was grabbed when android was just getting started.
Comment on attachment 488069 [details] [diff] [review]
Changes to address comments 13-15 of the first patch.

R-: patch malformed.  (We resolved why off-ticket.)

Please put me in the round for next review after you and Brent resolve the issue of whether we will treat android as OS or as CPU.  (I'm with Brent on this one -- android does not imply arm cpu, but does imply linux, so it makes more sense to say arm-android than to say android-linux.)
Attachment #488069 - Flags: review?(fklockii) → review-
I skimmed over the patch (couldn't help myself) and have a couple items of feedback:

- Don't you need to indent after the else: case in the Configuration __init__ ?  I'm paranoid about Python's white-space sensitivity, but even if its still doing the right thing with the current indentation, its still probably better for human comprehension to indent the else case.

- Are you deliberately ignoring the host environment's settings for CPPFLAGS/CXXFLAGS/CFLAGS/LDFLAGS/etc ?  I just noticed that the _target[0] == 'linux' case seems to allow the environment to override the defaults in the script.  I can imagine reasons to not inherit the host's settings, but I can also imagine that, since the Android SDK needs to populate the environment with its settings anyway, we might as well assume that those environment variables are indeed what we want to use.

- I like that you document that the Android SDK needs to be setup in some comments early on.  I also like that you are checking for ANDROID_BUILD_TOP to make sure that its been set up.  But you might consider merging the two (that is,having the message in the Exception include all of the information that is currently in the comment.)  Also, I'm not sure that running "choosecombo 0 1 2 3" actually works.  (I get lots of error messages of the form "No matches for product "2").  I did see similar instructions on the wiki, so it probably needs to be corrected there as well.
OS: Android → All
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Created attachment 488183 [details] [diff] [review] [details] [details]
> > > Based on patch #488069
> > > 
> > > This is the same as patch 488178 except that is resets the TARGET_OS to linux. 
> > > 
> > > This patch generates the same Makefile as patch "488069: Changes to address
> > > comments 13-15 of the first patch." The difference is that the configure call
> > > is different:
> > > 
> > > ../configure.py --enable-shell --target=arm-android
> > > vs
> > > ../configure.py --enable-shell --target=android-linux
> > > 
> > > I still think that this makes more sense since the CPU is not android and using
> > > android as the CPU assumes that we will only ever compile android for ARM and
> > > never for i686
> > 
> > This looks fine to me. It does need to reset TARGET_OS to 'linux' in
> > configuration.py but that's easy to live with.
> 
> Ok, spending more time with this code... There is no need to reset the
> TARGET_OS back to linux, what _should_ be done is that the "android" os should
> be introduced in the manifest files:
> 
> VMPI/manifest.mk
> manifest.mk
> shell/manifest.mk

I tried that early on in the development of this patch and couldn't get it to build. I'll give it another try now that I know more about how all this works.
(In reply to comment #28)
> As I am working through this patch I want to know which Android Makefile was
> used as the baseline.
> 
> [1] tamarin-redux/platform/android/Makefile
> [2] flash/products/android/make.avm + make.common
> 
> I think that it probably should have been [2] since it is the current working
> files that flash is using and [1] is an old Makefile that was grabbed when
> android was just getting started.

I used 1 because that's what we build with. A review of 2 would probably be helpful in answering some of the questions about variables that get set but aren't used so I'll do that.
(In reply to comment #32)
> (In reply to comment #28)
> > As I am working through this patch I want to know which Android Makefile was
> > used as the baseline.
> > 
> > [1] tamarin-redux/platform/android/Makefile
> > [2] flash/products/android/make.avm + make.common
> > 
> > I think that it probably should have been [2] since it is the current working
> > files that flash is using and [1] is an old Makefile that was grabbed when
> > android was just getting started.
> 
> I used 1 because that's what we build with. A review of 2 would probably be
> helpful in answering some of the questions about variables that get set but
> aren't used so I'll do that.

Brent and I discussed this briefly in the chat room.  Reviewing [2] sounds fine, but for this ticket, I would focus on replicating the current behavior of the buildbot, which I assume means [1].  That way this change should remain a refactoring of the code, rather than a potential source of meaningful changes to the builds.

Resolving the disparity between [1] and [2] is a good task but should be attacked separately after this ticket is resolved.
(In reply to comment #30)
> I skimmed over the patch (couldn't help myself) and have a couple items of
> feedback:
> 
> - Don't you need to indent after the else: case in the Configuration __init__ ?
>  I'm paranoid about Python's white-space sensitivity, but even if its still
> doing the right thing with the current indentation, its still probably better
> for human comprehension to indent the else case.
> 

I see correct indentation in my file after applying a patch locally with the malformation issues fixed, so this is probably just a malformed patch
problem.

> - Are you deliberately ignoring the host environment's settings for
> CPPFLAGS/CXXFLAGS/CFLAGS/LDFLAGS/etc ?  I just noticed that the _target[0] ==
> 'linux' case seems to allow the environment to override the defaults in the
> script.  I can imagine reasons to not inherit the host's settings, but I can
> also imagine that, since the Android SDK needs to populate the environment with
> its settings anyway, we might as well assume that those environment variables
> are indeed what we want to use.
> 

My objective with this patch was to bring it over to the new
environment as unchanged as possible, sort of a 'translation' approach, so I
set up the flags exactly like the original makefile. We can add environment flag checking as desired.

> - I like that you document that the Android SDK needs to be setup in some
> comments early on.  I also like that you are checking for ANDROID_BUILD_TOP to
> make sure that its been set up.  But you might consider merging the two (that
> is,having the message in the Exception include all of the information that is
> currently in the comment.)  Also, I'm not sure that running "choosecombo 0 1 2
> 3" actually works.  (I get lots of error messages of the form "No matches for
> product "2").  I did see similar instructions on the wiki, so it probably needs
> to be corrected there as well.

I think the wiki instructions were written when choosecombo gave slightly different prompts. It should read 'choosecombo 0 1 <CR> 3' since there's only one choice for target product now; I'll fix that in the next patch and expand the exception message.
(In reply to comment #34)
> I think the wiki instructions were written when choosecombo gave slightly
> different prompts. It should read 'choosecombo 0 1 <CR> 3' since there's only
> one choice for target product now; I'll fix that in the next patch and expand
> the exception message.

Seems like the following:

  % choosecombo 0 1 generic 3

would also work, and perhaps would be more generally useful for people describing command sequences.

(Weird that the choosecombo script does not list what the actual options are for the product choice (that's what "generic" is answering).  But that's not my department.)
(In reply to comment #35)
> (In reply to comment #34)
> > I think the wiki instructions were written when choosecombo gave slightly
> > different prompts. It should read 'choosecombo 0 1 <CR> 3' since there's only
> > one choice for target product now; I'll fix that in the next patch and expand
> > the exception message.
> 
> Seems like the following:
> 
>   % choosecombo 0 1 generic 3
> 
> would also work, and perhaps would be more generally useful for people
> describing command sequences.
> 
> (Weird that the choosecombo script does not list what the actual options are
> for the product choice (that's what "generic" is answering).  But that's not my
> department.)

Or, on the other end of the spectrum of answers: 

  % ( echo ; echo ; echo ; echo ) | choosecombo

would decouple the particular defaults and just build-in the assumption that four carriage returns are the way to select all defaults.
This patch includes everything in Brent's latest patch 488183 plus changes to improve the exception message when ANDROID_BUILD_TOP isn't found in the environment. I further modified the suggestion in comment #36 to make the instructions for running choosecombo even more generic; now if the default choices change the instructions will still apply. I also updated the wiki page to match. Here's the message as it prints out at the prompt:

ANDROID_BUILD_TOP not found in environment
Please mount android.dmg and cd to /Volumes/android/device
Run . build/envsetup.sh
Run choosecombo and press enter at each prompt
Attachment #488069 - Attachment is obsolete: true
Attachment #488183 - Attachment is obsolete: true
Attachment #489275 - Flags: review?(fklockii)
Attachment #489275 - Flags: review?(brbaker)
Attachment #488183 - Flags: feedback?(jsudduth)
Attachment #488069 - Flags: review?(brbaker)
Comment on attachment 489275 [details] [diff] [review]
Built on patch 488183, addresses comment #36 (exception message)

[review update] I am working on reviewing this patch, but will not be able to complete until tomorrow.
(i plan to start review of this tomorrow right after i finish with reviews for bug 555765.)
Blocks: 611094
Hmm, just a double-check on the plan: platform/android/ itself is not going away, just the Makefile and subsidiary files within it, right?  (Otherwise, I'm wondering where android_shell.sh and friends will migrate.)
(In reply to comment #7)
> It's coming slowly. I'm able to cross-compile a shell but it's smaller than the
> one we currently build (1.6 Mb vs. 2 Mb for the Release target); I'm
> investigating why there's a size difference.

FYI, the size difference for Debug builds is even more dramatic:

old: platform/android/avmshell debug: 35M
new: shell/avmshell debug: 14M

I also do not know where the (huge!) difference is coming from.  (Perhaps different -O settings when compiling in debug mode yielding absurdly large object code?)  Anyway, the build steps differ in some interesting ways.  But, based on the console output listing the compiled files when compiling Debug builds, it looks like:

* The following files are in the old build; but do not seem to be in the new one:

avm/SpyUtilsPosix.o
avm/avmplus.o
avm/pcre_maketables.o
avmshell/android_cpuid.o 

* The follow files are in the new build, but do not seem to be in the old one (most of these don't surprise me or even concern me; they are probably artifacts of platform/android/* getting stale, which is exactly what this bug is addressing):
avm/unixcpuid.o
avmshell/ST_mmgc_543560.o
avmshell/ST_mmgc_580603.o
mmgc/GCThreads.o
vprof/vprof.o
zlib/gzio.o


I'd like to know why the files from the old build have gone away, and whether it matters.
+        # SEARCH_DIRS gets picked up in configuration.py by MKPROGRAM
+        SEARCH_DIRS = "-L/Volumes/Builds$(topsrcdir)/objdir-release/"

What is this business?  I have no such volume mounted.  Inherited from old Makefile?

+    if os.environ['TARGET_BUILD_TYPE'] == 'debug':
+        DEBUG_CXXFLAGS += "-DDEBUG -D_DEBUG -DASYNC_DEBUG -O0 -ggdb3 "
+        DEBUG_CPPFLAGS = ""
+    else:
+        APP_CXXFLAGS += "-DNDEBUG -O3 -fomit-frame-pointer -fvisibility=hidden -finline-functions -fgcse-after-reload -frerun-cse-after-loop -frename-registers -fvisibility-inlines-hidden "
+        DEBUG_CPPFLAGS = ""

The TARGET_BUILD_TYPE environment variable also seems new.
(overall things are looking good, and I'm super happy that the darn recursive make -j4 invocation is gone at last!)
(In reply to comment #40)
> Hmm, just a double-check on the plan: platform/android/ itself is not going
> away, just the Makefile and subsidiary files within it, right?  (Otherwise, I'm
> wondering where android_shell.sh and friends will migrate.)

Haven't decided quite where everything will go yet; that's the next step in all this (wiring it into the build system) that we'll need to discuss in the QE group.
(In reply to comment #41)
> (In reply to comment #7)
> > It's coming slowly. I'm able to cross-compile a shell but it's smaller than the
> > one we currently build (1.6 Mb vs. 2 Mb for the Release target); I'm
> > investigating why there's a size difference.
> 
> FYI, the size difference for Debug builds is even more dramatic:
> 
> old: platform/android/avmshell debug: 35M
> new: shell/avmshell debug: 14M
> 
Hmm, I get a shell of ~32MB. Did you rerun choosecombo before making the debug Makfile with configure.py? That's important because the debug flags change quite a bit depending on the TARGET_BUILD_TYPE env setting.

> I also do not know where the (huge!) difference is coming from.  (Perhaps
> different -O settings when compiling in debug mode yielding absurdly large
> object code?)  Anyway, the build steps differ in some interesting ways.  But,
> based on the console output listing the compiled files when compiling Debug
> builds, it looks like:
> 
> * The following files are in the old build; but do not seem to be in the new
> one:
> 
> avm/SpyUtilsPosix.o
> avm/avmplus.o
> avm/pcre_maketables.o
> avmshell/android_cpuid.o 
> 
> * The follow files are in the new build, but do not seem to be in the old one
> (most of these don't surprise me or even concern me; they are probably
> artifacts of platform/android/* getting stale, which is exactly what this bug
> is addressing):
> avm/unixcpuid.o
> avmshell/ST_mmgc_543560.o
> avmshell/ST_mmgc_580603.o
> mmgc/GCThreads.o
> vprof/vprof.o
> zlib/gzio.o
> 
> 
> I'd like to know why the files from the old build have gone away, and whether
> it matters.

I noted these differences myself some time back and would also like to know the answer. Anyone?
(In reply to comment #42)
> +        # SEARCH_DIRS gets picked up in configuration.py by MKPROGRAM
> +        SEARCH_DIRS = "-L/Volumes/Builds$(topsrcdir)/objdir-release/"
> 
> What is this business?  I have no such volume mounted.  Inherited from old
> Makefile?

Good catch - this is an artifact from my local system; it will go away in the next patch.

> 
> +    if os.environ['TARGET_BUILD_TYPE'] == 'debug':
> +        DEBUG_CXXFLAGS += "-DDEBUG -D_DEBUG -DASYNC_DEBUG -O0 -ggdb3 "
> +        DEBUG_CPPFLAGS = ""
> +    else:
> +        APP_CXXFLAGS += "-DNDEBUG -O3 -fomit-frame-pointer -fvisibility=hidden
> -finline-functions -fgcse-after-reload -frerun-cse-after-loop
> -frename-registers -fvisibility-inlines-hidden "
> +        DEBUG_CPPFLAGS = ""
> 
> The TARGET_BUILD_TYPE environment variable also seems new.

I'm pretty sure TARGET_BUILD_TYPE has been there all along - it's important to getting the release vs. debug flags set right.
(In reply to comment #45)
> (In reply to comment #41)
> > (In reply to comment #7)
> > > It's coming slowly. I'm able to cross-compile a shell but it's smaller than the
> > > one we currently build (1.6 Mb vs. 2 Mb for the Release target); I'm
> > > investigating why there's a size difference.
> > 
> > FYI, the size difference for Debug builds is even more dramatic:
> > 
> > old: platform/android/avmshell debug: 35M
> > new: shell/avmshell debug: 14M
> > 
> Hmm, I get a shell of ~32MB.

Your shell of ~32MB is which one?  old?  or new?

> Did you rerun choosecombo before making the debug
> Makfile with configure.py? That's important because the debug flags change
> quite a bit depending on the TARGET_BUILD_TYPE env setting.

I have to re-run choosecombo, but select the same four choices, and the behavior will differ in some way?  That's interesting and surprising to me.

(Or are you saying I need to rerun choosecombo with different choices?  I had thought from the wiki that the choices for choosecombo only actually matter if you're building the OS itself -- and that is why I thought nothing of the directions (pretty much all of them) saying to just choose the default four choices no matter what.)
(In reply to comment #46)
> > The TARGET_BUILD_TYPE environment variable also seems new.
> 
> I'm pretty sure TARGET_BUILD_TYPE has been there all along - it's important to
> getting the release vs. debug flags set right.

I don't see it.

  -*- mode: grep; default-directory: "~/Dev/tamarin-redux/" -*-
  Grep started at Thu Nov 11 22:48:27
  
  find . -exec grep TARGET_BUILD_TYPE '{}' \;
  
  Grep finished (matches found) at Thu Nov 11 22:49:22

Can you point me in the right direction?
(In reply to comment #48)
> (In reply to comment #46)
> > > The TARGET_BUILD_TYPE environment variable also seems new.
> > 
> > I'm pretty sure TARGET_BUILD_TYPE has been there all along - it's important to
> > getting the release vs. debug flags set right.
> 
> I don't see it.
> 
>   -*- mode: grep; default-directory: "~/Dev/tamarin-redux/" -*-
>   Grep started at Thu Nov 11 22:48:27
> 
>   find . -exec grep TARGET_BUILD_TYPE '{}' \;
> 
>   Grep finished (matches found) at Thu Nov 11 22:49:22
> 
> Can you point me in the right direction?

(I certainly see that its defined in the Android envsetup.sh; what I'm questioning is why its being introduced into our build infrastructure when it was not referred to previously.)
Comment on attachment 489275 [details] [diff] [review]
Built on patch 488183, addresses comment #36 (exception message)

R+; none of the questions I had were deal breakers; they are things I would like resolved, but not worth blocking pushing the patch (though I am a little worried that it sounds like I've been misusing choosecombo all this time...)

Anyway, it sounds from your comments like we'll be seeing another patch posted; feel free to put me on the review request for that when/if you do.
Attachment #489275 - Flags: review?(fklockii) → review+
(In reply to comment #47)
> (In reply to comment #45)
> > (In reply to comment #41)
> > > (In reply to comment #7)
> > > > It's coming slowly. I'm able to cross-compile a shell but it's smaller than the
> > > > one we currently build (1.6 Mb vs. 2 Mb for the Release target); I'm
> > > > investigating why there's a size difference.
> > > 
> > > FYI, the size difference for Debug builds is even more dramatic:
> > > 
> > > old: platform/android/avmshell debug: 35M
> > > new: shell/avmshell debug: 14M
> > > 
> > Hmm, I get a shell of ~32MB.
> 
> Your shell of ~32MB is which one?  old?  or new?
> 

This is a shell I built after generating a new Makefile using the patched configure.py.

> > Did you rerun choosecombo before making the debug
> > Makfile with configure.py? That's important because the debug flags change
> > quite a bit depending on the TARGET_BUILD_TYPE env setting.
> 
> I have to re-run choosecombo, but select the same four choices, and the
> behavior will differ in some way?  That's interesting and surprising to me.
> 
> (Or are you saying I need to rerun choosecombo with different choices?  I had
> thought from the wiki that the choices for choosecombo only actually matter if
> you're building the OS itself -- and that is why I thought nothing of the
> directions (pretty much all of them) saying to just choose the default four
> choices no matter what.)

I should have explained the background to this more fully; it's part of the next step for this bug that is, wiring these changes into the build system, so I was sort of holding off. 

The current android Makefile (in /platform/android) contains some release and debug targets. To build one or the other you call it like this:

make MAKKECMDGOALS=release
make MAKECMMDGOALS=debug

so you get different builds. The cross-compile make files don't work this way, so instead of trying to duplicate the MAKECMDGOALS functionality for android only, I'm distinguishing between release and debug builds by detecting the TARGET_BUILD_TYPE env setting. That's most safely done by running choosecombo and choosing 2 at the second prompt. It can also be done by re-exporting these vars:

ANDROID_PRODUCT_OUT=/Volumes/android/device/out/debug/target/product/generic
OUT=/Volumes/android/device/out/debug/target/product/generic
TARGET_BUILD_TYPE=debug

(I don't know why ANDROID_PRODUCT_OUT and OUT are set to the same thing) 

So now it's obvious that the directions will need to be changed a bit.
(In reply to comment #48)
> (In reply to comment #46)
> > > The TARGET_BUILD_TYPE environment variable also seems new.
> > 
> > I'm pretty sure TARGET_BUILD_TYPE has been there all along - it's important to
> > getting the release vs. debug flags set right.
> 
> I don't see it.
> 
>   -*- mode: grep; default-directory: "~/Dev/tamarin-redux/" -*-
>   Grep started at Thu Nov 11 22:48:27
> 
>   find . -exec grep TARGET_BUILD_TYPE '{}' \;
> 
>   Grep finished (matches found) at Thu Nov 11 22:49:22
> 
> Can you point me in the right direction?

I thought you meant it wasn't in the patch before. True, TARGET_BUILD_TYPE is new to the tamarin-redux repo (in configure.py) for reasons explained in Comment 51.
(In reply to comment #51)
> The current android Makefile (in /platform/android) contains some release and
> debug targets. To build one or the other you call it like this:
> 
> make MAKKECMDGOALS=release
> make MAKECMMDGOALS=debug
> 
> so you get different builds. The cross-compile make files don't work this way,
> so instead of trying to duplicate the MAKECMDGOALS functionality for android
> only, I'm distinguishing between release and debug builds by detecting the
> TARGET_BUILD_TYPE env setting.

Why isn't --enable-debug a sufficient method for selecting which target you want your makefile to build?  (I thought that was how we selected between release/debug targets in the xplat system.)  Or is that outside the scope of this bug?
(In reply to comment #53)
> (In reply to comment #51)
> > The current android Makefile (in /platform/android) contains some release and
> > debug targets. To build one or the other you call it like this:
> > 
> > make MAKKECMDGOALS=release
> > make MAKECMMDGOALS=debug
> > 
> > so you get different builds. The cross-compile make files don't work this way,
> > so instead of trying to duplicate the MAKECMDGOALS functionality for android
> > only, I'm distinguishing between release and debug builds by detecting the
> > TARGET_BUILD_TYPE env setting.
> 
> Why isn't --enable-debug a sufficient method for selecting which target you
> want your makefile to build?  (I thought that was how we selected between
> release/debug targets in the xplat system.)  Or is that outside the scope of
> this bug?

The way to match the previous builds would be NOT to base this on TARGET_BUILD_TYPE but instead on the fact that MAKECMDGOALS=debug

http://hg.mozilla.org/tamarin-redux/annotate/f98eb78ba960/build/buildbot/slaves/android/scripts/build-debug-shell-android.sh#l65

The build system does NOT rerun choosecombo
(In reply to comment #54)
> (In reply to comment #53)
> > (In reply to comment #51)
> > > The current android Makefile (in /platform/android) contains some release and
> > > debug targets. To build one or the other you call it like this:
> > > 
> > > make MAKKECMDGOALS=release
> > > make MAKECMMDGOALS=debug
> > > 
> > > so you get different builds. The cross-compile make files don't work this way,
> > > so instead of trying to duplicate the MAKECMDGOALS functionality for android
> > > only, I'm distinguishing between release and debug builds by detecting the
> > > TARGET_BUILD_TYPE env setting.
> > 
> > Why isn't --enable-debug a sufficient method for selecting which target you
> > want your makefile to build?  (I thought that was how we selected between
> > release/debug targets in the xplat system.)  Or is that outside the scope of
> > this bug?
> 
> The way to match the previous builds would be NOT to base this on
> TARGET_BUILD_TYPE but instead on the fact that MAKECMDGOALS=debug
> 
> http://hg.mozilla.org/tamarin-redux/annotate/f98eb78ba960/build/buildbot/slaves/android/scripts/build-debug-shell-android.sh#l65
> 
> The build system does NOT rerun choosecombo

True, but MAKECMDGOALS isn't used by any other current build including cross-compile builds; since we probably want to make the android build match the others as much as possible maybe we shouldn't use it at all (of course, that also argues against using TARGET_BUILD_TYPE, but I'll get back to that). All MAKECMDGOALS does is set the BUILD_CONFIG variable in the Makefile to either 'Release' or 'Debug' if it isn't already set. Then BUILD_CONFIG is used to set a couple of variables to paths that are specific to the current android build and won't be used in the new android build environment (lines 119 and 121 in current Makefile).

So why did I use TARGET_BUILD_TYPE? Way back when I first started work on this bug I was unsure of what effect various of the many flags that are being set might have on the android SDK or the build environment. I decided to err on the side of caution so brought over as many of them as I could. I've since tracked down a number of flags in the current Makefile that aren't actually being used for anything, at least in our build environment. For example, YS_LIB and PLATFORM_LDFLAGS are used by Flash but not us, so don't need to be brought over to the cross-compile environment. 

I was imagining having our build system rerun choosecombo between release and debug builds to be sure we didn't miss something somewhere. But our current system doesn't do that and in fact, as far as I can tell doesn't reset the three environment variables that choosecombo resets (ANDROID_PRODUCT_OUT, OUT and TARGET_BUILD_TYPE). This puzzles me a bit because ANDROID_PRODUCT_OUT actually points to different directories in the android sdk when choosecombo is run as release or debug (/Volumes/android/device/out/target/product/generic vs. /Volumes/android/device/out/debug/target/product/generic respectively - Note '/debug' between /out and /target). But what's even stranger: there is no /debug folder on the android sdk we're building with, so why does choosecombo reset it? Will this folder be present on the public sdk that we'll be moving to soon? Because of this the new configure.py, like our current build, doesn't distinguish between release and debug in re this particular path (see near line 256 where LFLAGS_HEADLESS is set).

Anyway, as for using --enable-debug, I'll use that as long as we're sure NOT using TARGET_BUILD_TYPE and NOT running choosecombo has no effect on the build - it's a one-line change.
(In reply to comment #55)
> I was imagining having our build system rerun choosecombo between release and
> debug builds to be sure we didn't miss something somewhere. But our current
> system doesn't do that and in fact, as far as I can tell doesn't reset the
> three environment variables that choosecombo resets (ANDROID_PRODUCT_OUT, OUT
> and TARGET_BUILD_TYPE). This puzzles me a bit because ANDROID_PRODUCT_OUT
> actually points to different directories in the android sdk when choosecombo is
> run as release or debug (/Volumes/android/device/out/target/product/generic vs.
> /Volumes/android/device/out/debug/target/product/generic respectively - Note
> '/debug' between /out and /target). But what's even stranger: there is no
> /debug folder on the android sdk we're building with, so why does choosecombo
> reset it? Will this folder be present on the public sdk that we'll be moving to
> soon? Because of this the new configure.py, like our current build, doesn't
> distinguish between release and debug in re this particular path (see near line
> 256 where LFLAGS_HEADLESS is set).

I don't know if you're already seen this, but it seems apropos:

  "Your choosecombo values don't really matter when compiling Flash. They only matter when compiling the Android OS itself. The choosecombo scripts adds the Android dev tools to your shell PATH, so the Flashmakefiles can find Android's gcc."

From https://zerowing.corp.adobe.com/display/FlashPlayer/Android+-+For+Dummies
Yep, I saw that. Again, this all developed out of a sense of caution (paranoia?) due to the complexity of the build and my worries about hidden gotchas. 

I'll be posting a patch soon that includes the change to using --enable-debug and also the changes needed to include the new android build in our build system in the same manner as most of the other builds.
Comment on attachment 489275 [details] [diff] [review]
Built on patch 488183, addresses comment #36 (exception message)

Removing review pending, waiting for new patch.
Attachment #489275 - Flags: review?(brbaker)
This patch enables debug builds using --enable-debug instead of TARGET_BUILD_TYPE. It also wires up the android build into the build system in the same manner as the other platforms. The only changes necessary to do that (and the only diffferences between this patch and the last one 489275) were in tamarinredux.py and sandbox.py, plus renaming /slaves/android/scripts/build-check-android.sh to build-check.sh and /slaves/android/scripts/upload-asteam-android.sh to upoad-asteam.sh. so they can be called from tamarinredux.py/sandbox.py like the other builds. All other files, both in the old build folder /platform/android and in /slaves/android, have been left as they are for now.

I tested this with a sandbox run and verified that both release and debug shells built using the new code, were uploaded to asteam and were the new shells.
Attachment #489275 - Attachment is obsolete: true
Attachment #492339 - Flags: review?(brbaker)
Attachment #492339 - Flags: feedback?(fklockii)
Comment on attachment 492339 [details] [diff] [review]
Changes to add cross-compile android build to build system.

feedback+.

I don't know much about the buildbot scripts so I won't address those parts of the patch.

The configure.py and manifest.mk changes look clean.

There is a slight size difference in the builds between the old platform/android/Makefile and the new xplat Makefiles:

            old     new
release    2.0M    1.7M
  debug     35M     31M

But that might easily be chalked up to "out-of-date"ness artifacts in the old platform/android/Makefile.  (See e.g. the list of files included/excluded in the shift discussed in comment 41 and comment 45.)
Attachment #492339 - Flags: feedback?(fklockii) → feedback+
Comment on attachment 492339 [details] [diff] [review]
Changes to add cross-compile android build to build system.

Finishing review today (11/30/10).
Comment on attachment 492339 [details] [diff] [review]
Changes to add cross-compile android build to build system.

Requesting another quick review for me once buildbot changes have been made, otherwise my comments are centered around just a couple of small changes to move some items around a little:

./configure.py
- Can we remove the "android vars"? If they are unique to android can we just make sure that they are set?
 APP_CXXFLAGS = ""
@@ -114,6 +143,14 @@ 
 NSPR_INCLUDES = ""
 NSPR_LDOPTS = ""
 
+# android vars
+LFLAGS_HEADLESS = ""
+SEARCH_DIRS = ""
+DISABLE_RTMPE = None
+ANDROID_BUILD_TOP = None
+STRIP= ""
+LD=""
+
 if 'APP_CPPFLAGS' in os.environ:
     APP_CPPFLAGS += os.environ['APP_CPPFLAGS'] + " "
 if 'APP_CXXFLAGS' in os.environ:

- Move setting of STRIP and LD into build/configuration.py, this is were the other tools are set, this will also allow you to remove the config.subst() calls for STRIP and LD

./build/configuration.py
- set CXX an CC in the "self._acvars.update(" block instead of afterwards, match other platforms


I looked into the file size difference a little and I had thought that it might have been caused by how the program was linked, since the xplat links via libraries and the old android Makefile links by listing all of the object files. I tested linking the xplat in the same manner and it did not account for the size difference (only a very minor size increase). I would therefore speculate that the size difference is caused by different switches being passed to the compiler. In the previous android Makefile the gcc args were a subset of the g++ args, but in xplat script the gcc and g++ args are the same.  At this stage I think this might be introducing more work than what this bug is meant to accomplish since there is already a tracking bug to match the compiler switches with what the player is currently using (bug #... will post follow up comment) since we are currently using a very old/early copy of the android Makefile.


Buildbot Changes:
- compile steps should be modified to use compile_generic() build step
Example: (release build should be)
 android_compile_factory.addStep(compile_generic(name="Release", shellname="avmshell", args="--enable-shell --arm-arch=armv7-a --target=arm-android", upload="false", features="+AVMSYSTEM_32BIT +AVMSYSTEM_ARM"))
Attachment #492339 - Flags: review?(brbaker) → review-
Blocks: 615615
(In reply to comment #62)
> At this
> stage I think this might be introducing more work than what this bug is meant
> to accomplish since there is already a tracking bug to match the compiler
> switches with what the player is currently using (bug #... will post follow up
> comment) since we are currently using a very old/early copy of the android
> Makefile.

Android compiler flag review
https://bugzilla.mozilla.org/show_bug.cgi?id=615615
Attached patch Implement changes in comment #62 (obsolete) — — Splinter Review 
For now, only adding release and debug builds in tamarinredux.py and sandbox.py. I've tested that configure.py makes release/debug Makefiles and that they build the shells on my local Mac. Will run a sandbox build when I can bounce buildbot without disturbing other people's builds.
Attachment #492339 - Attachment is obsolete: true
Attachment #494202 - Flags: review?(brbaker)
Comment on attachment 494202 [details] [diff] [review]
Implement changes in comment #62

Reviewed the buildbot config changes and just have a single nit (which is code I provided):

You can remove the "features="+AVMSYSTEM_32BIT +AVMSYSTEM_ARM"" from the tamarinredux.py compile steps for android since the features will not be verified since the shell will NOT be executed on a device during the compilation step, so this is non-functional and should be removed.
Attachment #494202 - Flags: review?(brbaker) → review+
changeset: 5637:2a4f79371c71
user:      James Sudduth <jsudduth@adobe.com>
summary:   Bug 544540 - x-compile build android based on platform/android/Makefile - (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/2a4f79371c71
changeset: 5638:7b3071b0354a
user:      Brent Baker <brbaker@adobe.com>
summary:   Bug 544540: follow up patch to enable the x-compile script to run 'make' with multiple jobs, matching the number of cores available on the host machine. This will speed up the android builds (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/7b3071b0354a
Is this done?
Yes - I was just waiting to see if any unexpected issues cropped up.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: