Closed Bug 1117028 Opened 5 years ago Closed 5 years ago

Try to look for zipalign in all of Android build tools; r=nalexander

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file)

Some Android SDK installations do not have the zipalign program in
the same directory as other Android build tools.  For example,
zipalign may be found in /build-tools/21.1.2 whereas the
rest of the build tools are in /build-tools/android-4.4.
Attachment #8543212 - Flags: review?(nalexander)
Comment on attachment 8543212 [details] [diff] [review]
Try to look for zipalign in all of Android build tools

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

I'm not really against this approach, but there's definitely a problem (with using aapt for both checks).  Over in Bug 1108782 I'm considering pinning to specific build tools versions.  You have a ton of experience with supporting multiple build chains, do you have a strong opinion on this type of interrogation versus version pinning?

::: build/autoconf/android.m4
@@ +354,5 @@
>          android_build_tools="$android_platform_tools" # SDK Tools < r22
>      fi
> +    all_android_build_tools=""
> +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`

nit: build_tools_directory.

@@ +355,5 @@
>      fi
> +    all_android_build_tools=""
> +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`
> +        if test -d "$tools_directory" -a -f "$tools_directory/aapt"; then

Something's weird here: we shouldn't expect to find aapt in both tools/ and build-tools/*/.
Attachment #8543212 - Flags: review?(nalexander) → feedback+
(In reply to Nick Alexander :nalexander from comment #2)
> Comment on attachment 8543212 [details] [diff] [review]
> Try to look for zipalign in all of Android build tools
> 
> Review of attachment 8543212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not really against this approach, but there's definitely a problem (with
> using aapt for both checks).  Over in Bug 1108782 I'm considering pinning to
> specific build tools versions.  You have a ton of experience with supporting
> multiple build chains, do you have a strong opinion on this type of
> interrogation versus version pinning?

I doubt that pinning is a good idea.  Here is why.  In my SDK setup, not all of the build-tools have a zipalign binary (which is what broke my local build originally, causing me to write this patch):

$ cd /path/to/adt-bundle-mac
$ find . | grep aapt
./sdk/build-tools/17.0.0/aapt
./sdk/build-tools/18.0.1/aapt
./sdk/build-tools/18.1.0/aapt
./sdk/build-tools/18.1.1/aapt
./sdk/build-tools/19.0.1/aapt
./sdk/build-tools/19.0.2/aapt
./sdk/build-tools/19.0.3/aapt
./sdk/build-tools/19.1.0/aapt
./sdk/build-tools/20.0.0/aapt
./sdk/build-tools/21.0.0/aapt
./sdk/build-tools/21.0.1/aapt
./sdk/build-tools/21.0.2/aapt
./sdk/build-tools/21.1.0/aapt
./sdk/build-tools/21.1.1/aapt
./sdk/build-tools/21.1.2/aapt
./sdk/build-tools/android-4.4/aapt
$ find . | grep zipalign
./sdk/build-tools/19.1.0/zipalign
./sdk/build-tools/20.0.0/zipalign
./sdk/build-tools/21.0.0/zipalign
./sdk/build-tools/21.0.1/zipalign
./sdk/build-tools/21.0.2/zipalign
./sdk/build-tools/21.1.0/zipalign
./sdk/build-tools/21.1.1/zipalign
./sdk/build-tools/21.1.2/zipalign
./sdk/docs/tools/help/zipalign.html

So if we for example pinned the build-tools to android-4.4 or to 19.0.3, my local build would break in the exact same way.

I guess the bigger question to ask is, do we care if aapt or some other binary comes from a different build-tools than zipalign?

> ::: build/autoconf/android.m4
> @@ +354,5 @@
> >          android_build_tools="$android_platform_tools" # SDK Tools < r22
> >      fi
> > +    all_android_build_tools=""
> > +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> > +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`
> 
> nit: build_tools_directory.

FWIW I just copied the var name from the code right above.

> @@ +355,5 @@
> >      fi
> > +    all_android_build_tools=""
> > +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> > +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`
> > +        if test -d "$tools_directory" -a -f "$tools_directory/aapt"; then
> 
> Something's weird here: we shouldn't expect to find aapt in both tools/ and
> build-tools/*/.

We're not.  Not sure what you mean.  Perhaps you were confused because of the variable name $tools_directory?
Flags: needinfo?(nalexander)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> (In reply to Nick Alexander :nalexander from comment #2)
> > Comment on attachment 8543212 [details] [diff] [review]
> > Try to look for zipalign in all of Android build tools
> > 
> > Review of attachment 8543212 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm not really against this approach, but there's definitely a problem (with
> > using aapt for both checks).  Over in Bug 1108782 I'm considering pinning to
> > specific build tools versions.  You have a ton of experience with supporting
> > multiple build chains, do you have a strong opinion on this type of
> > interrogation versus version pinning?
> 
> I doubt that pinning is a good idea.  Here is why.  In my SDK setup, not all
> of the build-tools have a zipalign binary (which is what broke my local
> build originally, causing me to write this patch):

Understood; this is because Google changes layouts and also just makes mistakes and forgets to include required things (zipalign, proguard).

> $ cd /path/to/adt-bundle-mac
> $ find . | grep aapt
> ./sdk/build-tools/17.0.0/aapt
> ./sdk/build-tools/18.0.1/aapt
> ./sdk/build-tools/18.1.0/aapt
> ./sdk/build-tools/18.1.1/aapt
> ./sdk/build-tools/19.0.1/aapt
> ./sdk/build-tools/19.0.2/aapt
> ./sdk/build-tools/19.0.3/aapt
> ./sdk/build-tools/19.1.0/aapt
> ./sdk/build-tools/20.0.0/aapt
> ./sdk/build-tools/21.0.0/aapt
> ./sdk/build-tools/21.0.1/aapt
> ./sdk/build-tools/21.0.2/aapt
> ./sdk/build-tools/21.1.0/aapt
> ./sdk/build-tools/21.1.1/aapt
> ./sdk/build-tools/21.1.2/aapt
> ./sdk/build-tools/android-4.4/aapt
> $ find . | grep zipalign
> ./sdk/build-tools/19.1.0/zipalign
> ./sdk/build-tools/20.0.0/zipalign
> ./sdk/build-tools/21.0.0/zipalign
> ./sdk/build-tools/21.0.1/zipalign
> ./sdk/build-tools/21.0.2/zipalign
> ./sdk/build-tools/21.1.0/zipalign
> ./sdk/build-tools/21.1.1/zipalign
> ./sdk/build-tools/21.1.2/zipalign
> ./sdk/docs/tools/help/zipalign.html
> 
> So if we for example pinned the build-tools to android-4.4 or to 19.0.3, my
> local build would break in the exact same way.

Well, we would pin to a version that we understood.  android-4.4 was an anomaly, now deprecated by Google; IIRC, 19.0.3 is not recent enough to build with (we require 21+).

> I guess the bigger question to ask is, do we care if aapt or some other
> binary comes from a different build-tools than zipalign?

It's going to be hell on earth to debug mismatched toolchains if such an issue were to arise.  But I don't know of a mismatch problem yet.

> > ::: build/autoconf/android.m4
> > @@ +354,5 @@
> > >          android_build_tools="$android_platform_tools" # SDK Tools < r22
> > >      fi
> > > +    all_android_build_tools=""
> > > +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> > > +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`
> > 
> > nit: build_tools_directory.
> 
> FWIW I just copied the var name from the code right above.

Right, and it's not parallel: tools -> tools_directory, build/tools -> build_tools_directory.
 
> > @@ +355,5 @@
> > >      fi
> > > +    all_android_build_tools=""
> > > +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> > > +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`
> > > +        if test -d "$tools_directory" -a -f "$tools_directory/aapt"; then
> > 
> > Something's weird here: we shouldn't expect to find aapt in both tools/ and
> > build-tools/*/.
> 
> We're not.  Not sure what you mean.  Perhaps you were confused because of
> the variable name $tools_directory?

No, we're trying to find tools/aapt in the other loop and build-tools/*/aapt in the new loop.  One of those is wrong, probably the older one (since aapt is in build-tools).  I think we shouldn't be looking in android-* at all anymore, in fact, but that's not this ticket.

I really don't want to screw you or other devs just for correctness and future plans.  How about we do this but with:

1) the build_tools_directory nit;
2) looking for tools/android;
3) looking for build-tools/*/zipalign.

Sound reasonable?  I'll r+ that patch.
Flags: needinfo?(nalexander) → needinfo?(ehsan)
(In reply to Nick Alexander :nalexander from comment #4)
> > $ cd /path/to/adt-bundle-mac
> > $ find . | grep aapt
> > ./sdk/build-tools/17.0.0/aapt
> > ./sdk/build-tools/18.0.1/aapt
> > ./sdk/build-tools/18.1.0/aapt
> > ./sdk/build-tools/18.1.1/aapt
> > ./sdk/build-tools/19.0.1/aapt
> > ./sdk/build-tools/19.0.2/aapt
> > ./sdk/build-tools/19.0.3/aapt
> > ./sdk/build-tools/19.1.0/aapt
> > ./sdk/build-tools/20.0.0/aapt
> > ./sdk/build-tools/21.0.0/aapt
> > ./sdk/build-tools/21.0.1/aapt
> > ./sdk/build-tools/21.0.2/aapt
> > ./sdk/build-tools/21.1.0/aapt
> > ./sdk/build-tools/21.1.1/aapt
> > ./sdk/build-tools/21.1.2/aapt
> > ./sdk/build-tools/android-4.4/aapt
> > $ find . | grep zipalign
> > ./sdk/build-tools/19.1.0/zipalign
> > ./sdk/build-tools/20.0.0/zipalign
> > ./sdk/build-tools/21.0.0/zipalign
> > ./sdk/build-tools/21.0.1/zipalign
> > ./sdk/build-tools/21.0.2/zipalign
> > ./sdk/build-tools/21.1.0/zipalign
> > ./sdk/build-tools/21.1.1/zipalign
> > ./sdk/build-tools/21.1.2/zipalign
> > ./sdk/docs/tools/help/zipalign.html
> > 
> > So if we for example pinned the build-tools to android-4.4 or to 19.0.3, my
> > local build would break in the exact same way.
> 
> Well, we would pin to a version that we understood.  android-4.4 was an
> anomaly, now deprecated by Google; IIRC, 19.0.3 is not recent enough to
> build with (we require 21+).

Are all SDK installation exactly the same though?  I suspect not, since I seem to be the first person who has hit this issue...

> > I guess the bigger question to ask is, do we care if aapt or some other
> > binary comes from a different build-tools than zipalign?
> 
> It's going to be hell on earth to debug mismatched toolchains if such an
> issue were to arise.  But I don't know of a mismatch problem yet.

That's a fair point.  :-)

> > > ::: build/autoconf/android.m4
> > > @@ +354,5 @@
> > > >          android_build_tools="$android_platform_tools" # SDK Tools < r22
> > > >      fi
> > > > +    all_android_build_tools=""
> > > > +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> > > > +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`
> > > 
> > > nit: build_tools_directory.
> > 
> > FWIW I just copied the var name from the code right above.
> 
> Right, and it's not parallel: tools -> tools_directory, build/tools ->
> build_tools_directory.

I guess I'll fix the existing names too then, since I don't think that the existing code is organized like that.

> > > @@ +355,5 @@
> > > >      fi
> > > > +    all_android_build_tools=""
> > > > +    for suffix in `ls "$android_sdk_root/build-tools" | sed -e "s,android-,999.," | sort -t. -k 1,1nr -k 2,2nr -k 3,3nr -k 4,4nr -k 5,5nr`; do
> > > > +        tools_directory=`echo "$android_sdk_root/build-tools/$suffix" | sed -e "s,999.,android-,"`
> > > > +        if test -d "$tools_directory" -a -f "$tools_directory/aapt"; then
> > > 
> > > Something's weird here: we shouldn't expect to find aapt in both tools/ and
> > > build-tools/*/.
> > 
> > We're not.  Not sure what you mean.  Perhaps you were confused because of
> > the variable name $tools_directory?
> 
> No, we're trying to find tools/aapt in the other loop and build-tools/*/aapt
> in the new loop.  One of those is wrong, probably the older one (since aapt
> is in build-tools).

I still think you're mistaken.  This is the loop I am talking about: <http://mxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#346>.  I literally copied it immediately after the loop, and changed it so that instead of breaking out of the loop the first time we find build-tools/*/aapt, we gather a list of all build-tools directory with aapt in them.  There is no code looking at anything under tools/ here.

> I think we shouldn't be looking in android-* at all
> anymore, in fact, but that's not this ticket.
> 
> I really don't want to screw you or other devs just for correctness and
> future plans.  How about we do this but with:
> 
> 1) the build_tools_directory nit;
> 2) looking for tools/android;
> 3) looking for build-tools/*/zipalign.
> 
> Sound reasonable?  I'll r+ that patch.

Based on the above, I don't think you're reading my patch correctly, so I'm not sure if that makes sense yet.  :-)
Flags: needinfo?(ehsan) → needinfo?(nalexander)
Comment on attachment 8543212 [details] [diff] [review]
Try to look for zipalign in all of Android build tools

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

You were absolutely correct, I misread what your change is doing.  Original patch looks good.  Sorry!
Attachment #8543212 - Flags: review+
No worries, thanks for the review!  :-)
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/b1741d5e3b99
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.