Paths

Table of Contentst

Refactor pkg bootstrap to fetch compression types
Needs ReviewPublic
Actions

Authored by zac_freebsdfoundation.org on Sep 21 2020, 6:18 PM.

Details

Reviewers

Summary

Add support for fetching different types of compression of the pkg
package. It first tries to fetch pkg.txz (the original file), and if
unsuccessful, will try to fetch pkg.tzst, pkg.tbz, pkg.tgz, and pkg.tar
before failing.

Since the package extractor is compression-agnostic, only the code that
fetches the package was necessary to refactor.

Test Plan

Build the package binary, force a pkg bootstrap and confirm that it has
the same behaviour as package from upstream. Remove or rename the first
extension in the extensions array (".txz"), confirm with GDB that the
other package extensions are attempted to be fetched, and result in
desired behaviour once each fetch fails.

Moving the .txz extension (the compression currently being used for the
package) to a different position in the extensions array, use GDB to
confirm the program attempts to fetch the extensions in the correct
order before successfully fetching the package with the .txz extension.

Use Valgrind to confirm that no memory additional memory was leaked
from this code change.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33699
Build 30935: arc lint + arc unit

Event Timeline

zac_freebsdfoundation.org created this revision.Sep 21 2020, 6:18 PM

Herald added a reviewer: bdrewery. · View Herald TranscriptSep 21 2020, 6:18 PM

Harbormaster completed remote builds in B33699: Diff 77313.Sep 21 2020, 6:18 PM

zac_freebsdfoundation.org requested review of this revision.Sep 21 2020, 6:18 PM

emaste added a subscriber: emaste.Sep 21 2020, 6:35 PM

emaste added subscribers: manu, bapt.Sep 21 2020, 6:38 PM

emaste added inline comments.

usr.sbin/pkg/pkg.c
871	FreeBSD style is a 4 spaces for 2nd-level indents. clang-format should be able to help here, have a look at the instructions for clang-formatting a git patch at https://clang.llvm.org/docs/ClangFormat.html and see how it works, and then put those instructions on https://wiki.freebsd.org/Phabricator

Update formatting with clang-format

Harbormaster completed remote builds in B33702: Diff 77320.Sep 21 2020, 8:22 PM

Revert "Update formatting with clang-format"
Update formatting with git clang-format

Harbormaster completed remote builds in B33711: Diff 77338.Sep 21 2020, 11:57 PM

emaste added a subscriber: kevans.Sep 22 2020, 2:33 PM

kevans added inline comments.Sep 22 2020, 5:03 PM

usr.sbin/pkg/pkg.c
833	I'd be tempted to move this out to the global scope, make it static, drop the NULL terminator, then...
866–868	... loop here from 0 to `nitems(extension)` (macro provided by `sys/param.h`)
870–871	All paths through the oop after this either goto fetchfail or cleanup, so you could invert this condition and `break` out of the loop, dropping the rest of the loop's body out of it. Do we suspect that we could hit a failure mode where, e.g., pkg.txz exists but the rest of this won't line up and we need to try a different archive type?

emaste added inline comments.Sep 22 2020, 6:07 PM

usr.sbin/pkg/pkg.c
833	in discussion with @bapt we want to add also the `.bsd` extension, it's not used yet but he wants to use it as the generic extension for packages

Move array of package extensions to global scope

usr.sbin/pkg/pkg.c
870–871	I believe that the desired behavior for when pkg.txz is fetched but reaches a failure state for whatever reason to attempt to fetch the next file from the extensions array to attempt recovery.

zac_freebsdfoundation.org marked 3 inline comments as done.Sep 23 2020, 2:42 AM

Harbormaster completed remote builds in B33746: Diff 77397.Sep 23 2020, 2:53 AM

@bapt and I had an out-of-band discussion about this, and I think we'd like to see a slightly different approach taken -- there's been somewhat of a trend to move away from pkg tooling having to know/guess compression type that a repo uses before you have sufficient information to *know*. See, e.g., the 'recent' (probably not that recent anymore) transition to just using meta.conf instead of compressed meta.txz.

That said, the 'ideal' solution we discussed doesn't seem to be much harder at all from a cursory glance: poudriere should just create a compression format agnostic symlink in /Latest to the pkg.${COMPRESSION_FORMAT} format. pkg-bootstrap should then be taught to try that first and fallback to pkg.txz if that doesn't work.

The .{pubkeysig,sig} will also need a symlink since it has the (and should probably retain the) compression format embedded in the name -- I think it makes sense to just create a pkg.signature symlink to simplify the fetching logic and switch on signature_type for verify_signature/verify_pubsignature.

I think this captured what bapt and I had discussed, at least -- questions / comments / concerns?

what about using pkg.bsd as the format-agnostic symlink name?

pkg.bsd -> pkg.txz

bapt suggests pkg.latest or pkg.bootstrap as the agnostic symlink name

on IRC @bapt suggests latest/pkg.bootstrap as the symlink, full URL pkg.freebsd.org/FreeBSD:13:amd64/latest/pkg.bootstrap
Note that this removes the Latest from the URL currently used for pkg.txz.

18:16 <@bapt> so all in all to be backward compatible you can just create a new symlink in bsd.port.mk
18:17 <@bapt> and coordinate with antoine so the hook so sign now also sign the pkg.bootstrap

Revision Contents
Changeset List

Path

Size

usr.sbin/

pkg/

pkg.c

88 lines

Diff 77313

View Options

Refactor pkg bootstrap to fetch compression typesNeeds ReviewPublicActions