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

Added option to run ADB in no-streaming mode #1922

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

xperroni
Copy link

ADB can install an app either directly from the host ("streamed install"), or by copying the APK to the device and then running the installation process. The former is the current default, but the latter is still needed for some devices. Added an option allowing streamed install to be disabled.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

ADB can install an app either directly from the host ("streamed install"), or
by copying the APK to the device and then running the installation process. The
former is the current default, but the latter is still needed for some devices.
Added an option allowing streamed install to be disabled.

Signed-off-by: Helio Perroni Filho <[email protected]>
@mhsmith
Copy link
Member

mhsmith commented Jul 19, 2024

In my experience ADB always chooses the mode automatically based on the version of the device. Why would you need to override that?

@xperroni
Copy link
Author

In my experience ADB always chooses the mode automatically based on the version of the device.

That's not always the case. For example, it's an ongoing issue in LineageOS. That's actually what motivated me to write this patch.

@mhsmith
Copy link
Member

mhsmith commented Jul 21, 2024

OK, but since this is a rare issue, I don't think it justifies adding a specific Briefcase argument. Instead, we could create an --Xadb-install argument which follows the pattern of the existing --Xemulator, allowing anything to be passed through to the subprocess.

@freakboy3742
Copy link
Member

OK, but since this is a rare issue, I don't think it justifies adding a specific Briefcase argument. Instead, we could create an --Xadb-install argument which follows the pattern of the existing --Xemulator, allowing anything to be passed through to the subprocess.

I agree. This seems like a very specific niche workaround for a specific Linux distro, not something that needs a generic capability added to Briefcase. However, adb does take a lot of options, and some of them may be useful under edge-case circumstances, so we should provide a generic mechanism to expose those options.

Replaced the --no-streaming option with the more general --Xadb-install, which
allows passing arbitrary options to adb install.

Signed-off-by: Helio Perroni Filho <[email protected]>
@xperroni
Copy link
Author

I replaced the --no-streaming option with the more general --Xadb-install as discussed, but now I'm getting a coverage error in the CI / unit test suite. I'm having trouble understanding what the exact issue is; I suppose this means I need to add more tests for the new option, but how do I know what the coverage system is looking for?

@xperroni
Copy link
Author

OK, I seem to have figured a way to placate the code coverage system, though to be honest I'm still not sure what was wrong with the previous version of the code.

@freakboy3742
Copy link
Member

OK, I seem to have figured a way to placate the code coverage system, though to be honest I'm still not sure what was wrong with the previous version of the code.

FWIW: The coverage gap was reported as 1503->1506. This is telling you about branch coverage - the branch that jumps from line 1503 to line 1506 (i.e., the implicit else: pass that was part of the original if extra_args is None statement).

If you just have line coverage, calling the method with extra_args = None will cover every line of code that is written. With branch coverage, you also need to cover the implicit else: pass case, to ensure that you've tested every way the code can be used.

This isn't picked up when you convert the line into an inline ternary statement, due to a bug/missing feature in how coverage works - but it should ideally still have a test case.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks good!

One suggestion about the release note; and there's a need for documentation of the new option in docs/reference/platforms/android/gradle.rst. You can use the documentation for --Xemulator as a template for this.

Also, as noted in my note about coverage, there's one more test needed around invoking install_apk with extra_args=None. That's probably a good opportunity to revert to the older if extra_args is None syntax - to me, it's a little clearer than the "all in one line, but avoids the coverage problem" approach.

@@ -0,0 +1 @@
Added to ``briefcase run android`` a ``--Xadb-install`` option for passing extra arguments to ``adb install``. This makes it possible to handle various corner cases, for example, setting the ``--no-streaming`` option to make installation work for devices running recent versions of LineageOS.
Copy link
Member

Choose a reason for hiding this comment

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

These notes should be written as a description of the "exciting new feature in the release", not a rehash of the commit message.

Suggested change
Added to ``briefcase run android`` a ``--Xadb-install`` option for passing extra arguments to ``adb install``. This makes it possible to handle various corner cases, for example, setting the ``--no-streaming`` option to make installation work for devices running recent versions of LineageOS.
The ``--Xadb-install`` option can now be used to pass arguments to ``adb install`` when running apps on Android.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants