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

Conversation

ifreund
Copy link
Contributor

@ifreund ifreund commented Dec 2, 2024

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: #2363

Before: ALTABI set in the environment, with -o, or in pkg.conf only affected the ALTABI variable expanded while parsing repo conf files and nothing else.

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.

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
Copy link
Contributor Author

ifreund commented Dec 2, 2024

@kevemueller I did end up leaving the -x32 architectures out of the pkg_arch enum for now. In my opinion we should have tests for parsing all the architectures in that enum from at least one kind binary of on at least one OS.

It would be trivial to add them or any other currently unsupported architecture to that enum and the handful of functions converting to/from strings in pkg_abi.c if/when pkg gains full support for new architectures.

@ifreund
Copy link
Contributor Author

ifreund commented Dec 2, 2024

Also, this will conflict with #2372, I'm happy to do the rebase if you want to review/merge that one first @bapt. No rush though of course :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant