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

libpkg: rework internal ABI handling, fix bugs #2376

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Commits on Nov 25, 2024

  1. libpkg: rework internal ABI handling, fix bugs

    First of all, apologies for the scope creep on this one. I didn't find a
    good way to break it up into smaller patches. Writing this was a bit
    like pulling a thread that caused the whole knit sweater to unravel.
    
    This is in my opinion a much needed refactor that will make future work
    on further architecture-specific code such as the shlib tracking
    features significantly easier.
    
    The main guiding principle of this patch is to create a single source of
    truth for as many ABI related details as possible. I'll summarize the
    changes with a series of before/after comparisons:
    
    Before: Setting -o ABI=... on the command line but not -o OSVERSION=...
    led to inconsistency between ABI and OSVERSION.
    
    After: Setting ABI requires setting OSVERSION as well (and vice versa).
    (Note: OSVERSION is FreeBSD-specific)
    Related: freebsd#2363
    
    Before: ALTABI set in the environment, with -o, or in pkg.conf only
    affected the ALTABI variable expanded while parsing repo conf files.
    
    After: setting ALTABI is completely ignored and a notice is printed if
    it is set. The ALTABI formatted string derived from the automatically
    detected or configured ABI is still printed by `pkg config ALTABI`.
    
    Before: Setting ABI/OSVERSION in pkg.conf would not affect the variable
    expansions of ABI, OSVERSION, ARCH, VERSION_MAJOR, etc.  in pkg.conf.
    This was a pretty big footgun IMO, though I don't know of any examples
    of people getting bitten by this. I don't think setting ABI in pkg.conf
    is a normal thing to do.
    
    After: Setting ABI, ALTABI, and OSVERSION in pkg.conf is not allowed.
    This is consistent with ABI_FILE, which was never able to be set using
    pkg.conf. It is still possible to set ABI and OSVERSION on the command
    line and e.g. `pkg config ABI` still works. This is the simplest
    solution I see to this problem, but it is a breaking change.
    
    Before: Parsing of user-provided ABI strings was very lax. Pretty much
    anything was accepted without giving any error as long is it contained
    at least one colon.  The full affects of providing such invalid ABI
    strings to pkg are not well understood, I think it can safely be assumed
    that interesting bugs would result.
    
    After: Parsing of user-provided ABI strings is very strict. Only a well
    defined set of supported OS names and architectures are accepted.
    Missing or extra components cause a hard error.
    
    Before: The global `ctx.oi` os_info struct stored multiple copies of the
    same information that could get out of sync with each other. For
    example, the version was stored in 4 or 5 different places in the same
    struct.
    
    After: The global `ctx.abi` struct contains no redundant information.
    
    Before: Most code dealing with ABI information worked with stringly
    typed data.  Conversion to/from strings and parsing details were strewn
    across the codebase.
    
    After: Most code dealing with ABI information uses the new `struct
    pkg_abi` representation with enums for OS and architecture and integers
    for the version.  All parsing is unified in pkg_abi.c and multiple minor
    inconsistencies between previously duplicated code are resolved.
    
    Closes:		freebsd#2363
    Sponsored by:	The FreeBSD Foundation
    ifreund committed Nov 25, 2024
    Configuration menu
    Copy the full SHA
    945a844 View commit details
    Browse the repository at this point in the history