Skip to content
This repository has been archived by the owner. It is now read-only.

Implement zapping of apk packages with ver different than aports (466) #474

Merged
merged 1 commit into from Aug 28, 2017

Conversation

Projects
None yet
2 participants
@craftyguy
Copy link
Member

commented Aug 26, 2017

This adds a new option to zap: -b / --mismatch-bins

When set, any binary apks in the work directory packages folder will be
removed if their version differs from the version in the relevant
APKBUILD in aports.

@craftyguy craftyguy self-assigned this Aug 26, 2017

@craftyguy craftyguy requested a review from ollieparanoid Aug 26, 2017

@craftyguy craftyguy force-pushed the feature/466_zap_mismatch_binaries branch from f72c3c7 to 33b8b49 Aug 26, 2017

@ollieparanoid
Copy link
Member

left a comment

Thank you for this useful contribution! I've added a few comments.

@@ -44,3 +45,23 @@ def zap(args, confirm=True, packages=False, http=False):
for match in matches:
if not confirm or pmb.helpers.cli.confirm(args, "Remove " + match + "?"):
pmb.helpers.run.root(args, ["rm", "-rf", match])

if mismatch_bins:

This comment has been minimized.

Copy link
@ollieparanoid

ollieparanoid Aug 26, 2017

Member

I recommend moving the whole block into a new function in the same file.

This comment has been minimized.

Copy link
@craftyguy

craftyguy Aug 28, 2017

Author Member

done!

aport_version = apkbuild["pkgver"] + "-r" + apkbuild["pkgrel"]
# Clear out any binary apks that do not match what is in aports
if pmb.parse.version.compare(bin_version, aport_version) and os.path.exists(bin_apk_path):
logging.info("Removing mismatched APK for " + bin_pkgname + "-" + bin_version)

This comment has been minimized.

Copy link
@ollieparanoid

ollieparanoid Aug 26, 2017

Member
  • For consistency with other messages, I would write Remove instead of Removing.
  • How about specifying the arch, that gets removed? Consider noarch packages, which would print the same line three times (x86_64, armhf, aarch64).
  • How about specifying the version found in the aports folder?

With the bullet points above I would recommend:

logging.info("Remove mismatched binary package (aports version: " + aport_version + "): " + arch + "/" + bin_pkgname + "-" + bin_version + ".apk")

This comment has been minimized.

Copy link
@craftyguy

craftyguy Aug 28, 2017

Author Member

I will point out that "removing" is correct grammar in this instance, but I've changed it to "removing" nonetheless!

@@ -151,6 +151,9 @@ def arguments():
" the precious, self-compiled packages")
zap.add_argument("-hc", "--http", action="store_true", help="also delete http"
"cache")
zap.add_argument("-b", "--mismatch-bins", action="store_true", help="also delete"

This comment has been minimized.

Copy link
@ollieparanoid

ollieparanoid Aug 26, 2017

Member

-m would seem more intuitive to me, is there a reason why you chose -b? (We could also add both versions I think.)

This comment has been minimized.

Copy link
@craftyguy

craftyguy Aug 28, 2017

Author Member

I used -b for "binaries"! but yes, if the whole theme is "mismatched-binaries" then your suggestion makes sense.

bin_pkgname = bin_apks[bin_apk]["pkgname"]
bin_version = bin_apks[bin_apk]["version"]
bin_apk_path = arch_pkg_path + "/" + bin_pkgname + "-" + bin_version + ".apk"
aport = pmb.build.other.find_aport(args, bin_pkgname)

This comment has been minimized.

Copy link
@ollieparanoid

ollieparanoid Aug 26, 2017

Member

For me it failed here with:

ERROR: Could not find aport for package: weston-shell-fullscreen

This isn't because of this PR, but because our current APKBUILD parser can't resolve the subpackages correctly from the weston package (if you look at the APKBUILD, they are quite complex!). But it should not crash here in my opinion.
How about:

  • pmb.build.other.find_aport(args, bin_pkgname, False) setting the must_exist to false
  • adding these lines (advantage is, that you catch the error and show a message and do not need to intend the following code):
if not aport:
    logging.warning("WARNING: Could not resolve aport for package " + bin_apk_path)
    continue

@craftyguy craftyguy force-pushed the feature/466_zap_mismatch_binaries branch 2 times, most recently from 93f4fe2 to b11473d Aug 28, 2017

Implement zapping of apk packages with ver different than aports (466)
This adds a new option to `zap`: `-b / --mismatch-bins`

When set, any binary apks in the work directory packages folder will be
removed if their version differs from the version in the relevant
APKBUILD in aports.

@craftyguy craftyguy force-pushed the feature/466_zap_mismatch_binaries branch from b11473d to c92fac8 Aug 28, 2017

@craftyguy

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

@ollieparanoid requested changes have been made

Also, should I remove the "dont_merge_pr" label after making changes requested for a review, or just leave it?

@ollieparanoid

This comment has been minimized.

Copy link
Member

commented Aug 28, 2017

Please remove the label when there's nothing left to discuss/change.

Regarding the incorrect grammar with "remove" instead of "removing": You are right, that is a bit strange. If you prefer it, please open a ticket so we can change it across the whole codebase ("(native) install ..." becomes "(native) installing ...") etc.

I have tested your PR again and everything is working as expected.
Thanks again for this contribution \o/

@ollieparanoid ollieparanoid merged commit a9e5b36 into master Aug 28, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ollieparanoid ollieparanoid deleted the feature/466_zap_mismatch_binaries branch Aug 28, 2017

PureTryOut added a commit that referenced this pull request Feb 21, 2018

Implement zapping of apk packages with ver different than aports (466) (
#474)

This adds a new option to `zap`: `-m / --mismatch-bins`

When set, any binary apks in the work directory packages folder will be
removed if their version differs from the version in the relevant
APKBUILD in aports.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.