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

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

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Oct 4, 2023

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:

  1. 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
  2. the probes are, as before, unique for all packages, but since this is an evaluated command we can play with more complex probes
  3. 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)
  4. 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:

  • alpine
  • debian
  • fedora
  • linuxmint
  • opensuse
  • rhel
  • ubuntu

Also take a look at

Closes #439.
Closes #431.

kerl Outdated Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Contributor Author

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

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

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)!

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Oct 5, 2023

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 Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

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).

@paulo-ferraz-oliveira
Copy link
Contributor Author

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
Copy link
Collaborator

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 Outdated Show resolved Hide resolved
kerl Outdated
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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you could combine these messages.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

kerl Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Contributor Author

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

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
Copy link
Collaborator

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

Copy link
Collaborator

@jadeallenx jadeallenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jadeallenx
Copy link
Collaborator

jadeallenx commented Oct 9, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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

There we go: #475

@paulo-ferraz-oliveira
Copy link
Contributor Author

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
Copy link
Collaborator

Yeah let's plan for a release on Thursday

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit aad7ea3 into kerl:master Oct 9, 2023
9 checks passed
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the fix/missing-pkg-management branch October 9, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants