Replace devtools functionality by chrooti · Pull Request #1155 · aurutils/aurutils

chrooti · 2024-05-03T16:07:57Z

This is an attempt at solving (part of) #909.

This PR takes inspiration from the branch linked in the issue and a few arch projects to replace devtools with pure systemd-nspawn.

A few things:

--namcap and --checkpkg will need a separate implementation (should it be done in this PR?)
I've left a few XXX comments where I thought there could be improvements
I absolutely don't think this is ready for merge in the current state, I'm looking for pointers on what I've already done in order to know if something is fundamentally wrong/missing

AladW · 2024-05-09T23:25:36Z

The changes look good. To allow for a transition period, perhaps make it aur-chroot-standalone at first?

chrooti · 2024-05-10T12:08:15Z

With the last commit I think I've made the whole --margs mechanism redundant (at least in aur chroot). I haven't removed it yet for backward compatibility but it might be worth it (it's not a huge change).

I agree with renaming it into aur chroot standalone (I'll do it as a last step to simplify reviewing the diffs). I'd like your input on:

what to do with namcap/checkpkg? Should I keep them commented? Remove them entirely from aur chroot?
how would a user decide if aur-chroot or aur-chroot--standalone should be used from aur build? envvar?

AladW · 2024-05-13T20:54:34Z

namcap seems simple enough to implement, but AFAICT checkpkg has no relevance to an AUR repo setup. If someone wants it, they could make a request.
An envvar like AUR_BUILD_CHROOT could work, provided aur-chroot and aur-chroot-standalone expose the same interface on the command-line.

chrooti · 2024-05-14T11:03:25Z

Aside deciding how to deal with AUR_PACMAN_AUTH I think the PR is approaching its final shape.
I still think there are a lot of calls to systemd-nspawn that could be compressed into a single one by making a script (something like aur-chroot-helper), copying it in our chroot and running the various commands through it.

Example: instead of

    $AUR_PACMAN_AUTH systemd-nspawn -D "$directory"/root pacman-key --init
    $AUR_PACMAN_AUTH systemd-nspawn -D "$directory"/root pacman-key --populate

    # XXX: use localedef to generate locales directly (without nspawn)
    $AUR_PACMAN_AUTH systemd-nspawn -D "$directory"/root locale-gen

we could have

 $AUR_PACMAN_AUTH systemd-nspawn -D "$directory"/root /aur-chroot-helper setup

It could also simplify the namcap implementation uglyness I did to keep it in one line :D

Thoughts?

AladW · 2024-05-14T23:07:19Z

That sounds reasonable. In line with the other naming, I'd call it aur-chroot--helper (two hyphens at the end).

chrooti · 2024-05-16T22:29:07Z

I went for a slightly different solution inspired once again by makechrootpkg: declare -f as a poor man's jit. The mechanism is convoluted but the script code itself is still quite readable (IMHO).

I don't think there's anything left, I plan to restore the old chroot and rename this one into chroot--standalone pending your approval.

AladW · 2024-05-24T22:54:00Z

declare -f seems reasonable for now, especially when the script needs to live inside the chroot.

Earnestly · 2024-05-25T11:28:57Z

+
+    # allow makepkg_user to install packages (e.g. --syncdeps)
+    tee "$directory/root/etc/sudoers.d/makepkg_user" \
+        < <(printf '%q ALL = NOPASSWD: /usr/bin/pacman' "$makepkg_user") \


%q is a form of encoding that produces strings according to bash rather than sudo. The User grammar for sudo doesn't allow single quotes but uses similar mechanism for escaping problem characters:

Consider the sudoers manual:

User_List ::= User | User ',' User_List User ::= '!'* user name | '!'* #user-ID | '!'* %group | '!'* %#group-ID | '!'* +netgroup | '!'* %:nonunix_group | '!'* %:#nonunix_gid | '!'* User_Alias [...] A user name, user-ID, group, group-ID, netgroup, nonunix_group or nonunix_gid may be enclosed in double quotes to avoid the need for es‐ caping special characters. Alternately, special characters may be spec‐ ified in escaped hex mode, e.g., \x20 for space. When using double quotes, any prefix characters must be included inside the quotes. [...] Quotes around group names are optional. Unquoted strings must use a backslash (‘\’) to escape spaces and special characters. See “Other special characters and reserved words” for a list of characters that need to be escaped.

Here quotes are " which %q won't use, but it might use \. So I would be careful about mixing encoding schemes and making sure they accord to one another.

(N.B. sudoers doesn't seem to say how to handle " inside a quoted string, whether \" or closing, escaping and re-opening is needed ("\""))

thanks for taking the time to explain this!

AladW · 2024-05-26T12:43:14Z

Could you rebase this to one commit for aur-build changes, and another for the aur-chroot--standalone addition?

chrooti · 2024-05-26T22:44:44Z

I made one extra for aur-sync, hopefully it's alright.

chrooti · 2024-06-10T13:26:49Z

@AladW sorry for pinging but I see you have answered issues and opened a PR... are you still interested in merging this?

AladW · 2024-06-11T11:23:29Z

I'm still interested, I just got some deadlines that prevent me from looking into more complex PRs. Will give it a look in the next week or two.

AladW · 2024-08-09T18:49:47Z

 }

+auth_cmd() {
+    ${AUR_PACMAN_AUTH:-sudo --preserve-env} "$@"


I think a blanket --preserve-env is too coarse, e.g. the pacman commands to update the database do not require this (and this change would thus break existing sudoers rules.) I'd stick to ${AUR_PACMAN_AUTH:-sudo} and leave the --preserve-env stuff to aur-chroot.

I'm not follwoing: aur build is the one calling aur chroot and has some prior infrastructure to call various commands with an auth_cmd. Once I call it with sudo without --preserve-env I can't do anything to get the envvars back.
I see a few ways of doing what you suggest:

not calling aur chroot with sudo and having a separate logic to sudo only the relevant commands. This is what I did in my first implementation, and you suggested to change it. I'm fine with changing it again, however.

adding a separate auth_cmd_chroot function and the respective envvar inside aur build

Any third option I haven't thought about?

The discussion you linked mentions check_root in aur-chroot to reexec, but it seems that's no longer part of the script?

AladW · 2024-08-09T18:56:52Z

        --pkgver)
-            run_pkgver=1; makepkg_args+=(--noextract)
-            chroot_build_args+=(--margs '--holdver') ;;
+            run_pkgver=1; makepkg_args+=(--noextract) ;;


This change and those in lines 150, 166 will break the semantics of the existing aur-chroot, which does not allow specifying arbitrary makepkg arguments with --.

This was done to preserve the meaning of -- for end-of-options, e.g. to specify additional packages the user wants to install: see #807.

aur-chroot--standalone also uses --margs, though, but does not pass on makepkg_args to the actual makepkg command.

I thought you didn't particularly like the --margs interface with the comma as separator and it felt like a good moment for breaking changes, more so because there is still --create/--update to add more packages to the environment.
Just put a thumbs up and I'll do what you asked, no biggie.

The challenge is when combining --build with --create / --update. Then the additional arguments could be both makepkg arguments, and package names. With the end-of-options approach, to pass on additional packages to be installed in the chroot you'd need an additional flag like --extra-packages. Since that approach gives some special meaning for -- when --margs doesn't, I think --margs is preferable overall.

AladW · 2024-08-09T18:57:07Z

            chroot_args+=(--temp) ;;
        # makepkg options (common)
        -A|--ignorearch|--ignore-arch)
-            chroot_build_args+=(--ignorearch)


AladW · 2024-08-09T18:57:14Z

        -L|--log)
            makepkg_args+=(--log) ;;
        --nocheck|--no-check)
-            chroot_build_args+=(--nocheck)


AladW · 2024-08-09T18:57:38Z

+# Therefore they do not need to be updated a second time with makepkg
+# --verifysource in makechrootpkg.
+if (( chroot )) && (( run_pkgver )); then
+    makepkg_args+=(--holdver)


--margs '--holdver', see L129. Alternatively aur-chroot could include a --holdver flag.

AladW · 2024-08-09T19:05:11Z

+    # # XXX: allow setting machine-id from host (and provide default) to enable .nspawn file usage
+    systemd-firstboot --root="$directory"/root --copy-locale --copy-timezone --setup-machine-id
+
+    pacman -Sy "${base_packages[@]}" \


I would put the pacman -Sy arguments on one line here

AladW · 2024-08-09T19:08:32Z

+    )
+
+    # create makepkg_user
+    systemd-sysusers --root "$directory"/root --replace=/usr/lib/sysusers.d/build.conf - \


Maybe it makes sense to use a second helper for the --build step, or overwrite (>) the contents. As it stands, the commands added for the --create and --update steps would be run a second time when both --build and --create / --update are specified.

AladW · 2024-08-09T19:12:44Z

+
+    # create makepkg_user
+    systemd-sysusers --root "$directory"/root --replace=/usr/lib/sysusers.d/build.conf - \
+        < <(printf 'u %s %s "makepkg user" /build /bin/bash\n' "$makepkg_user" "$AUR_CHROOT_UID")


AUR_CHROOT_UID seems like a good idea, but as written this relies on it being set by the caller, i.e. aur-build. When running aur-chroot on its own, the value is empty. I don't know if this is handled implicitly by systemd-sysusers, or if some explicit value should be set.

as long as we call aur chroot with sudo externally this needs to be passed, and I think the safest approach is just to raise if this variable doesn't exist. What do you think?

Let's return to this then after solving #1155 (review)

AladW · 2024-08-09T19:14:45Z

+        < <(printf 'u %s %s "makepkg user" /build /bin/bash\n' "$makepkg_user" "$AUR_CHROOT_UID")
+
+    # allow makepkg_user to install packages (e.g. --syncdeps)
+    tee "$directory/root/etc/sudoers.d/makepkg_user" \


cat also expands shell arguments:

cat <<EOF >"$directory/root/etc/sudoers.d/makepkg_user" $makepkg_user ALL = NOPASSWD: /usr/bin/pacman EOF

I just disliked the weird indentation and it seemed a useless use of cat, but it's a matter of taste. I'll change it.

AladW · 2024-08-09T19:15:30Z

+        1> /dev/null
+    chmod 440 "$directory/root/etc/sudoers.d/makepkg_user"
+
+    printf 'makepkg %s || exit\n' "${*@Q}" >> "$directory"/root/aur-chroot--helper


${makepkg_args[@]} instead of ${*@Q}

AladW · 2024-08-19T15:03:22Z

My responses are still delayed, sorry about that. I still hope to see this in v20.

AladW · 2024-08-29T20:43:17Z

Superseded by #1179.

AladW reviewed May 8, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

Comment thread lib/aur-chroot Outdated

Comment thread lib/aur-chroot

Comment thread lib/aur-chroot Outdated

Comment thread lib/aur-chroot Outdated

AladW added enhancement devtools labels May 8, 2024

AladW reviewed May 10, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

AladW mentioned this pull request May 13, 2024

build, chroot: add -I <pkg>, and also aur build --cargs just in case #1125

Closed

Earnestly reviewed May 17, 2024

View reviewed changes

Comment thread lib/aur-build Outdated

Earnestly reviewed May 17, 2024

View reviewed changes

Comment thread lib/aur-build Outdated

Earnestly reviewed May 17, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

AladW mentioned this pull request May 24, 2024

Is the warning about makechrootpkg still relevant #1145

Closed

Earnestly reviewed May 25, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

Earnestly reviewed May 25, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

Earnestly reviewed May 25, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

Earnestly reviewed May 25, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

Earnestly reviewed May 25, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

Earnestly reviewed May 25, 2024

View reviewed changes

Comment thread lib/aur-chroot Outdated

Earnestly reviewed May 25, 2024

View reviewed changes

AladW reviewed May 26, 2024

View reviewed changes

Comment thread lib/aur-build Outdated

AladW reviewed May 26, 2024

View reviewed changes

Comment thread makepkg/PKGBUILD

AladW reviewed May 26, 2024

View reviewed changes

Comment thread man1/aur-chroot.1

AladW reviewed May 26, 2024

View reviewed changes

Comment thread completions/zsh/_aur

chrooti added 2 commits May 27, 2024 00:43

implement standalone aur-chroot

d30ea96

adapt aur-build to work with standalone aur-chroot

b749efb

track possible deprecated flag in aur-sync

3ce08a3

chrooti force-pushed the standalone-aur-chroot branch from a74512b to 3ce08a3 Compare May 26, 2024 22:44

AladW added this to the 20 milestone Jul 23, 2024

AladW requested changes Aug 9, 2024

View reviewed changes

chrooti closed this Aug 16, 2024

AladW reopened this Aug 19, 2024

AladW mentioned this pull request Aug 29, 2024

chroot: replace devtools functionality (part II) #1179

Open

AladW closed this Aug 29, 2024

Conversation

chrooti commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AladW commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

chrooti commented May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AladW commented May 13, 2024

Uh oh!

chrooti commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AladW commented May 14, 2024

Uh oh!

chrooti commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AladW commented May 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AladW commented May 26, 2024

Uh oh!

chrooti commented May 26, 2024

Uh oh!

chrooti commented Jun 10, 2024

Uh oh!

AladW commented Jun 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AladW Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

chrooti commented May 3, 2024 •

edited

Loading

AladW commented May 9, 2024 •

edited

Loading

chrooti commented May 10, 2024 •

edited

Loading

chrooti commented May 14, 2024 •

edited

Loading

chrooti commented May 16, 2024 •

edited

Loading

AladW Aug 9, 2024 •

edited

Loading

AladW Aug 9, 2024 •

edited

Loading