Strongly type chroot suffix

Rename the "Suffix" type to "Chroot" and have it represent the full path (fetching the work dir from a new global variable)

sounds good

Use Pythons "pathlib" object oriented Path type and provide a Chroot.path() function. This will let us write paths like (chroot.path() / "etc/apk/repositories")

why is this better than the shorter chroot.path("/etc/apk/repositories")? (genuinely curious, I guess you have a good reason)

We can extend the chroot type in the future to abstract common operations and maybe encapsulate additional state

I like that

We adopt the pmb.core namespace, making it strongly typed and "stable" (if not public). The "args" variable would be banned from here, and we would basically try to use it for the parts of pmbootstrap that might be useful externally.

good idea!

@calebccff: Could you make some before and after examples for what you proposed? Then we might discuss it a bit more, and then I'd suggest transforming the whole thing as in !2252 (merged) (but with the new ideas discussed here).

By Oliver Smith on 2024-03-12T20:26:42

why is this better than the shorter chroot.path("/etc/apk/repositories")?

I don't think one is inherently better, but I think adopting pathlib.Path is overall more inline with what we're doing in pmb, and this is the "pythonic" way of doing it, so I'd appeal to that I guess. There's maybe some argument to be made about the semantics of my_path.resolve() / "my/file" compared to my_path("my/file").resolve() but honestly I haven't thought it through.

We could also make it a property so it would be chroot.path instead of chroot.path(), that might be more idiomatic.

Could you make some before and after examples for what you proposed? Then we might discuss it a bit more, and then I'd suggest transforming the whole thing

For sure. I did some more brainstorming on this and I think depending on how long this takes, and especially given the added complexity of converting all our strings to real Path objects, it might make sense to do a feature freeze in pmbootstrap and have all new features target the reworked version. I don't know if it will come to that but just throwing the idea out there.

By Caleb Connolly on 2024-03-12T20:26:42

Also, from https://lists.sr.ht/~postmarketos/pmbootstrap-devel/%3C20230629-suffix-type-hint-v1-0-e92802ae20a8%40postmarketos.org%3E#%3CCU1D0K6SVET8.4EI0XUU9957H@pm-mail-aerc%3E, if we touch all related code anyway it might be a good idea to rename buildroot_x86_64 to build_x86_64 etc.

By Oliver Smith on 2024-03-12T20:28:12

I would rather do this as a followup change so we can minimise breakage, but I'm on board with this. If we do this properly then this should be just changing a single string literal (.. and writing some migration code)!

By Caleb Connolly on 2024-03-13T18:05:32

Here's an example of a function updated to my proposal. I realised our Chroot class could overload the / operator so we can construct paths with chroot / "etc/apk/repositories rather than like chroot.path / "etc/apk/repositories". This only works if the chroot is the first part of the path, but I can't think of an example when that would be the case, as the chroot object represents the absolute path to the chroot dir.

EDIT: ah it turns out you can overload both normal div / by implementing __truediv__() but you can also implement __rtruediv__() where you implement the reverse. So we can have / work no matter where the chroot is in the list, e.g. Path("/beep") / Chroot.native() results in Path("/beep/home/cas/.local/var/pmbootstrap/chroot_native").

By Caleb Connolly on 2024-03-12T22:46:29

The example you provided above looks pretty nice, I like that we can just write chroot and combine it with slashes like that. It's short and does the strong typing, perfect!

I've attempted to review !2252 (merged) now, but TBH I think this does too many things at once. I think it would be good to discuss the major changes that are made with examples first, because then it is easier to review and iterate (like done above with the Chroot class).

And we should try to stick to one logical change per commit, e.g. "treewide: add a Chroot type and adopt pathlib.Path" patch could at least be split into:

move args.work to config.work
replace args with PmbArgs (or maybe omit this for now? see below)
Add and use the chroot class
(possibly more, I didn't read through the entire commit)

Regarding PmbArgs: I'm not sure if we should make it part of pmb.core. What I want to get rid of eventually, is the pattern that it gets passed to almost every function in pmbootstrap (#1879). It is basically a global variable, and should just be made one, I did so in more recent code bases (e.g. ttq). I'm not sure how to do it elegantly with the more strongly typed ideas... but I want to avoid that if we make it part of the public API, we cannot get rid of passing it to every function and/or make related code more complex?

So to move forward:

Could you make a list of commits with one logical change per commit (without actually implementing them for now)
Add examples for each major change in the commit

Thank you very much for working on this!

By Oliver Smith on 2024-03-13T23:07:25

Ah, thanks for having a look!

but TBH I think this does too many things at once.

I get that, it's one of those things where I have the energy for it and I really don't know for how long that will last, so I want to just do as much as possible while I have the motivation.

And we should try to stick to one logical change per commit

I can definitely split this up, probably like:

move args.work to config.work
pmb.core: introduce a type-annotated Chroot class
- The Chroot class is meant to be an ephemeral representation of a chroot directory, as well as providing some light utility methods. It doesn't not maintain any state. It can be de-structured into a Path with the / operator.
- This would only add the pmb.core directory and Chroot class, no other changes
pmb.core: type hint args
- This is just meant to help with mypy.
treewide: adopt Chroot and Path types
- Replace the "suffix" strings with a real type, use Path objects instead of strings.
- Fly-by cleanups of many functions with type hinting.

Regarding PmbArgs: I'm not sure if we should make it part of pmb.core

I can move the definition elsewhere. The reason I'm adding it in the first place is because it makes mypy actually useful when working with args. I don't expect it to be 100% accurate, and I definitely don't it to stick around.

It shouldn't be part of the API no, I just threw it in there (I also wouldn't declare pmb.core public yet, we should give ourselves some time to iterate on it. I think there's a lot of places in pmbootstrap where we encapsulate some idiomatic behaviour into the Chroot type).

Moving forward, I would want to look into rewriting args to use something like the TAP library, this gives us proper type hinted argument parsing and just generally seems like a big improvement. This will then make it much easier to clean up args and split it up.

I don't think we should make args global, the biggest reason why is that (at least until we can properly reason about it) this would make it less obvious if a function uses it or not.

I would probably want to go at it like this:

Split out readonly stuff like pmb.config and use global accessors (a Config type which can be (De)serialised to ini would be nice).
Convert ~trivial functions that just take "args" to instead take the actual data they need (provided it's less than 5 arguments or something).
Convert more complicated processes (like pmbootstrap build) to loosely follow the builder pattern, just that instead of constructing an object we're constructing a package. I think this would then naturally lead us towards more abstractions.

I'm wary to lean too far into OOP stuff, but I think data classes with very minimal validation/utility methods are a good way to maintain invariants (like the Chroot class does) and give us more readable code.

As an example of what this could look like (very much a strawman, just what I came up with after messing with this for a bit):

With internals like this exposed via an API, one example of a tool I'd love to have would be a meson wrapper that builds Alpine packages. You can use meson introspect --dependencies to get a list of deps with pkgconfig compatible data, and meson introspect --buildoptions to get the build options. Then it's easy to generate an APKBUILD (with pc: depends), spin up a buildroot and build the package. It also should be pretty easy to make meson projects cross compile properly, so we could do that too.

source code for reference


def build_packages(*, session: Any, packages: List[str]):
    pass
class Arch(enum.StrEnum):
NATIVE = os.uname().machine
AARCH64 = "aarch64"
class ApkbuildOpt(enum.StrEnum):
STRICT = "pmb:strict"
NO_CHECK = "!check"
# ...
class Apkbuild():
def init(self, path: Path):
self.__path = path
# deserialise magic here

@property
def options(self) -> List[ApkbuildOpt]:
    return []


class Package(Namespace):
"""
A representation of a package, backed by an APKBUILD and/or APKINDEX entry.
"""
__apkindex: Optional[Dict[str, Any]]
__apkbuild: Optional[Apkbuild]
src: Optional[Path] # build --src

def __init__(self, name: str, aport: Optional[Path] = None, src: Optional[Path] = None):
    self.__name = name
    self.__aport = aport or pmb.helpers.pmaports.find(PmbArgs(), name)
    self.src = src

    assert not self.__aport.is_absolute(), "aport must be relative to the pmaports directory"
    
    # Load apkindex/apkbuild and deserialise them here. apkbuild properties take precedence.

def version(self, binary: bool = False) -> str:
    return ""

@property
def base_package(self) -> 'Package':
    if self.is_subpackage:
        return self.parent
    return self


class BuildSession(Namespace):
"""
A container for a "build session", caches packages that it has built before and skips
rebuilding them.
"""
def init(self, arch: Arch):
pass

def reset(self):
    """Clear cache"""
    pass


def main():
# Build cross compiler
build_packages(
session = BuildSession(Arch.NATIVE),
packages = [
Package("gcc"),
],
)

builder = BuildSession(Arch.AARCH64)
builder.strict = True
builder.force = True

build_packages(
    session = builder,
    packages = [
        Package("hello-world", src=Path("./hello-world")),
        Package("postmarketos-base"),
    ],
)

By Caleb Connolly on 2024-03-13T23:19:27

Sounds interesting, and I like the idea of having a meson wrapper!

In order to not block systemd stuff however, I think we should wait until #2328 (closed) is done before doing any big refactoring. I do want to make these changes though (maybe after discussing the details a bit more), I think they will make pmbootstrap much better!

By Oliver Smith on 2024-03-17T16:53:55

added to epic &4

By Caleb Connolly on 2024-03-14T17:50:03

mentioned in merge request !2252 (merged)

By Oliver Smith on 2024-03-27T20:26:31

changed milestone to %The tooling rework

mentioned in issue #2464

Strongly type chroot suffix

Child items 0

Activity