android: Point EMULATOR_SDK_ROOT to a location inside the repository.

android: Point EMULATOR_SDK_ROOT to a location inside the repository.

The existing value would lead to a directory two levels above the root of
the source tree, which can easily break.

Instead, point to the SDK present in the android_tools checkout in src/third_party/android_tools/sdk and update the call sites that used to manually append 'android_tools/sdk' to the path.

R=peter@chromium.org,bulach@chromium.org,craigdh@chromium.org,frankf@chromium.org

[Peter Beverloo, 2013-09-09 14:05:49]

Thanks for the patch, Raphael!  Did this change brought you closer to running
tests on an emulator?

https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants.py
File build/android/pylib/constants.py (right):

https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants...
build/android/pylib/constants.py:17: 'third_party'))
This probably needs 'android_tools' and 'sdk' as arguments as well?

[Raphael Kubo da Costa (rakuco), 2013-09-09 14:08:04]

That was a fast response :-)

I'm having additional problems trying to get KVM to work inside a VM running in
an LXC container, but at least I've managed to get a few steps ahead with this
change.

https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants.py
File build/android/pylib/constants.py (right):

https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants...
build/android/pylib/constants.py:17: 'third_party'))
On 2013/09/09 14:05:50, Peter Beverloo wrote:
> This probably needs 'android_tools' and 'sdk' as arguments as well?

Not really; the places which still reference EMULATOR_SDK_ROOT append
['android_tools', 'sdk'] themselves.

[Peter Beverloo, 2013-09-09 14:12:00]

On 2013/09/09 14:08:04, Raphael Kubo da Costa (rakuco) wrote:
> That was a fast response :-)
> 
> I'm having additional problems trying to get KVM to work inside a VM running
in
> an LXC container, but at least I've managed to get a few steps ahead with this
> change.
> 
>
https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants.py
> File build/android/pylib/constants.py (right):
> 
>
https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants...
> build/android/pylib/constants.py:17: 'third_party'))
> On 2013/09/09 14:05:50, Peter Beverloo wrote:
> > This probably needs 'android_tools' and 'sdk' as arguments as well?
> 
> Not really; the places which still reference EMULATOR_SDK_ROOT append
> ['android_tools', 'sdk'] themselves.

GetSDK() in install_emulator_deps.py is the only path that doesn't require
android_tools/sdk/, but we can remove that function altogether since there's no
point in installing it (since it's an in-tree dependency for Android now).

[Raphael Kubo da Costa (rakuco), 2013-09-09 14:22:09]

On 2013/09/09 14:12:00, Peter Beverloo wrote:
> GetSDK() in install_emulator_deps.py is the only path that doesn't require
> android_tools/sdk/, but we can remove that function altogether since there's
no
> point in installing it (since it's an in-tree dependency for Android now).

Besides the 4 occurrences in install_emulator_deps.py, both avd.py and
emulator.py also append either 'android_tools' or 'android_tools/sdk' to
EMULATOR_SDK_ROOT (as of r222012).

I was thinking of getting rid of GetSDK() in a separate patch, but I can do that
here if you think it's better.

[Raphael Kubo da Costa (rakuco), 2013-09-09 14:33:54]

On 2013/09/09 14:22:09, Raphael Kubo da Costa (rakuco) wrote:
> I was thinking of getting rid of GetSDK() in a separate patch

https://codereview.chromium.org/23621025/

[Peter Beverloo, 2013-09-09 17:39:50]

On 2013/09/09 14:22:09, Raphael Kubo da Costa (rakuco) wrote:
> On 2013/09/09 14:12:00, Peter Beverloo wrote:
> > GetSDK() in install_emulator_deps.py is the only path that doesn't require
> > android_tools/sdk/, but we can remove that function altogether since there's
> no
> > point in installing it (since it's an in-tree dependency for Android now).
> 
> Besides the 4 occurrences in install_emulator_deps.py, both avd.py and
> emulator.py also append either 'android_tools' or 'android_tools/sdk' to
> EMULATOR_SDK_ROOT (as of r222012).
> 
> I was thinking of getting rid of GetSDK() in a separate patch, but I can do
that
> here if you think it's better.

Following your GetSDK() removal patch, all call-sites will be adding
third_party/android_tools/sdk/ (which is what CheckSDK should check for).  Let's
make the constant as clear as possible and add it in there.

[bulach, 2013-09-09 18:59:52]

https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants.py
File build/android/pylib/constants.py (right):

https://codereview.chromium.org/23513022/diff/1/build/android/pylib/constants...
build/android/pylib/constants.py:17: 'third_party'))
On 2013/09/09 14:08:04, Raphael Kubo da Costa (rakuco) wrote:
> On 2013/09/09 14:05:50, Peter Beverloo wrote:
> > This probably needs 'android_tools' and 'sdk' as arguments as well?
> 
> Not really; the places which still reference EMULATOR_SDK_ROOT append
> ['android_tools', 'sdk'] themselves.

would suggest change this to point to android_tools/sdk and remove the places
where this is being appended.. :)

[Raphael Kubo da Costa (rakuco), 2013-09-09 19:49:23]

Patch v2 is up (with an updated CL description). I've updated all places that
referenced EMULATOR_SDK_ROOT except for GetSDK(), which is being removed in the
other CL.

[Peter Beverloo, 2013-09-11 13:58:08]

+Maria since she should review this.

Following the suggestion from the e-mail thread, a few comments on your latest
patch.  Thank you Raphael!!  Please check with Maria whether all of this is OK
and works for the use-cases she has in mind.

https://codereview.chromium.org/23513022/diff/6001/build/android/install_emul...
File build/android/install_emulator_deps.py (right):

https://codereview.chromium.org/23513022/diff/6001/build/android/install_emul...
build/android/install_emulator_deps.py:38: True if the android_tools/sdk
directory exists in current directory.
nit: EMULATOR_SDK_ROOT is an absolute path, so the "in current directory"
addendum here isn't valid.

https://codereview.chromium.org/23513022/diff/6001/build/android/install_emul...
build/android/install_emulator_deps.py:81: dst =
os.path.join(constants.EMULATOR_SDK_ROOT, 'android_tools')
Shouldn't this just be EMULATOR_SDK_ROOT, since the android_tools directory is
not relevant anymore now?  We'd only like to extract the system-images/
directory if we decide to put the images here.

https://codereview.chromium.org/23513022/diff/6001/build/android/pylib/consta...
File build/android/pylib/constants.py (right):

https://codereview.chromium.org/23513022/diff/6001/build/android/pylib/consta...
build/android/pylib/constants.py:16: EMULATOR_SDK_ROOT =
os.path.abspath(os.path.join(
This is now equal to the ANDROID_SDK_ROOT constant, can we remove
EMULATOR_SDK_ROOT?

[Raphael Kubo da Costa (rakuco), 2013-09-11 14:15:05]

Patch v3 is up. It has been rebased on top of trunk, which automatically solves
one of Peter's comments (now that GetSDK() is gone).

I've updated the CL description and got rid of EMULATOR_SDK_ROOT altogether.

[Maria, 2013-09-11 16:55:42]

On 2013/09/11 14:15:05, Raphael Kubo da Costa (rakuco) wrote:
> Patch v3 is up. It has been rebased on top of trunk, which automatically
solves
> one of Peter's comments (now that GetSDK() is gone).
> 
> I've updated the CL description and got rid of EMULATOR_SDK_ROOT altogether.

This looks good to me. The only concern I have is that the SDK we used to
download would come with a system image for "armeabi-v7a", but the checked in
version doesn't have it. This can be done separately, but we probably need to
augment install_emulator_deps.py to also download ARM system image in addition
to x86.

[Peter Beverloo, 2013-09-11 17:29:35]

On 2013/09/11 16:55:42, Maria wrote:
> This looks good to me. The only concern I have is that the SDK we used to
> download would come with a system image for "armeabi-v7a", but the checked in
> version doesn't have it. This can be done separately, but we probably need to
> augment install_emulator_deps.py to also download ARM system image in addition
> to x86.

Agreed, which I also outlined in my e-mail.  Raphael, if you could re-instate
GetSDK() to download the SDK to /tmp and *only* extract the ARM system image to
the third_party/android_tools/sdk/system-images/ directory (for which I included
a command line :-)), then I think this is ready to fly.

https://codereview.chromium.org/23513022/diff/18001/build/android/avd.py
File build/android/avd.py (right):

https://codereview.chromium.org/23513022/diff/18001/build/android/avd.py#newc...
build/android/avd.py:26: os.environ['ANDROID_SDK_ROOT'] =
constants.ANDROID_SDK_ROOT
nit: should we allow overriding of the ANDROID_SDK_ROOT constant in the
environment, i.e. not update it if it's already set?  Trivial, we can address
this in another patch.

https://codereview.chromium.org/23513022/diff/18001/build/android/install_emu...
File build/android/install_emulator_deps.py (right):

https://codereview.chromium.org/23513022/diff/18001/build/android/install_emu...
build/android/install_emulator_deps.py:46: 'system-images', API_TARGET, 'x86'))
The comment doesn't match the code.

[Raphael Kubo da Costa (rakuco), 2013-09-11 17:44:42]

Patch v4 is up fixing your inline comments.

As for GetSDK(): instead of re-instating it, isn't it possible to download the
ARM images separately from a certai URL like we do for the x86 ones? One of the
benefits of getting rid of that function was not having to download a >600M zip
file with a lot of duplicate code...

[mariakhomenko, 2013-09-11 18:15:09]

I think
https://dl-ssl.google.com/android/repository/sysimg_armv7a-18_r01.zip would
give you just the system image files that you want. Agree it's going to be
a big improvement to not have to download the whole SDK.


On Wed, Sep 11, 2013 at 10:44 AM, <raphael.kubo.da.costa@intel.com> wrote:

> Patch v4 is up fixing your inline comments.
>
> As for GetSDK(): instead of re-instating it, isn't it possible to download
> the
> ARM images separately from a certai URL like we do for the x86 ones? One
> of the
> benefits of getting rid of that function was not having to download a
> >600M zip
> file with a lot of duplicate code...
>
>
https://codereview.chromium.**org/23513022/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[navabi, 2013-09-11 21:29:51]

I have read the comments on the different patches and tried to keep up. I
apologize if I missed something.

The original reason we download a separate emulator SDK was because that SDK
needs the system images. Those system images are large and we did not want to
commit them. I also wanted to avoid downloading the system-images into the tree,
so that that 'git status' shows untracked files as that often confuses people
(perhaps that is not that big of a deal).

What is nice about having a different SDK all-together is that I can open
sdk/tools/android and see if the proper system images are installed. A potential
problem with opening the version in our tree is that it aggressively wants to
upgrade other things, and developers may upgrade SDK libraries through the UI.

I basically wanted to avoid any chance that developers could mess up the trees
SDK which is used for building, from the emulator one that is used only for
launching the emulator.

I think this makes sense too, as long as we know the potential problems.

[Raphael Kubo da Costa (rakuco), 2013-09-12 09:41:12]

On 2013/09/11 18:15:09, mariakhomenko_google.com wrote:
> I think
> https://dl-ssl.google.com/android/repository/sysimg_armv7a-18_r01.zip would
> give you just the system image files that you want. Agree it's going to be
> a big improvement to not have to download the whole SDK.

Thank you. Done in https://codereview.chromium.org/23708033

It's important to note that both this CL and the one I mentioned above are based
on trunk, so if one lands the other will need to be adjusted.

[Raphael Kubo da Costa (rakuco), 2013-09-12 09:45:28]

On 2013/09/11 21:29:51, navabi wrote:
> What is nice about having a different SDK all-together is that I can open
> sdk/tools/android and see if the proper system images are installed. A
potential
> problem with opening the version in our tree is that it aggressively wants to
> upgrade other things, and developers may upgrade SDK libraries through the UI.
> 
> I basically wanted to avoid any chance that developers could mess up the trees
> SDK which is used for building, from the emulator one that is used only for
> launching the emulator.

I see. I haven't used the SDK myself so far (my primary interest at the moment
is getting some automated builds with tests backed by the emulator running), so
I'd rather let the other folks comment on these points. My biggest concern with
the previous setup is that it wasn't documented that using the emulator required
checking out the tree into a certain hierarchy with at least N levels of
directories that can be writable by the user (plus downloading another rather
big SDK).

[Peter Beverloo, 2013-09-13 13:47:12]

On 2013/09/11 21:29:51, navabi wrote:
> The original reason we download a separate emulator SDK was because that SDK
> needs the system images. Those system images are large and we did not want to
> commit them. I also wanted to avoid downloading the system-images into the
tree,
> so that that 'git status' shows untracked files as that often confuses people
> (perhaps that is not that big of a deal).
> 
> What is nice about having a different SDK all-together is that I can open
> sdk/tools/android and see if the proper system images are installed. A
potential
> problem with opening the version in our tree is that it aggressively wants to
> upgrade other things, and developers may upgrade SDK libraries through the UI.
> 
> I basically wanted to avoid any chance that developers could mess up the trees
> SDK which is used for building, from the emulator one that is used only for
> launching the emulator.
> 
> I think this makes sense too, as long as we know the potential problems.

I consider the proposed path of downloading the system images in the
third_party/android_tools/sdk/ directory a way of lazily loading these massive
files which not everyone needs.  There is precedent for this in Blink
(third-party Python libraries), Clang (binaries), NaCL (toolchain), and so
forth.  It's a different situation than having generated source files throughout
the source tree.

The task on us would be to update to the appropriate images when the used SDK
changes.  The risk I can see is if we ever want to include these images in the
default Android check-out, but checking in hundreds of megabytes of images which
change multiple times per year sounds unlikely.

I'm a bit wary signing off on this without your approval, Armand.  Do you think
Raphael's latest patches adequately address your concerns?

[bulach, 2013-09-13 14:01:43]

silly question: can we download with a specific name and put in .gitignore so it
never shows up in "git status"?

[Raphael Kubo da Costa (rakuco), 2013-09-13 14:06:13]

On 2013/09/13 14:01:43, bulach wrote:
> silly question: can we download with a specific name and put in .gitignore so
it
> never shows up in "git status"?

The downloaded zip files themselves are always downloaded with the same name,
but I guess you mean using a fixed name for the destination where the images are
put, right? The only thing that varies there is the ABI number, and I'm not the
best person to tell if removing that from the path makes sense. If it doesn't,
it would be a matter of updating .gitignore every time the ABI changes.

[bulach, 2013-09-13 17:05:12]

On 2013/09/13 14:06:13, Raphael Kubo da Costa (rakuco) wrote:
> On 2013/09/13 14:01:43, bulach wrote:
> > silly question: can we download with a specific name and put in .gitignore
so
> it
> > never shows up in "git status"?
> 
> The downloaded zip files themselves are always downloaded with the same name,
> but I guess you mean using a fixed name for the destination where the images
are
> put, right? The only thing that varies there is the ABI number, and I'm not
the
> best person to tell if removing that from the path makes sense. If it doesn't,
> it would be a matter of updating .gitignore every time the ABI changes.

yeah, I meant the destination: can we always put in a root folder that is
.gitignore'd ?
for instance, "build/android/downloaded_emulator_images/" ?
then we can use that directory as a dumping ground for all images, wdyt?

[Raphael Kubo da Costa (rakuco), 2013-09-13 17:18:27]

On 2013/09/13 17:05:12, bulach wrote:
> yeah, I meant the destination: can we always put in a root folder that is
> .gitignore'd ?
> for instance, "build/android/downloaded_emulator_images/" ?
> then we can use that directory as a dumping ground for all images, wdyt?

That I'm not sure of. The places that reference "system-images" seem to be
android/avd_configs/AVD_for_Galaxy_Nexus_by_Google_*.avd/config.ini, and I don't
know if they accept paths that are above the emulator tree.

One other possibility would be adding a .gitignore to android_tools itself and
make it ignore "/system-images".

[mariakhomenko, 2013-09-14 18:54:19]

IMHO, I am not sure that having unchecked-in files in the tree is a problem
at all. It will only show up if the user does git status within
third_party/android_tools directory, not at the top level. I would suggest
it's not worth handling and if it bothers anyone, they can always add it to
their .gitignore file.

I think that the possibility the users might use the android tool to update
their checked-in SDK and mess up their compilation is more serious since
it's likely to be hard to figure out what happened and harder to fix. But I
don't see a way to avoid it if we are using the checked in SDK.


On Fri, Sep 13, 2013 at 10:18 AM, <raphael.kubo.da.costa@intel.com> wrote:

> On 2013/09/13 17:05:12, bulach wrote:
>
>> yeah, I meant the destination: can we always put in a root folder that is
>> .gitignore'd ?
>> for instance, "build/android/downloaded_**emulator_images/" ?
>> then we can use that directory as a dumping ground for all images, wdyt?
>>
>
> That I'm not sure of. The places that reference "system-images" seem to be
> android/avd_configs/AVD_for_**Galaxy_Nexus_by_Google_*.avd/**config.ini,
> and I don't
> know if they accept paths that are above the emulator tree.
>
> One other possibility would be adding a .gitignore to android_tools itself
> and
> make it ignore "/system-images".
>
>
https://codereview.chromium.**org/23513022/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[navabi1, 2013-09-16 22:29:16]

On 2013/09/14 18:54:19, mariakhomenko_google.com wrote:
> IMHO, I am not sure that having unchecked-in files in the tree is a problem
> at all. It will only show up if the user does git status within
> third_party/android_tools directory, not at the top level. I would suggest
> it's not worth handling and if it bothers anyone, they can always add it to
> their .gitignore file.
> 
> I think that the possibility the users might use the android tool to update
> their checked-in SDK and mess up their compilation is more serious since
> it's likely to be hard to figure out what happened and harder to fix. But I
> don't see a way to avoid it if we are using the checked in SDK.

There is not a way to avoid it completely. But putting in system images, and
launching the android tool from the checked in SDK makes it more likely
developers will try to update. When you run the avd.py script, it prints the
commands to create the AVD and launch the AVD. If there is ever a problem, a
developer may try to run those commands manually, or run android to see if the
proper images have been downloaded. The "android" UI is very aggressive about
trying to get you updates.

If the primary problem is documentation, I would prefer we do not check into the
source tree, and keep it the way it is and I will improve the documentation. I
can extend the emulator testing instructions on AndroidTestInstructions
(https://code.google.com/p/chromium/wiki/AndroidTestInstructions), such that
Step 3 explains that the install_emulator_deps.py will download a separate SDK
with system images, and put it as a sibling directory to your chromium checkout.
WDYT?

[mariakhomenko, 2013-09-16 23:49:28]

So here's what I understand the issues are today:

New SDK install:
 + Isolates user's ability to build from their emulator setup
 + Guarantees to keep SDK stable and images in sync
 - Places files in a location that may be inconvenient for users who are
not using Google setup machines
 - Involves downloading a lot of large files

Reusing SDK Install:
 + Already exists in the tree at a convenient location
 - Will required to download system_images and create non-tracked file in
the git repo
 - Puts users at risk of accidentally updating their checked-in SDK and
messing up their build
 - If checked in SDK is updated, system_images need to be updated as well

What do you think about the following option:

1. Download an SDK + system images
2. Create a new directory under src/ for the sdk, e.g. emulator_sdk/...
3. Add emulator_sdk/... to .gitignore
4. Set up to use that directory for the emulator

This keeps the problem of having to do a large download (that's a one time
cost), but solves the issue of potentially placing SDK in a bad location
AND untracked files won't show up AND if the user updates this SDK this
will only affect the emulator and not their project compilation. WDYT?


On Mon, Sep 16, 2013 at 3:29 PM, <navabi@chromium.org> wrote:

> On 2013/09/14 18:54:19, mariakhomenko_google.com wrote:
>
>> IMHO, I am not sure that having unchecked-in files in the tree is a
>> problem
>> at all. It will only show up if the user does git status within
>> third_party/android_tools directory, not at the top level. I would suggest
>> it's not worth handling and if it bothers anyone, they can always add it
>> to
>> their .gitignore file.
>>
>
>  I think that the possibility the users might use the android tool to
>> update
>> their checked-in SDK and mess up their compilation is more serious since
>> it's likely to be hard to figure out what happened and harder to fix. But
>> I
>> don't see a way to avoid it if we are using the checked in SDK.
>>
>
> There is not a way to avoid it completely. But putting in system images,
> and
> launching the android tool from the checked in SDK makes it more likely
> developers will try to update. When you run the avd.py script, it prints
> the
> commands to create the AVD and launch the AVD. If there is ever a problem,
> a
> developer may try to run those commands manually, or run android to see if
> the
> proper images have been downloaded. The "android" UI is very aggressive
> about
> trying to get you updates.
>
> If the primary problem is documentation, I would prefer we do not check
> into the
> source tree, and keep it the way it is and I will improve the
> documentation. I
> can extend the emulator testing instructions on AndroidTestInstructions
>
(https://code.google.com/p/**chromium/wiki/**AndroidTestInstructions<https://c...),
> such that
> Step 3 explains that the install_emulator_deps.py will download a separate
> SDK
> with system images, and put it as a sibling directory to your chromium
> checkout.
> WDYT?
>
>
https://codereview.chromium.**org/23513022/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[atreat, 2013-09-20 14:03:30]

Just wanted to add to the discussion that I have applied the latest version of
this patch and it worked for me to setup my emulator for the first time.  One
big downside that I note to this patch is that it ties me to the SDK that is in
the tree which might be an older version.  I am at the whim of whoever is
responsible for updating the SDK in the tree.  One problem I encountered with
this is the gdb version of the SDK in the tree is quite old and doesn't allow me
to see the thread names of the chromium process(es).

[navabi, 2013-09-24 02:50:13]

On 2013/09/20 14:03:30, atreat wrote:
> Just wanted to add to the discussion that I have applied the latest version of
> this patch and it worked for me to setup my emulator for the first time.  One
> big downside that I note to this patch is that it ties me to the SDK that is
in
> the tree which might be an older version.  I am at the whim of whoever is
> responsible for updating the SDK in the tree.  One problem I encountered with
> this is the gdb version of the SDK in the tree is quite old and doesn't allow
me
> to see the thread names of the chromium process(es).

Good point atreat.

I like Maria's idea to checkout a separate SDK but it in src/ and add it to
.gitignore.

[navabi, 2013-10-03 17:50:44]

On 2013/09/24 02:50:13, navabi wrote:
> On 2013/09/20 14:03:30, atreat wrote:
> > Just wanted to add to the discussion that I have applied the latest version
of
> > this patch and it worked for me to setup my emulator for the first time. 
One
> > big downside that I note to this patch is that it ties me to the SDK that is
> in
> > the tree which might be an older version.  I am at the whim of whoever is
> > responsible for updating the SDK in the tree.  One problem I encountered
with
> > this is the gdb version of the SDK in the tree is quite old and doesn't
allow
> me
> > to see the thread names of the chromium process(es).
> 
> Good point atreat.
> 
> I like Maria's idea to checkout a separate SDK but it in src/ and add it to
> .gitignore.

Ping. WDYT about checking out a separate SDK into src, Raphael?

[Raphael Kubo da Costa (rakuco), 2013-10-04 11:16:05]

On 2013/10/03 17:50:44, navabi wrote:
> Ping. WDYT about checking out a separate SDK into src, Raphael?

Sounds like a good compromise to me. Having to download a separate SDK again is
not ideal, but I guess it is impossible to avoid due to all the reasons
mentioned above.

Doing all we've discussed in a clean way will probably require more than one CL,
so I've created http://code.google.com/p/chromium/issues/detail?id=304129 to
track everything (and also to summarize everything we've talked about so far).

Adam, you didn't show up in the autocompletion CC list in crbug.com, but feel
free to start the issue and chime in there.

[Raphael Kubo da Costa (rakuco), 2013-10-04 20:53:02]

Closing this one, crbug.com/304129 and crrev.com/25675010 are solving the whole
issue in a different way.