liquidctl / liquidtux

Linux kernel hwmon drivers for AIO liquid coolers and other devices
Other
80 stars 15 forks source link

Makefile: fix shell errors in dkms_install #41

Closed amezin closed 1 year ago

amezin commented 1 year ago

See https://github.com/liquidctl/liquidtux/actions/runs/3955424494/jobs/6773694899#step:6:7

I don't know dash (Ubuntu's /bin/sh) that well, so let's just force Makefile SHELL to be bash.

jonasmalacofilho commented 1 year ago

That whole sourcing of dkms.conf was already somewhat ugly and non-obvious.

I propose we consider going back to the default sh and using the fact that we know what both PACKAGE_NAME and PACKAGE_VERSION are supposed to be.

# untested, but at least shows what I mean

INSTALL_BASE_DIR := $(DESTDIR)$(prefix)/src/liquidtux-$(PKGVER)

dkms_install: dkms.conf
    mkdir -p $(INSTALL_BASE_DIR)
    $(INSTALL_DATA) dkms.conf Makefile Kbuild $(SOURCES) $(INSTALL_BASE_DIR)/

This has the disadvantage of no longer allowing external customization to those variables by direct edits to dkms.conf, but do we specifically care about that use case?


EDIT: if that use case is indeed important to us, we can add an additional PKGNAME variable to the Makefile, and change dkms.conf.in accordingly, to support customization of that variable directly, still without needing to source dkms.conf.

amezin commented 1 year ago

PKGVER might be out of sync with PACKAGE_VERSION from dkms.conf I. e. you've generated a dkms.conf, then made a commit dkms.conf won't be re-generated, dkms install will fail That's why I'm trying to read from dkms.conf

jonasmalacofilho commented 1 year ago

Yes, that's what I meant by

This has the disadvantage of no longer allowing external customization to those variables by direct edits to dkms.conf, but do we specifically care about that use case?

A Makefile is elegant but quite rudimentary: there are always implicit preconditions like "don't change x, y, and z files from under us, or things won't work".

So unless we specifically care about supporting that specific use case (=externally changing dkms.conf), I don't think we need to.


EDIT (again, sorry): and if we do care about allowing changes to PACKAGE_NAME, shouldn't we just support doing it through the Makefile, like we do with PACKAGE_VERSION?

amezin commented 1 year ago

It's not an external customization - it's just a stale file.

Again:

  1. I've ran make dkms_install once - dkms.conf was generated, with a commit id in it
  2. git commit
  3. make dkms_install again - PKGVER has new commit id in it (plus increased distance from tag), but dkms.conf haven't been updated - version mismatch between install path and the dkms.conf installed in it.

No manual edits in dkms.conf. It's a normal workflow that shouldn't fail this way IMO.

I do not expect anyone to edit the generated file. I only care about mismatching versions. PACKAGE_NAME is there just because dkms.conf is loaded anyway.

Alternatives are:

jonasmalacofilho commented 1 year ago

In that case we're already messing up a normal workflow: Arch users generally build new versions of an AUR package with the previous build files (or, when using an AUR helper, a cache).


PKGVER has to go through to the dkms.conf every time: distros and even users expect that to happen once they explicitly specified it.

But for those manually installing with make dkms_install, without overriding PKGVER, then perhaps a better alternative is for us to start using a manually managed version number? I say this because uninstalling the previous DKMS files isn't super ergonomic, especially when the version comes from git describe. So it might be good to avoid bumping the version number every commit, and only doing it when there has been a significant change.

Of course, keeping track of a version number for humans (and not just machines) takes some work. And we would need to decide when to update it, preferably with some consistent yet simple guidelines.

amezin commented 1 year ago

Ok, I've changed the makefile to generate dkms.conf every time

jonasmalacofilho commented 1 year ago

@amezin,

Thanks. But what about

[...] perhaps a better alternative is for us to start using a manually managed version number? [...]

amezin commented 1 year ago

But for those manually installing with make dkms_install, without overriding PKGVER, then perhaps a better alternative is for us to start using a manually managed version number?

All I care about is that dkms_install shouldn't produce inconsistent/incorrect results.

It's of course possible to just cut off the commit id from the version/simply use the latest tag as a version. I even have a github action for the case when you want a version but are too lazy to do proper semver: https://github.com/ddterm/autotag

Having the commit id in a version string is useful when developing/testing though - avoids the question "did I actually build/install the intended version".

But let's focus on the issue that needs to be fixed first: error messages when doing make dkms_install.

jonasmalacofilho commented 1 year ago

All I care about is that dkms_install shouldn't produce inconsistent/incorrect results.

We should also care about providing *install rules that don't fill the user's system with too much junk (or that force the user to find another way to deal with that junk).

But let's focus on the issue that needs to be fixed first: error messages when doing make dkms_install.

Merging this to deal with the errors, for now.

amezin commented 1 year ago

We should also care about providing *install rules that don't fill the user's system with too much junk (or that force the user to find another way to deal with that junk).

Users should really install distro packages...

I need to experiment with rpm/deb packaging and Open Build Service (for a different project). dkms package is relatively simple, has almost no dependencies, so maybe I'll try to setup packaging for liquidtux as an excercise.