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

anbox: fixes and upgrades #125600

Closed
wants to merge 23 commits into from

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Jun 4, 2021

Motivation for this change

Make anbox work.

Some changes, mainly geared towards making it work.

Technically some of the changes could be considered breaking... But anbox doesn't work at all currently. So it can't be more breaking than that.

The AArch64 image from anbox doesn't work on my system. Though the postmarketOS image works fine.

Outdated info about kernel config

Newer kernels will require this configuration to be set:

{
boot.kernelPatches = [
  {
    name = "anbox-options";
    patch = null;
    extraConfig = ''
      ASHMEM y
      ANDROID y
      ANDROID_BINDER_IPC y
      ANDROID_BINDERFS y
    '';
  }
];
}

I do not know if we would prefer setting these as defaults. They're not modules, so it changes the way the kernel acts I guess.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

Tested with:

  • Pinebook Pro, kernel 5.11, with kernel configuration snippet added
To be done
  • Test on x86_64
  • Test on kernel with compatible anbox module (e.g. 5.4)
  • Validate how it acts on 5.5+ when the kernel is configured "incorrectly"
  • Add image edition facilities (to allow end-users to e.g. add F-Droid or a rooting solution...)
  • Look at making the generated desktop files more robust (hint)
  • Adopt linux-zen patches? No they reverted them
  • Add "OSS" android-tools android-tools: init at 31.0.0p1 #124992 already did it... I feel silly.
  • Use those android tools for the anbox test
  • Give escape hatches for "expert mode use" through internal options; useAnboxModules and useBinderFS options that defaults according to the current semantics of looking at the kernel version. (Mobile NixOS, and devices using vendor kernels may not fit the requirements, but will actually work.)

TO ANYONE TESTING: Can you please include these informations:

  • Kernel package used (e.g. linuxPackages_latest)
  • How exactly you made ashmem and binderfs available

@edwtjo
Copy link
Member

edwtjo commented Jun 4, 2021

Good job (from a cursory inspection)! I will try these out during the weekend.

@cidkidnix
Copy link
Contributor

cidkidnix commented Jun 4, 2021

After the fix to the postmarketos image everything works well for me (I'm on x86_64). But if fails to launch android apps from desktop files without anbox session-manager running in a terminal

EDIT: Scratch that this might be due to my system setup to wipe /var every boot

@cidkidnix
Copy link
Contributor

1622820191_screenshot
Everything works well but audio is pretty badly delayed

@samueldr
Copy link
Member Author

samueldr commented Jun 4, 2021

Ah, note that even on another attempt a year back anbox session-manager never worked for me. I was using the same ocmmand anbox-application-manager aliases to.

Launching the first anbox app from a fresh boot/session may take some time, as it has to start the container too. Anbox assumes on a modern system it takes "up to 10 seconds". The assumption was changed to 100 seconds, similar to what postmarketOS does.

@samueldr samueldr force-pushed the feature/anbox-2021-06-refresh branch from eca7814 to b084ab6 Compare June 4, 2021 18:31
@samueldr
Copy link
Member Author

samueldr commented Jun 4, 2021

@cidkidnix can you also tell me the kernel version, and how you made ashmem and binderfs available? For statistical purposes :).

@cidkidnix
Copy link
Contributor

cidkidnix commented Jun 4, 2021

{
  config = lib.mkIf config.virtualisation.anbox.enable {
    boot.kernelPatches = [
      {
        name = "ashmem-binder";
        patch = null;
        extraConfig = ''
          ASHMEM y
          ANDROID y
          ANDROID_BINDER_IPC y
          ANDROID_BINDERFS y
          ANDROID_BINDER_DEVICES binder,hwbinder,vndbinder
        '';
      }
    ];
  };
}

I patched my nixpkgs with this PR, My kernel version is 5.11.21, and the above code snippet is what I have imported as a module to enable the kernel configs

EDIT: The audio issue would be resolved with this PR for anbox anbox/anbox#1034 but it has yet to be merged and I don't know if it ever will

@cidkidnix
Copy link
Contributor

I'll also be able to test on my pinephone sometime soon:tm:

@samueldr
Copy link
Member Author

samueldr commented Jun 4, 2021

Looks like the patch for audio requires a fresh image build. So that wouldn't help AFAICT.

It would be nice to see @danielfullmer's robotnix build the Anbox images, so that a more trustful environment can be built to run Anbox.

@samueldr
Copy link
Member Author

samueldr commented Jun 4, 2021

Here's an example usage of the option for customizing the image.

{ pkgs, ... }:
{
  virtualisation.anbox = {
    enable = true;
    image = pkgs.anbox-postmarketos-image;

    # https://gitlab.com/postmarketOS/pmaports/-/blob/bf6ad7a78c5506eb5ad9089e87f9c1cf7e8cd1f8/main/anbox-image/APKBUILD#L39-63
    imageModifications =
      let
        fdroid_apk = pkgs.fetchurl {
          url = "https://f-droid.org/repo/org.fdroid.fdroid_1013000.apk";
          sha256 = "1n5zcxsfn42b3i067pnkjy3xf5ljs0fj9h01v69xfincr84mlh0y";
        };
        fdroidpriv_apk = pkgs.fetchurl {
          url = "https://f-droid.org/repo/org.fdroid.fdroid.privileged_2120.apk";
          sha256 = "1axa72vfd8qq2dyk7d171vpwb6rf5ps59zrchhhgqmrqmpv88gww";
        };
      in
    ''
      (PS4=" $ "; set -x

        echo "Disabling su"
        rm -v system/xbin/su

        echo "Installing FDroid"
        mkdir system/app/FDroid
        mkdir system/priv-app/FDroid

        cp "${fdroid_apk}"     system/app/FDroid/${fdroid_apk.name}
        cp "${fdroidpriv_apk}" system/priv-app/FDroid/${fdroidpriv_apk.name}

        chmod 0644 system/app/FDroid/*
        chmod 0644 system/priv-app/FDroid/*
      )
    '';
  };
}

Customised this way, the end-user does not need to install F-Droid themselves, and F-Droid does not require additional fiddling around to install apps.

I personally do not think such things belong in Nixpkgs... Mainly because this uses some pre-built APKs...

Maybe we could provide a common way to pre-install any APK? Though I see that as a future improvement. The basic foundations exists to give the end-users the ability to do what they want.

@cidkidnix
Copy link
Contributor

Could we not make a script that runs once after anbox is run to adb install applications?

@samueldr
Copy link
Member Author

samueldr commented Jun 5, 2021

Could we not make a script that runs once after anbox is run to adb install applications?

Running once is... harder than it sounds. And doesn't handle removing apps.

The best bet actually would be to force the desired apps at the system level just like F-Droid here. Though I wonder how things act once a system app is removed. What happens with its data? With a normal Android app uninstall, the data goes away too.

Still, it's out of scope for these changes. The foundations are now here to work on more complex behaviours if desired.

I do not intend to work on fully nix-ifying the Anbox system config, it'd be hard for not enough benefits.

I think installing F-Droid here to allow you to then install apps you want, and have F-Droid managing updates is a good enough solution for what hopefully is a temporary problem. We don't want to rely on Android apps!

@cidkidnix
Copy link
Contributor

A massive change will probably be needed when erfan's A10 anbox stuff for halium gets ready to use, but we can deal with that when it happens https://github.com/Anbox-halium

@cidkidnix
Copy link
Contributor

cidkidnix commented Jun 5, 2021

To add on, when a system app is uninstalled all data should be removed, although that isn't always the case

@mvnetbiz
Copy link
Contributor

mvnetbiz commented Jun 5, 2021

Test on x86_64

Works with setting kernelPatches and after running manually mount -t binder binder /dev/binderfs

Test on kernel with compatible anbox module (e.g. 5.4)

application-manager starts on 5.4

Validate how it acts on 5.5+ when the kernel is configured "incorrectly"

On 5.10 without kernel CONFIG_ANDROID options, anbox-application-manager window stays open for a minute, then exits.

Jun 05 03:36:54 nixos anbox[1428]: Failed to start as either binder or ashmem kernel drivers are not loaded
Jun 05 03:36:54 nixos dbus-daemon[1111]: [session uid=1000 pid=1111] Activated service 'org.anbox' failed: Process org.anbox exited with status 1

@mvnetbiz
Copy link
Contributor

mvnetbiz commented Jun 5, 2021

For some reason it is not mounting /dev/binderfs for me. I'm trying to figure out, but you could make it a mount unit required by the container-manager like here. https://github.com/NixOS/nixpkgs/pull/102341/files#diff-b05067fd6d27b2c89be023bde744ecaab904222a09a136d004f9e5c5148fe966R81-R87

@samueldr
Copy link
Member Author

samueldr commented Jun 5, 2021

@mvnetbiz

I assume this is with e.g. 5.10 (lts) or the latest kernel.

  • You are using the amended kernel config, and booted in the rebuilt kernel, right?
  • What is the error messages from the failure to mount /dev/binderfs?

EDIT: github scrolled me to the last comment... the first comment I sent was not in any way helpful

@samueldr
Copy link
Member Author

samueldr commented Jun 5, 2021

Note that I don't know how this acts on 5.4 yet. I still need to get a "normal" computer that runs 5.4 nicely to test a bit with the anbox kernel modules.

I assume it's possible /dev/binderfs won't mount in that case. And it's possibly fine?

@samueldr
Copy link
Member Author

samueldr commented Jun 5, 2021

On 5.10 without kernel CONFIG_ANDROID options, anbox-application-manager window stays open for a minute, then exits.

Yes, that's to be expected. Nothing we can do if the kernel options are not enabled. They are features Anbox and Android uses.

samueldr and others added 4 commits June 8, 2021 01:55
Cleaner than forcing a "full" filesystem mount.

Co-authored-by: Matt Votava <mvnetbiz@gmail.com>
As noted in NixOS#102341 this is not
actually running as a forked process. It only tells the process that it
is running "as a daemon, so shut the warning up".

See `daemon_` here

 - https://github.com/anbox/anbox/blob/9de4e87cdd05135e1c71e6eadb68bf82719cebdf/src/anbox/cmds/container_manager.cpp#L38-L79

It is **strictly** used to hide that message.

Co-authored-by: Matt Votava <mvnetbiz@gmail.com>
The AArch64 image from anbox will not start.
@samueldr samueldr force-pushed the feature/anbox-2021-06-refresh branch from c477bda to fdf7b4b Compare June 8, 2021 05:55
@mvnetbiz
Copy link
Contributor

Tested again, both images on x86_64 are working.

@cidkidnix
Copy link
Contributor

I'll test but it seems anbox crashes for me when it's running in the background and restarts itself, temporarily(maybe?) causing my user to lose connection to systemd, started happening shortly after I enabled anbox

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/run-android-apps-in-nixos/14437/2

@@ -85,7 +79,7 @@ stdenv.mkDerivation rec {
systemd
];

patchPhase = ''
prePatch = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
prePatch = ''
postPatch = ''

otherwise patches no longer rapply.

@asdf8dfafjk
Copy link
Contributor

asdf8dfafjk commented Aug 25, 2021

@cidkidnix btw, anbox halium is at https://github.com/waydroid/anbox-halium

@alyssais
Copy link
Member

The lxc patch seems to no longer be required in latest nixpkgs.

@asdf8dfafjk
Copy link
Contributor

asdf8dfafjk commented Oct 3, 2021

Also relevant: #140200

@arximboldi
Copy link
Contributor

Sorry if this is a very newbie question, but is there a way to try to this PR without bringing this commit into the channels nor replacing my whole nixpkgs by this -- i.e. just using the relevant packages and options from here? I know how to do that when it's just one package to add to environemnt.systemPackages or alike, but this seems a bit more involved.

@Atemu
Copy link
Member

Atemu commented Nov 4, 2021

Best just clone nixpkgs, merge this branch and then nixos-rebuild test -I nixpkgs=/path/to/checkout/

@arximboldi
Copy link
Contributor

Aha! That's easy and nice, thank you @Atemu I didn't know I could just do test -I

@asdf8dfafjk
Copy link
Contributor

@arximboldi I don't want to disrespect anybody's work but at this point you should probably try #141076 since waydroid is newer than anbox

@mzabani
Copy link

mzabani commented Dec 27, 2021

Merged this into a relatively recent 21.11 nixpkgs branch, removed the lxc patch as suggested by @alyssais and it seems to work. Is something other than a suggested change
by @SuperSandro2000 holding this? Is there something I/we can do to help get this merged?

squashfsTools
];
} ''
echo "→ Extracting Anbox root image..."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
echo " Extracting Anbox root image..."
echo "-> Extracting Anbox root image..."

we shouldn't use special unicode characters

${cfg.imageModifications}
)

echo "→ Packing modified Anbox root image..."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
echo " Packing modified Anbox root image..."
echo "-> Packing modified Anbox root image..."

url = imgroot + "/android-7.1.2_r39-anbox_x86_64-userdebug.img";
sha256 = "16vmiz5al2r19wjpd44nagvz7d901ljxdms8gjp2w4xz1d91vzpm";
};
}.${stdenv.system}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing or throw unsupported system to eval on unsupported platforms.

@SuperSandro2000
Copy link
Member

by @SuperSandro2000 holding this? Is there something I/we can do to help get this merged?

I can't really help because I don't know kernels to well.

@Atemu
Copy link
Member

Atemu commented Jan 17, 2022

The kernel stuff should be dropped anyways. It was necessary to support kernels <5.5 which we don't really need to anymore.

@Mindavi
Copy link
Contributor

Mindavi commented May 10, 2022

Kernel config was added here: #145768

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 12, 2022
@rnhmjoj rnhmjoj mentioned this pull request Sep 3, 2023
12 tasks
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Sep 3, 2023

Continuation in #253146.

@rnhmjoj rnhmjoj closed this Sep 3, 2023
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