Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

androidenv: update generated expressions #82118

Merged
merged 13 commits into from Apr 5, 2020

Conversation

lucafavatella
Copy link
Contributor

@lucafavatella lucafavatella commented Mar 9, 2020

Depends on #82067 .

Motivation for this change

Refresh the generated expressions.

This started by reusing bits from #58131 , grew with some smoke testing of emulator and adb, and reusing some bits from #78623 .

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
    • I am afraid I do not have time for understanding and testing this - help welcome.
  • Built on platform(s)
    • NixOS
    • macOS
      • As mentioned in commit "bump emulator version to latest stable - as per Linux", "Each OS has a distinct version..." so on macOS it may be not possible to use an emulator: I am not sure whether emulator version is configurable with the current version of the scripts. Help is welcome.
    • other Linux distributions
      • Ubuntu Bionic on Travis CI.
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
    • I am afraid I do not have time for understanding and testing this - help welcome.
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
    • I am afraid I do not have time for understanding and testing this - help welcome.
  • Tested execution of all binary files (usually in ./result/bin/)
    • I tested some binaries, not all. Please refer to comments for details.
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    • I am afraid I do not have time for understanding and testing this - help welcome.
  • Ensured that relevant documentation is up to date
    • I did one update but I am afraid for bringing all of the documentation to tested working examples would take too much time and I am unable to do that. (I am not sure whether the documentation examples are also present somewhere as tests executed on some CI server - that would make the doc simpler to update.)
  • Fits CONTRIBUTING.md.

lucafavatella and others added 3 commits March 9, 2020 03:43
Update generate.sh to run using nix-shell. Also make it fail with
meaningful output instead of writing empty output files.

This is extracted from https://github.com/NixOS/nixpkgs PR 58131.

This relies on the shebang being used.
Updated with fixes for `convertsystemimages.xsl`:
- Use `type-details/codename` if it exists, falling back to
  `type-details/api-level`: this results in "Q" rather than "28" for
  preview images
- Use `<xsl:text>` elements to control whitespace in the output.

This is extracted from https://github.com/NixOS/nixpkgs PR 58131.
Entry `<remotePackage path="cmdline-tools;latest">` resulted in a
duplicated `"cmdline-tools"."1.0"`.
Move to a known version of the emulator.  Each OS has a distinct
version... pick the one on Linux.  A better solution would be to let
the user of `emulateApp` overload what the emulator version shall be
(and maybe it is already possible and I do not see it) - without need
to reproduce large portions of `default.nix`.

Using the previous emulator showed the following warning:
```
Your emulator is out of date, please update by launching Android Studio:
```

I am not aware of any reasons for not wanting the latest emulator (as
I expect it shall be compatible usually with more system images - not
less), so bump its default version.

Emulator release notes:
https://developer.android.com/studio/releases/emulator
... as same name hence clashing.

This shall rather be solved by enhancement of the scripts.
... complex as apparently dependent on OS of host of emulator.

This shall rather be solved by enhancement of the scripts.
This is extracted from https://github.com/NixOS/nixpkgs PR 78623.

The symptom I observed was:
```
builder for '/nix/store/7zz585ch9jqjvz8k88rf7fs046inxszq-emulator-30.0.3.drv' failed with exit code 1; last 10 log lines:
    libXext.so.6 -> found: /nix/store/qkmj8pa3ka9v335bbgx74ah4080fwdbf-libXext-1.3.4/lib/libXext.so.6
    libXfixes.so.3 -> found: /nix/store/nhp59xxq4g51mgpwgjsp8pvkn7531b86-libXfixes-5.0.3/lib/libXfixes.so.3
    libXi.so.6 -> not found!
    libXrender.so.1 -> not found!
    libXtst.so.6 -> not found!
    libasound.so.2 -> not found!
    libz.so.1 -> found: /nix/store/pb2am7dfsv524pb2z7m8mp8nkcmgd3cl-zlib-1.2.11/lib/libz.so.1
    libdbus-1.so.3 -> found: /nix/store/ddqrvb1b0xdb8g18f4advmq4wk0aiknd-dbus-1.12.16-lib/lib/libdbus-1.so.3
    libGL.so.1 -> found: /nix/store/bbpgyifsh2hv0ivhl3g4ik2nw7d10zdx-emulator-30.0.3/libexec/android-sdk/emulator/lib64/gles_mesa/libGL.so.1
    libc++.so.1 -> found: /nix/store/bbpgyifsh2hv0ivhl3g4ik2nw7d10zdx-emulator-30.0.3/libexec/android-sdk/emulator/lib64/libc++.so.1
```
@Ma27 Ma27 requested a review from svanderburg March 9, 2020 09:28
@lucafavatella lucafavatella force-pushed the androidenv-update-2 branch 2 times, most recently from 97c910f to 4d9c399 Compare March 9, 2020 15:17
Symptom:
```
Available Android targets:
----------
id: 1 or "android-28"
     Name: Android 9
     Type: Platform
     API level: 28
     Revision: 6
     Skins: HVGA, QVGA, WQVGA400, WQVGA432, WSVGA, WVGA800 (default), WVGA854, WXGA720, WXGA800, WXGA800-7in
 Tag/ABIs : google_ndk_playstore/x86
...
Error: Invalid --tag google_apis_playstore for the selected target.
```

For creation of AVD, switch from `android` to `avdmanager` as the
latter seems enabling selecting the image with less friction (and is
better documented as not deprecated).  This requires using recent
tools - from https://developer.android.com/studio/releases/sdk-tools :
> SDK Tools, Revision 25.3.0 (March 2017)
> ...
> `android avd` command-line functionality replaced with new
> `avdmanager` tool.

For listing of targets, switch from `android` to `avdmanager` as the
`android` command invocation fails in recent tools.  Symptom (not
missing `s` as backward incompatibility):
```
Invalid or unsupported command "list targets"

Supported commands are:
android list target
android list avd
android list device
android create avd
android move avd
android delete avd
android list sdk
android update sdk
```

References:
- https://developer.android.com/studio/tools/help/android
- https://developer.android.com/studio/command-line/avdmanager
This commit was built as a revert commit followed by deletion:
* Revert "androidenv: manually (!) delete oldest revision of google
  images".
* Delete other revision.

Using `systemImageType = "google_apis_playstore"` (and `abiVersion =
"x86"` and `platformVersion = "28"` - that I expect resolved to
`97d9d4f4a2afa8b0f5d52e90748e19c10406ca93`), the symptom is:
```
Warning: Observed package id 'system-images;android-28;google_ndk_playstore;x86' in inconsistent location '/nix/store/...-androidsdk/libexec/android-sdk/system-images/android-28/google_apis_playstore/x86' (Expected '/nix/store/...-androidsdk/libexec/android-sdk/system-images/android-28/google_ndk_playstore/x86')
...
Error: Package path is not valid. Valid system image paths are:
system-images;android-28;google_ndk_playstore;x86
```
How is the actual image name `google_ndk_playstore` when the fetched
image has id `google_apis_playstore`?

Attempt keeping - of the two images - the one that looks simpler.

For the `"28".google_apis."x86"` images, in the XML the differences
are: more complex license (what is `arm-dbt`?); higher emulator.
Namely:
```
		<uses-license ref="android-sdk-license"/>
		<dependencies>
			<dependency path="patcher;v4"/>
			<dependency path="emulator">
				<min-revision>
					<major>27</major>
					<minor>1</minor>
					<micro>7</micro>
...
		<uses-license ref="android-sdk-arm-dbt-license"/>
		<dependencies>
			<dependency path="patcher;v4"/>
			<dependency path="emulator">
				<min-revision>
					<major>29</major>
					<minor>1</minor>
					<micro>12</micro>
```

Analogously for `"28".google_apis_playstore."x86"`.
@lucafavatella
Copy link
Contributor Author

Tested execution of all binary files (usually in ./result/bin/)

  • I tested some binaries, not all. Please refer to comments for details.

Here is the comment (so the comment stays in the record with the commit history in the GitHub PR events sequence as shown in Web UI): I tested binaries adb and run-test-emulator on Ubuntu Bionic. The relevant excerpts of my usage are the following.

  - >
    nix-build
    -Q
    -E '
      with import (builtins.fetchGit {
        url = "https://github.com/lucafavatella/nixpkgs.git";
        ref = "emulator-google_apis_playstore";
        rev = "8ccc3870b0dca42593067c5bd78761f5d6f9da6b";
      }) {
        config.android_sdk.accept_license = true;
      };
      androidenv.emulateApp {
        name = "emulator-Device";
        /*
         * Android API 28 is Android 9.0.
         * See https://developer.android.com/studio/releases/platforms#9.0
         */
        platformVersion = "28";
        /*
         * From https://developer.android.com/studio/releases/emulator#support_for_arm_binaries_on_android_9_and_11_system_images :
         *
         * > ... you can now use the Android 9 x86 system image or any
         * > Android 11 system image to run your app – it is no longer
         * > necessary to download a specific system image to run ARM
         * > binaries.  These Android 9 and Android 11 system images support
         * > ARM by default and provide dramatically improved performance
         * > when compared to those with full ARM emulation.
         * >
         * > Known issues
         * > Some ARMv7 binaries fail to run on Android 11 x86 and x86_64
         * > system images.  Consider building for ARM64 when targeting
         * > Android 11.
         */
        abiVersion = "x86";
        systemImageType = "google_apis_playstore";
      }
    '
    -o emulator
  - test -f emulator/bin/run-test-emulator  # Ref https://github.com/NixOS/nixpkgs/blob/8d178aeed27482df666208a96b04357afc9726ad/pkgs/development/mobile/androidenv/emulate-app.nix#L134
  #
  - >
    nix-build
    -Q
    -E '
      with import <nixpkgs> {
        config.android_sdk.accept_license = true;
      };
      let
        androidComposition = androidenv.composeAndroidPackages {
        };
      in
      androidComposition.platform-tools
    '
    -o platform-tools
  - test -f platform-tools/bin/adb  # Ref https://github.com/NixOS/nixpkgs/blob/8d178aeed27482df666208a96b04357afc9726ad/pkgs/development/mobile/androidenv/compose-android-packages.nix#L254
  - export PATH="$(pwd)/platform-tools/bin:${PATH?}"
  - |
    AUDIO="-no-audio"
    EMU_PARAMS="-verbose -no-snapshot -no-window -camera-back none -camera-front none -selinux permissive -qemu -m 2048 -skip-adb-auth"
    # This double "sudo" monstrosity is used to have Travis execute the
    # emulator with its new group permissions and help preserve the rule
    # of least privilege.
    sudo -E sudo -u $USER -E \
      env NIX_ANDROID_EMULATOR_FLAGS="${AUDIO?} ${EMU_PARAMS?}" \
      ./emulator/bin/run-test-emulator
  - adb devices -l

Commit 8ccc3870b0dca42593067c5bd78761f5d6f9da6b is the merge of this branch (as of f01278d) and of PR #82119 (as of 38aa25a).

@lucafavatella
Copy link
Contributor Author

Tested execution of all binary files (usually in ./result/bin/)

Even if not strictly an output binary, I tested generate.sh - both by running it via bash ./generate.sh on a Nix-less macOS setup (for convenience), and on the following Docker image (via Docker Machine / VirtualBox) - comparing the results for determinism:

FROM nixos/nix
RUN mkdir /workdir
ADD generate.sh /workdir/
ADD convertpackages.xsl convertsystemimages.xsl convertaddons.xsl /workdir/
CMD ["/workdir/generate.sh"]

docker run --rm --mount type=bind,src=/generated,dst=/workdir/generated -w /workdir <IMAGE-ID-HERE>

... to the actually tested one.
@lucafavatella lucafavatella marked this pull request as ready for review March 9, 2020 20:06
@lucafavatella lucafavatella mentioned this pull request Mar 9, 2020
10 tasks
@lucafavatella
Copy link
Contributor Author

  • As mentioned in commit "bump emulator version to latest stable - as per Linux", "Each OS has a distinct version..." so on macOS it may be not possible to use an emulator: I am not sure whether emulator version is configurable with the current version of the scripts. Help is welcome.

@fmnxl Your insight may be doubly helpful here. I already included your change re dependencies (thanks!); I also noticed that you had proposed a change that makes emulator 29.3.6 available on both Linux and macOS. Could you please consider reviewing this (#82118) PR, and prepare a branch with the enhancements you envisage so we get most androidenv-related enhancements merged.

@lucafavatella lucafavatella mentioned this pull request Mar 9, 2020
10 tasks
@lucafavatella
Copy link
Contributor Author

@dicearr I read in one of your recent comments that you used the files changed in this PR (androidenv). It would be great if you managed to try this out and recommend fixes - preferably in the form of a branch/PR. Thanks!

@svanderburg
Copy link
Member

Great work so far. I need a bit of time to review this, but so far the androidenv changes look great.

@svanderburg
Copy link
Member

@lucafavatella To make my life a bit easier when it comes to reviewing. Can you try to run the tests in this repository? https://github.com/svanderburg/nix-androidenvtests

You can find tests in this folder: https://github.com/svanderburg/nix-androidenvtests/tree/master/tests

If you take a look at the root expression in deployment/default.nix, then you'll might notice that there is a parameter called useUpstream that is set to false. If you change this parameter to true then it will try to build the testcases with the androidenv implementation that is in Nixpkgs, instead of the implementation in the repository.

For example, running the following testcase described in the README.md file with the useUpstream parameter:

nix-build -A emulate_myfirstapp_debug.host_x86_64-linux.build_16.emulate_16.armeabi-v7a --arg useUpstream true

will build a script that builds an APK and launches the emulator using the androidenv from Nixpkgs.

If these testcases succeed, then there is nothing holding me back from integrating these changes into Nixpkgs.

@lucafavatella
Copy link
Contributor Author

@lucafavatella To make my life a bit easier when it comes to reviewing. Can you try to run the tests in this repository? https://github.com/svanderburg/nix-androidenvtests

@svanderburg Thanks for coming back to me, that is helpful! I hope to be able to try those out in the following couple of days, then I shall report back.

lucafavatella added a commit to lucafavatella/nix-androidenvtests that referenced this pull request Mar 12, 2020
lucafavatella added a commit to lucafavatella/nix-androidenvtests that referenced this pull request Mar 12, 2020
@lucafavatella
Copy link
Contributor Author

For example, running the following testcase described in the README.md file with the useUpstream parameter:

@svanderburg Could you please consider helping me with svanderburg/nix-androidenvtests#12 - there is one example failing. So far I did not arrive at investigating why.

Once the examples pass with upstream, I shall update those examples for showing they still pass with the changes in this PR #82118 .

@lucafavatella
Copy link
Contributor Author

Once the examples pass with upstream, I shall update those examples for showing they still pass with the changes in this PR #82118 .

Tests are passing with upstream on CI at svanderburg/nix-androidenvtests#12 - @svanderburg could you please consider merging that, enabling Travis CI in that repo? It would help future updates in this (nixpkgs) repo.

I still need to run the tests in svanderburg/nix-androidenvtests with the changes in this (#82118) PR, though I will not arrive at doing that for now. Help is welcome.

@dicearr
Copy link

dicearr commented Mar 21, 2020

@dicearr I read in one of your recent comments that you used the files changed in this PR (androidenv). It would be great if you managed to try this out and recommend fixes - preferably in the form of a branch/PR. Thanks!

Hi @lucafavatella I have checked your work so far and It looks great. I had to do similar stuff to make the emulator work locally. I don't think I can contribute anyhow to this PR but I will keep an eye on it and make a PR if I come up with anything.

@ijohanne
Copy link

ijohanne commented Apr 4, 2020

I've been eagerly awaiting getting the ndkBundle bumped in version (cargo-ndk doesn't work with anything less than 19c, and before this PR goes in - the only one available is 18). I checked out the branch and used -I nixpkgs=../nixpkgs to see if this one would remedy everything for me, but I found out that the pkgs/development/mobile/androidenv/ndk-bundle/make_standalone_toolchain.py_18.patch doesn't apply when using androidComposePackages. I'm not sure why the patch needs to be there (forgive me for missing some history on why regarding that) - but having removed the last three chunks in the diff - it applied cleanly, and I could then use cargo-ndk and build packages inside a nix-shell. Is it out of scope of this PR to also account for this before it gets merged?

@lucafavatella
Copy link
Contributor Author

Is it out of scope of this PR to also account for this before it gets merged?

@ijohanne I would prefer to keep it out of scope unless strictly necessary (and I understand it is not).

Once I get back to this PR, I shall check if the tests at https://github.com/svanderburg/nix-androidenvtests/tree/4daacee111d13b69a707618dab6af5c656036d20 still pass and attempt to fix this PR if not. This comes from review earlier in this PR:

... Can you try to run the tests in this repository? https://github.com/svanderburg/nix-androidenvtests

...

If these testcases succeed, then there is nothing holding me back from integrating these changes into Nixpkgs.

@svanderburg svanderburg merged commit 542a74a into NixOS:master Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants