Try to make it easier to maintain for packages (and distros) #471

paulo-ferraz-oliveira · 2023-10-04T02:26:25Z

Description

The initial goal was to ease maintenance for "missing package" -related messages. Conceptually, this code is not very different from the previous one except that:

distros are dynamic (their ID is obtained from os-releases - with fallback) and then used to either "error" out or find the list of "probe-able" packages
the probes are, as before, unique for all packages, but since this is an evaluated command we can play with more complex probes
we now have distinct distros (this might actually, in a first release, reduce the number of targets we can use - since "package manager" is much more generic than "distro"), so we can also have e.g. multiple package managers per distro and allow for different package names (Fedora's g++ is gcc-c++ in RHEL - for the same package manager)
there's an environment variable to silence the output

Further considerations

I had hacked the think locally to test with darwin and e.g. rebar3 but reverted those changes before pushing.

~~At the same time, ⚠️ I haven't tested in actual targets, but hope to do so with a few different Docker images.~~

I'm testing now:

Also take a look at

Closes #439.
Closes #431.

I have performed a self-review of my changes
I have read and understood the contributing guidelines

kerl

paulo-ferraz-oliveira · 2023-10-05T13:23:50Z

@jadeallenx, I'm starting tests, with Alpine as the first candidate. Out-of-the-box it doesn't have curl installed (at least not the Docker image I'm using). I know the README mentions Kerl depends on git and curl but it also depends on a bunch of other stuff, potentially existing in all base distros, these days, like grep, sed, cut, awk, find, cat, chmod, echo, printf, tail (potentially more). I'm wondering if we should test for this list, or simply update the README to mention more than the 2 commands, there, or say we'll outright crash if a command isn't present. Thoughts?

paulo-ferraz-oliveira · 2023-10-05T14:02:45Z

Might be worth mentioning all logs have a [packages] prefix.

Idea 💡

If all logs add a prefix, too, we could even think of a way to filter out undesirable ones, e.g.

ns="packages"
msg="Unable to determine"
log $ns $msg

and then inside log we could search for filters in a e.g. KERL_LOG_FILTERS= variable we'd expose.

(food for thought for another pull request 😄)

paulo-ferraz-oliveira · 2023-10-05T14:51:24Z

Example logfile output

[packages] Found "Alpine Linux v3.18" with ID alpine
[packages] Release ID found in /etc/os-release: alpine
[packages] Found package declarations for alpine. Linux flavor is known.
[packages] Acting on known Linux distro alpine
[packages] Probe success for openssl-dev (distro: alpine)!
[packages] Probe success for make (distro: alpine)!
[packages] Probe success for autoconf (distro: alpine)!
[packages] Probe success for ncurses-dev (distro: alpine)!
[packages] Probe success for gcc (distro: alpine)!
[packages] Probe success for g++ (distro: alpine)!

kerl

paulo-ferraz-oliveira · 2023-10-05T17:21:07Z

Can't get alpine to build properly. Keep getting

make[2]: *** [aarch64-unknown-linux-musl/Makefile:704: aarch64-unknown-linux-musl/erl_db_insert_list.ycf.h] Killed

I looked at https://github.com/bitwalker/alpine-erlang/blob/master/Dockerfile, for example, and don't think I'm doing anything different from what they're doing. I installed all the packages that image install, and am passing --build as a configuration option.

Edit: this was Docker configuration, mostly likely related to limits I imposed on RAM consumption.

kerl

The _apk, _rpm and _dpkg functions do an echo (they build a string) That string is used to both output what failed, but also be evaluated to probe for a given package install

In RHEL the value is "rhel", with the quote (and since we're not sourcing the file, but parsing it, we need to do some extra work)

We're already, out of the functions, doing the /dev/null + 2>&1 dance

paulo-ferraz-oliveira · 2023-10-08T20:12:25Z

Idea 💡

This is maybe outside the scope of kerl but how would you feel having the "package" checker output a command to install the missing packages? (this would, of course, be a different pr, to avoid "idea" creep).

This appears to require zypper to be done, so can probably wait for community contribution to happen

paulo-ferraz-oliveira · 2023-10-08T20:53:36Z

As a bonus, and to check if it'd be easy to extend to e.g. Arch Linux, using pacman I'm also testing a local version for that. If I got it right, I only need a probe function and some package names...

jadeallenx · 2023-10-09T01:07:47Z

Idea 💡

This is maybe outside the scope of kerl but how would you feel having the "package" checker output a command to install the missing packages? (this would, of course, be a different pr, to avoid "idea" creep).

Hm. This sounds like a lot of maintenance work. To be honest, I don't really want kerl to do any package detection but when builds break, it is typically because build prerequisites are not available and then people open tickets that "kerl" is broken somehow. So all of this effort is to help people figure out that it's not kerl's fault lol - not sure we need to help people that much?

What benefit do you see if this were an option?

kerl

jadeallenx · 2023-10-09T01:12:58Z

kerl

+    kpp=$(eval echo \$_KPP_"${os_release_id}"_pkgs)
+    if [ -n "${kpp}" ]; then
+        echo "[packages] Found package declarations for ${os_release_id}. Linux flavor is known." >>"$LOGFILE"
+        echo "[packages] Acting on known Linux distro ${os_release_id}" >>"$LOGFILE"


Seems like you could combine these messages.

[packages] Your linux seems to be ${os_release_id}. ???

Yeah, I guess so. A previous implementation had them separated into different code paths, but one I joined it into this single if didn't take care of it. 👍

7fa76c3, then f6deb2b to not have it called "flavor" sometimes and "distro" some other times 😄

Yeah we should drop them or stick to one consistent word :)

kerl

For readability purposes

paulo-ferraz-oliveira · 2023-10-09T21:02:40Z

What benefit do you see if this were an option?

Apart from copy-paste ease, none, so I agree we shouldn't do it.

paulo-ferraz-oliveira · 2023-10-09T21:07:00Z

I did the request changes. Based on our prior discussions for this stuff, do you think the end result is close to what you expected? Or do you think it'll be a lot of maintenance work to keep adding distros/packages? (I do understand that kerl could do without all this, but it's also - as you marginally state - part of its value as a tool)

jadeallenx · 2023-10-09T21:07:29Z

should test for this list, or simply update the README to mention more than the 2 commands, there, or say we'll outright crash if a command isn't present. Thoughts?

We've survived this long noting only git and curl, I think we can rely on those other utilities being part of the base install tbqh

jadeallenx

lgtm

jadeallenx · 2023-10-09T21:13:17Z

OK don't kill me but seeing all of the

echo "${msg}" >> $LOGFILE

makes me think we should make a function called log() which takes a string and prepends the date and writes it to a filehandle

Thoughts?

jadeallenx · 2023-10-09T21:15:58Z

Eh. On second thought maybe we should open a new issue/PR for log emission cleanup/generalization - because stderr() is a specialized log emitter than writes to stderr and not a disk file.

jadeallenx · 2023-10-09T21:22:09Z

Might be worth mentioning all logs have a [packages] prefix.

Idea 💡

If all logs add a prefix, too, we could even think of a way to filter out undesirable ones, e.g.
ns="packages"
msg="Unable to determine"
log $ns $msg
and then inside log we could search for filters in a e.g. KERL_LOG_FILTERS= variable we'd expose.

(food for thought for another pull request 😄)

lol great minds - yeah we should definitely open an issue to track this - don't think this work needs to delay a release tho?

paulo-ferraz-oliveira · 2023-10-09T23:11:55Z

There we go: #475

paulo-ferraz-oliveira · 2023-10-09T23:15:31Z

I'm squash-merging this in preparation for the release. I think we can hold off on it for a few more days (some consumers use the main branch) to see if this surfaces any "issues", but we don't have to if you don't wanna.

jadeallenx · 2023-10-09T23:16:29Z

Yeah let's plan for a release on Thursday

Try to make it easier to maintain for packages

f2a000d

paulo-ferraz-oliveira commented Oct 4, 2023

View reviewed changes

kerl Outdated Show resolved Hide resolved

paulo-ferraz-oliveira commented Oct 4, 2023

View reviewed changes

kerl Show resolved Hide resolved

paulo-ferraz-oliveira commented Oct 4, 2023

View reviewed changes

kerl Show resolved Hide resolved

paulo-ferraz-oliveira requested a review from jadeallenx October 4, 2023 02:33

paulo-ferraz-oliveira mentioned this pull request Oct 4, 2023

Add pacman check to avoid false warnings #377

Closed

paulo-ferraz-oliveira added 2 commits October 5, 2023 14:56

Act on test results: we need the _pkgs suffix to identify distro

76178e3

Act on self-review: be more specific in README

26ba3b3

Act on test results: make sure build header appears before first entries

918e306

paulo-ferraz-oliveira commented Oct 5, 2023

View reviewed changes

kerl Show resolved Hide resolved

paulo-ferraz-oliveira commented Oct 5, 2023

View reviewed changes

kerl Outdated Show resolved Hide resolved

paulo-ferraz-oliveira added 5 commits October 5, 2023 19:48

Simplify code for our specific use case

a15da36

The _apk, _rpm and _dpkg functions do an echo (they build a string) That string is used to both output what failed, but also be evaluated to probe for a given package install

Act on test results: further normalise ID string

9d47290

In RHEL the value is "rhel", with the quote (and since we're not sourcing the file, but parsing it, we need to do some extra work)

Act on self-review results: simplify probe code

a68e24b

We're already, out of the functions, doing the /dev/null + 2>&1 dance

Act on test results: on rhel g++ is called gcc-c++

48aa2b1

Act on local tests: add missing Alpine/apk dep. automake

afab83d

paulo-ferraz-oliveira added 3 commits October 8, 2023 21:21

Act on local tests: go back to a more robust dpkg-query check

e7cb68e

"Remove" opensuse package detection

c6a7ac7

This appears to require zypper to be done, so can probably wait for community contribution to happen

Act on local test results: make commands easier to spot

38201b2

jadeallenx requested changes Oct 9, 2023

View reviewed changes

paulo-ferraz-oliveira added 3 commits October 9, 2023 21:52

Act on review comment: make true/false yes/no instead of 1/0

6505b69

For readability purposes

Act on review comment: merge log sentences

7fa76c3

Act on self-review: be consistent in writing

f6deb2b

paulo-ferraz-oliveira requested a review from jadeallenx October 9, 2023 21:07

jadeallenx approved these changes Oct 9, 2023

View reviewed changes

paulo-ferraz-oliveira mentioned this pull request Oct 9, 2023

Improve on logging and stderr'ing stuff #475

Closed

paulo-ferraz-oliveira merged commit aad7ea3 into kerl:master Oct 9, 2023
9 checks passed

paulo-ferraz-oliveira deleted the fix/missing-pkg-management branch October 9, 2023 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to make it easier to maintain for packages (and distros) #471

Try to make it easier to maintain for packages (and distros) #471

paulo-ferraz-oliveira commented Oct 4, 2023 •

edited

Loading

paulo-ferraz-oliveira commented Oct 5, 2023

paulo-ferraz-oliveira commented Oct 5, 2023

paulo-ferraz-oliveira commented Oct 5, 2023

paulo-ferraz-oliveira commented Oct 5, 2023 •

edited

Loading

paulo-ferraz-oliveira commented Oct 8, 2023

paulo-ferraz-oliveira commented Oct 8, 2023

jadeallenx commented Oct 9, 2023

Idea 💡

jadeallenx Oct 9, 2023

paulo-ferraz-oliveira Oct 9, 2023

paulo-ferraz-oliveira Oct 9, 2023

jadeallenx Oct 9, 2023

paulo-ferraz-oliveira commented Oct 9, 2023

paulo-ferraz-oliveira commented Oct 9, 2023

jadeallenx commented Oct 9, 2023

jadeallenx left a comment

jadeallenx commented Oct 9, 2023 •

edited

Loading

jadeallenx commented Oct 9, 2023

jadeallenx commented Oct 9, 2023

Idea 💡

paulo-ferraz-oliveira commented Oct 9, 2023

paulo-ferraz-oliveira commented Oct 9, 2023

jadeallenx commented Oct 9, 2023

Try to make it easier to maintain for packages (and distros) #471

Try to make it easier to maintain for packages (and distros) #471

Conversation

paulo-ferraz-oliveira commented Oct 4, 2023 • edited Loading

Description

Further considerations

Also take a look at

paulo-ferraz-oliveira commented Oct 5, 2023

paulo-ferraz-oliveira commented Oct 5, 2023

Idea 💡

paulo-ferraz-oliveira commented Oct 5, 2023

paulo-ferraz-oliveira commented Oct 5, 2023 • edited Loading

paulo-ferraz-oliveira commented Oct 8, 2023

Idea 💡

paulo-ferraz-oliveira commented Oct 8, 2023

jadeallenx commented Oct 9, 2023

Idea 💡

jadeallenx Oct 9, 2023

Choose a reason for hiding this comment

paulo-ferraz-oliveira Oct 9, 2023

Choose a reason for hiding this comment

paulo-ferraz-oliveira Oct 9, 2023

Choose a reason for hiding this comment

jadeallenx Oct 9, 2023

Choose a reason for hiding this comment

paulo-ferraz-oliveira commented Oct 9, 2023

paulo-ferraz-oliveira commented Oct 9, 2023

jadeallenx commented Oct 9, 2023

jadeallenx left a comment

Choose a reason for hiding this comment

jadeallenx commented Oct 9, 2023 • edited Loading

jadeallenx commented Oct 9, 2023

jadeallenx commented Oct 9, 2023

Idea 💡

paulo-ferraz-oliveira commented Oct 9, 2023

paulo-ferraz-oliveira commented Oct 9, 2023

jadeallenx commented Oct 9, 2023

paulo-ferraz-oliveira commented Oct 4, 2023 •

edited

Loading

paulo-ferraz-oliveira commented Oct 5, 2023 •

edited

Loading

jadeallenx commented Oct 9, 2023 •

edited

Loading