1460355 - "mach bootstrap" fails on NON artifact with "Warning: Unknown argument --package_file=/...mozilla-unified/python/mozboot/mozboot/android-packages.txt"

Status: RESOLVED FIXED

(Firefox Build System :: Bootstrap Configuration, defect)

Description

[Alessio Placitelli [:Dexter]]

This seems related to bug 1451447, except that after the latest m-c pull it's failing for non-artifact builds as well. The full error log is attached to this bug.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: Error log

Comment 2

[Aniket Kadam]

First things first, this might be fixed when the next Android SDK comes out, possibly in late August according to (https://www.techadvisor.co.uk/new-product/google-android/android-p-3670552/ and the info in the google bug report https://issuetracker.google.com/issues/66465833#comment7).

Would we want to add a workaround that's only going to be around maybe a month or two?

Description:
Google's sdkmanager version 26.1.1 and below have a bug that makes this command fail:
    args = [sdkmanager_tool, '--package_file={0}'.format(package_file_name)]
    subprocess.check_call(args)
The error is in Google's sdkmanager, where the --package_file param isn't recognized.
    The error on Google's side is being tracked here https://issuetracker.google.com/issues/66465833
Meanwhile, this workaround achieves installing all required Android packages by reading
them out of the same file that --package_file would have used, and passing them as strings.
So from here: https://developer.android.com/studio/command-line/sdkmanager
Instead of:
    sdkmanager --package_file=package_file [options]
I'd want to do the equivalent of:
    sdkmanager "platform-tools" "platforms;android-26"

Almost the pseudocode for the fix I'm proposing would require a change in the file: https://dxr.mozilla.org/mozilla-beta/rev/fc0a1b63ccc727f9cb4ed424d3ac88e86529fc46/python/mozboot/mozboot/android.py#245

From just replacing that line with the following.

    args = ""
    if ( isSdkManagerBrokenVersion() ):
        args = [sdkmanager_tool]
        args.extend(getPackagesToInstall(package_file_name))
    else:
        args = [sdkmanager_tool, '--package_file={0}'.format(package_file_name)]



def isSdkManagerBrokenVersion():
    return subprocess.check_output(['sdkmanager','--version']).strip() == '26.1.1'

def fixSdkManager(packagesFile):
    """
    sdkmanager version 26.1.1 and below have a bug that makes this command fail:
        args = [sdkmanager_tool, '--package_file={0}'.format(package_file_name)]
        subprocess.check_call(args)
    The error is in the sdkmanager, where the --package_file param isn't recognized.
        The error is being tracked here https://issuetracker.google.com/issues/66465833
    Meanwhile, this workaround achives installing all required Android packages by reading
    them out of the same file that --package_file would have used, and passing them as strings.
    So from here: https://developer.android.com/studio/command-line/sdkmanager
    Instead of:
        sdkmanager --package_file=package_file [options]
    We're doing:
        sdkmanager "platform-tools" "platforms;android-26"
    """
    allPackages = ""
    with open(packagesFile) as packagesFile:
        allPackages = packagesFile.readlines()
    # Since that file includes newlines that would distort the output, we need to strip them.
    return map(lambda cur: cur.strip(), allPackages)


However, I'd also like to propose a quirks.py, which may import from a quirks-android.py which can contain all the workarounds done in mozilla, for code that's external to mozilla.
This way we could track changes that are only required for a while and remove them when unnecessary while keeping them isolated from actual mozilla code.
The two functions I'm proposing to add, would go in there.

Comment 3

[Aniket Kadam]

So sorry, def fixSdkManager should be def getPackagesToInstall. I renamed the invocation to the proper name but missed the definition.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Aniket Kadam from comment #2)
> First things first, this might be fixed when the next Android SDK comes out,
> possibly in late August according to
> (https://www.techadvisor.co.uk/new-product/google-android/android-p-3670552/
> and the info in the google bug report
> https://issuetracker.google.com/issues/66465833#comment7).
> 
> Would we want to add a workaround that's only going to be around maybe a
> month or two?
> 
> Description:
> Google's sdkmanager version 26.1.1 and below have a bug that makes this
> command fail:
>     args = [sdkmanager_tool, '--package_file={0}'.format(package_file_name)]
>     subprocess.check_call(args)
> The error is in Google's sdkmanager, where the --package_file param isn't
> recognized.
>     The error on Google's side is being tracked here
> https://issuetracker.google.com/issues/66465833
> Meanwhile, this workaround achieves installing all required Android packages
> by reading
> them out of the same file that --package_file would have used, and passing
> them as strings.
> So from here: https://developer.android.com/studio/command-line/sdkmanager
> Instead of:
>     sdkmanager --package_file=package_file [options]
> I'd want to do the equivalent of:
>     sdkmanager "platform-tools" "platforms;android-26"
> 
> Almost the pseudocode for the fix I'm proposing would require a change in
> the file:
> https://dxr.mozilla.org/mozilla-beta/rev/
> fc0a1b63ccc727f9cb4ed424d3ac88e86529fc46/python/mozboot/mozboot/android.
> py#245
> 
> From just replacing that line with the following.
> 
>     args = ""
>     if ( isSdkManagerBrokenVersion() ):
>         args = [sdkmanager_tool]
>         args.extend(getPackagesToInstall(package_file_name))
>     else:
>         args = [sdkmanager_tool,
> '--package_file={0}'.format(package_file_name)]

Let's just try the --package_file=... approach; if it fails, try the command line approach.  If both fail, print the second output and bail out.  If you can detect the specific issue with the argument and not try the command line approach if it's not this issue, great; if not, fine.

> 
> def isSdkManagerBrokenVersion():
>     return subprocess.check_output(['sdkmanager','--version']).strip() ==
> '26.1.1'

Meh; simplicity is better than hard checks.

> def fixSdkManager(packagesFile):
>     """
>     sdkmanager version 26.1.1 and below have a bug that makes this command
> fail:
>         args = [sdkmanager_tool,
> '--package_file={0}'.format(package_file_name)]
>         subprocess.check_call(args)
>     The error is in the sdkmanager, where the --package_file param isn't
> recognized.
>         The error is being tracked here
> https://issuetracker.google.com/issues/66465833
>     Meanwhile, this workaround achives installing all required Android
> packages by reading
>     them out of the same file that --package_file would have used, and
> passing them as strings.
>     So from here:
> https://developer.android.com/studio/command-line/sdkmanager
>     Instead of:
>         sdkmanager --package_file=package_file [options]
>     We're doing:
>         sdkmanager "platform-tools" "platforms;android-26"
>     """
>     allPackages = ""
>     with open(packagesFile) as packagesFile:
>         allPackages = packagesFile.readlines()
>     # Since that file includes newlines that would distort the output, we
> need to strip them.
>     return map(lambda cur: cur.strip(), allPackages)
> 
> 
> However, I'd also like to propose a quirks.py, which may import from a
> quirks-android.py which can contain all the workarounds done in mozilla, for
> code that's external to mozilla.
> This way we could track changes that are only required for a while and
> remove them when unnecessary while keeping them isolated from actual mozilla
> code.
> The two functions I'm proposing to add, would go in there.

It's quirks all the way down!  I'm all for making code easy to delete, but I think you can do that much more simply -- just try one and then try the other.  Eventually, we get rid of the "try the other" branch, if anybody notices it.

For Python, we generally use snake_case, not CamelCase.

Flag me for patch review.  Thanks!

Comment 5

[Aniket Kadam]

Attachment: Bug 1460355 - Fix: Change how the packages are sent to the sdkmanager to install, avoid missing argument.

The argument --package_file was removed in the latest sdkmanager by Google's Android. But the docs for it say
packages can also be sent by putting them in quotes and calling the sdk manager with them as individual args.
So now instead of sending the file directly with the --package_file argument, the package names are read from
the file and the sdk manager is called with them as individual args.
Historically this has been thought of as a bug that happens with the wrong version of the JDK, but this can be
reproduced with just java 1.8.0_181 and the most up to date version of sdkmanager currently 26.1.1
Important note, the mach bootstrap command downloads an older version of the sdk and this bug is not present in
the older version.
Since the way of updating packages I'm proposiing to use is backwards compatible, there shouldn't be any problem
in any version of the sdkamanger.
This is a simpler fix than trying the --package_file argument, particularly because it would involve capturing
output (to detect this particular bug) that's also supposed to be shown to the user because this also happens
when the user is supposed to be interacting with the install.

Review commit: https://reviewboard.mozilla.org/r/259044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259044/

Comment 6

[Aniket Kadam]

I tried to do the both branch method, I ran into the following issue:
1. Trying to capture the error, which gets put into STDOUT as far as I can tell, along with printing the output to the console turns out to be really hard. This is to account for the normal flow where people are asked to accept the licenses themselves.

Doing that for a while, I realized that what we're trying to do here is really simple. The docs specify that there are two ways to specify packages to the sdkmanager, one is to give a file to the argument --package_file. The other is to just list the packages in front of it.

SUGGESTED FIX:
If we just list the packages in front of it all the time, it's a very minimal code change that should just work everywhere all the time since it's also one of the accepted ways to do this in the sdkmanager docs.

This, in my opinion, leads to simpler code to read and is a smaller modification than the branch and handling cases that result from it.

I've submitted this for review here, please let me know if I should try again the previous way.
https://reviewboard.mozilla.org/r/259044/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8994492 [details]
Bug 1460355 - Fix: Change how the packages are sent to the sdkmanager to install, avoid missing argument.

https://reviewboard.mozilla.org/r/259044/#review266072


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: python/mozboot/mozboot/android.py:232
(Diff revision 1)
>          # preserve the old convention to smooth detecting existing SDK
>          # installations.
>          install_mobile_android_sdk_or_ndk(sdk_url, os.path.join(mozbuild_path,
>                                            'android-sdk-{0}'.format(os_name)))
>  
> +def get_packages_to_install(packages_file_name):

Error: Expected 2 blank lines, found 1 [flake8: E302]

::: python/mozboot/mozboot/android.py:234
(Diff revision 1)
>          install_mobile_android_sdk_or_ndk(sdk_url, os.path.join(mozbuild_path,
>                                            'android-sdk-{0}'.format(os_name)))
>  
> +def get_packages_to_install(packages_file_name):
> +    """
> +    sdkmanager version 26.1.1 (current) and some versions below have a bug that makes this command fail:

Error: Line too long (104 > 99 characters) [flake8: E501]

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8994492 [details]
Bug 1460355 - Fix: Change how the packages are sent to the sdkmanager to install, avoid missing argument.

https://reviewboard.mozilla.org/r/259044/#review266122

This looks great!  Could you address the automated lint issues, and s/proposiing/proposing/ in the commit message, but otherwise this is good to go.  Many thanks!

Comment 9

[Aniket Kadam]

Comment on attachment 8994492 [details]
Bug 1460355 - Fix: Change how the packages are sent to the sdkmanager to install, avoid missing argument.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259044/diff/1-2/

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8994492 [details]
Bug 1460355 - Fix: Change how the packages are sent to the sdkmanager to install, avoid missing argument.

https://reviewboard.mozilla.org/r/259044/#review266130


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: python/mozboot/mozboot/android.py:232
(Diff revision 2)
>          # preserve the old convention to smooth detecting existing SDK
>          # installations.
>          install_mobile_android_sdk_or_ndk(sdk_url, os.path.join(mozbuild_path,
> -                                          'android-sdk-{0}'.format(os_name)))
> +                                                                'android-sdk-{0}'.format(os_name)))
>  
> +def get_packages_to_install(packages_file_name):

Error: Expected 2 blank lines, found 1 [flake8: E302]

Comment 11

[Aniket Kadam]

Comment on attachment 8994492 [details]
Bug 1460355 - Fix: Change how the packages are sent to the sdkmanager to install, avoid missing argument.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259044/diff/2-3/

Comment 12

[Nick Alexander :nalexander [he/him]]

Aniket -- I deleted a comment on your initial patch saying that you'd done a great job meeting expectations -- good commit message, well formatted patch, no extraneous changes -- because I figured this wasn't your first time committing to Firefox.  But Bugzilla is marking you as "new to Bugzilla", so I'll say it here: great (first?) patch!  Thanks!

Comment 13

[Aniket Kadam]

It's my first commit to any open source project actually! It was a pretty awesome experience, thanks! So many people were really nice in helping me navigate this process, I hope I can return the favor to others.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/427b229b7ea4
Fix: Change how the packages are sent to the sdkmanager to install, avoid missing argument. r=nalexander

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/427b229b7ea4