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

RESOLVED FIXED in Firefox 63

Status

RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: Dexter, Assigned: anikadamg)

Tracking

Version 3
mozilla63

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
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.
(Reporter)

Comment 1

7 months ago
Created attachment 8974459 [details]
Error log
(Assignee)

Comment 2

5 months ago
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.
(Assignee)

Comment 3

5 months ago
So sorry, def fixSdkManager should be def getPackagesToInstall. I renamed the invocation to the proper name but missed the definition.
(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 hidden (mozreview-request)
(Assignee)

Comment 6

5 months ago
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

5 months ago
mozreview-review
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

5 months ago
mozreview-review
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!
Attachment #8994492 - Flags: review?(nalexander) → review+
Comment hidden (mozreview-request)

Comment 10

5 months ago
mozreview-review
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 hidden (mozreview-request)
Assignee: nobody → anikadamg
Status: NEW → ASSIGNED
Keywords: checkin-needed
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!
(Assignee)

Comment 13

5 months ago
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

5 months ago
Pushed by rgurzau@mozilla.com:
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
Keywords: checkin-needed

Comment 15

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/427b229b7ea4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.