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

Remove embedded DESTDIR in make variables, honour them #339

Merged
merged 19 commits into from
Sep 21, 2023

Conversation

evelikov
Copy link
Collaborator

@evelikov evelikov commented Aug 15, 2023

This series was inspired by a bunch of hardcoded paths across the project. The more i looked into the code, the bugs stared back at me ...

As a whole this series does a few closely related things:

  • Removes a few seemingly broken and unused scripts
  • No longer compresses the man page
  • Removes some should-not-have-been-a-variable and unused-variable instances
  • Removes the uncommon hard-coded DESTDIR prefix for variables
  • Ensures the remaining variables are honoured across the board

In practical terms this means:

  • find-provides, lsb_release and dkms_common.postinst (only user that I can find is Remove /usr/lib/dkms/common.postinst instances/handling openzfs/zfs#15175) (edit: keep dkms_common.postinst) are gone. @anbe42, @xnox, @scaronni others, I believe Debian Ubuntu and Fedora should not be affected by this removal. Can you give it a quick check on your end and let me know?
  • The gzip manpage compression is left for builders/distros - @AndrewAmmerlaan you can drop the workaround from the Gentoo ebuild now
  • setting VAR, ETC, BASHDIR, MAN, DOCDIR and SHAREDIR is no longer allowed - only Arch was setting one of these BASHDIR - to the default value nevertheless
  • setting SYSTEMD, SBIN, LIBDIR and KCONF path does not need explicit DESTDIR prefix
  • hardcoded paths are removed in favour of the SBIN, LIBDIR and KCONF variables - @scaronni @AndrewAmmerlaan

Some notes to the Arch maintainer @seblu (Arch task):

  • the 4 year old PKGBUILD.26 can be removed
  • the moddir patching aka s,/lib/modules,/usr/lib/modules,g should be dropped - we don't patch kmod either
  • BASHDIR can be dropped, it's the default
  • SBIN, KCONF and LIBDIR are relative to DESTDIR so they should be updated
  • hook.sh should probably be reworked to use dkms autoinstall - happy to make autoinstall changes to make that possible edit: had a quick look, there's a lot more going on in hook.sh

Closes: #324

It was used solely by the make_rpm machinery. With that one gone we can
drop the script.

The script seems outdated (no support for zstd modules) and
undocumented. If we are to revive it (no objections there) we should
address those two as a base acceptance entry.

Signed-off-by: Emil Velikov <[email protected]>
A while ago we started using os-release to get the distribution
name/details. Since then all the distros that I've seen (apart from
Debian and derivatives) have dropped lsb-release from their
dependencies.

Let's remove the final remanands, this in turns allows us to remove the
special PATH mangling and the unmaintained in-tree lsb_release (see next
commit).

As a nice bonus - source the /etc/os-release in a sub-shell, so we don't
get any other control flow/variables as side-effect.

Signed-off-by: Emil Velikov <[email protected]>
The codebase has moved to use /etc/os-release with a fallback to
lsb_release. That said, the latter has two variants a data file in /etc
or an executable in $PATH.

The in-tree lsb_release is an executable script, although it's installed
outside of $PATH and no distribution (that I can see) adjusts the
environment variable or even uses external lsb_release dependency.

Considering the lack of practical users, let's remove the script. We can
worry about the remaining in-tree lsb-release references later on.

Signed-off-by: Emil Velikov <[email protected]>
Signed-off-by: Emil Velikov <[email protected]>
mkdir inherits the umask of the environment. Using install gets us past
that, while also being consistent with the rest of the codebase.

Signed-off-by: Emil Velikov <[email protected]>
There are dozen of instances across the codebase, that use /var...
although none of them actually honour the variable. Skimming through
existing projects/distros - they all seem to use the default.

Inline the instance and print a meaningful error in case it's set.

Signed-off-by: Emil Velikov <[email protected]>
There are dozen of instances across the codebase, that use /etc...
although none of them actually honour the variable. Skimming through
existing projects/distros - they all seem to use the default.

Inline the instance and print a meaningful error in case it's set.

Signed-off-by: Emil Velikov <[email protected]>
There are dozen of instances across the codebase, that use /usr...
although none of them actually honour the variable. Skimming through
existing projects/distros - they all seem to use the default.

Inline the instance and print a meaningful error in case it's set.

Signed-off-by: Emil Velikov <[email protected]>
The uncompressed manual is absolutely tiny, plus the decision to
compress or not is up-to the distribution. In cases where people
build/install dkms manually they can handle this themselves.

For example: Gentoo has a workaround to remove the compression
@AndrewAmmerlaan - you can now drop that \o/

Signed-off-by: Emil Velikov <[email protected]>
There is a single instance of the variable, there's no Linux
distributions that I can see overriding the default. Yet most
importantly the variable should really be called MANDIR, to be
consistent with the vast majority of projects out there.

BSD, Solaris and friends tend to use different path, but dkms is not
usually used on those.

As needed we can reinstate the config toggle (as MANDIR really) if
needed.

Signed-off-by: Emil Velikov <[email protected]>
Usual conventions dictates that DESTDIR is applied at the install stage.
We currently embed it into each use-configurable toggle, which goes
against that.

Signed-off-by: Emil Velikov <[email protected]>
AFACIT no linux distro out there uses the non-default values. Similar to
MAN - some BSD/Solaris distros might have differing path. Although until
that proves to be an issue, let's remove these.

Signed-off-by: Emil Velikov <[email protected]>
Usual conventions dictates that DESTDIR is applied at the install stage.
We currently embed it into each use-configurable toggle, which goes
against that.

Signed-off-by: Emil Velikov <[email protected]>
The current all target is a bit bonkers - it cleans all the generated
files, does not explicitly regenerate them and creates a distribution
tarball.

With the latter having the side effect of regenerating the required
files, by having a dependency list.

Similar to the tarball target, install also has an explicit dependency
list of files that need to be generated.

All of this is fairly uncommon to how all the projects (that I've seen)
operate. Namely:

all: all_the_generated_targets

install: all
   install steps

tarball: all

clean:
   foobar ... the target is never a dependency of any of the above

Signed-off-by: Emil Velikov <[email protected]>
If the user has provided SBIN we should honour it.

The Makefile handling is a bit repetitive, but polishing that is an
exercise for another day.

Signed-off-by: Emil Velikov <[email protected]>
Usual conventions dictates that DESTDIR is applied at the install stage.
We currently embed it into each use-configurable toggle, which goes
against that.

Signed-off-by: Emil Velikov <[email protected]>
If the user has set LIBDIR we should honour it.

Signed-off-by: Emil Velikov <[email protected]>
@Nowa-Ammerlaan
Copy link
Contributor

  • The gzip manpage compression is left for builders/distros - @AndrewAmmerlaan you can drop the workaround from the Gentoo ebuild now
    ...
  • hardcoded paths are removed in favour of the SBIN, LIBDIR and KCONF variables - @scaronni @AndrewAmmerlaan

Great! Thanks for the heads-up.

@evelikov
Copy link
Collaborator Author

Correction: Debian packaging has some common.postinst references - namely 1 and 2 although I'm not sure if these Legacy use-cases are still applicable/relevant. The HowTo looks outdated.

@anbe42 feel free to forward ^^ to fellow maintainers.

@anbe42
Copy link
Contributor

anbe42 commented Aug 17, 2023

I believe the legacy support in dh_dkms can go away (just need to check that it is not used anymore by any package) and the howto should be dropped.

But installation of a *-dkms package calls /usr/lib/dkms/common.postinst from its postinst script to build the newly installed module for all installed kernel headers, so that script is still needed. And it is expected to exist by all existing *-dkms packages that depend on dkms >= 2.1

I'll look into this in more detail after holidays ...

Usual conventions dictates that DESTDIR is applied at the install stage.
We currently embed it into each use-configurable toggle, which goes
against that.

Signed-off-by: Emil Velikov <[email protected]>
If the user has set KCONF we should honour it.

Signed-off-by: Emil Velikov <[email protected]>
@evelikov
Copy link
Collaborator Author

Sounds like it's better if we keep the script for now. Especially given its overall complexity, completely undocumented nature and the limited response (understandably) from core OpenZFS people.

Will open a separate MR for that yakshaving exercise ... and its companions.

@evelikov
Copy link
Collaborator Author

Modulo any objections I will be merging this on Friday.

@evelikov
Copy link
Collaborator Author

Whelp I guess it was my time to catch COVID. Somewhat recovered, let's merge this and enjoy any fallouts :-)

@AllKind
Copy link
Contributor

AllKind commented Feb 29, 2024

@evelikov
As I was made aware here openzfs/zfs#15415 (comment), the way I implemented the dkms routines for ZFS do not work in chroot environment.
The only way to make it work I see is to reintroduce the use of common.postinst.
So I come here to ask if you have any other idea?

@evelikov
Copy link
Collaborator Author

@AllKind haven't used chroot in ages, so I don't recall all the gotchas.

We do have a task to make dkms chroot safe/happy - #145 - patches are very much welcome 😅

@AllKind
Copy link
Contributor

AllKind commented Feb 29, 2024

@evelikov
I referenced #145...
The user says:

The chroot environment is an offline build of a virtual machine disk image and uses a different kernel from the host so installing those headers is not the solution. Other software using dkms does not have this problem and until recent 2.1.x versions zfs did not have an issue. We've been building this way since 0.6.x releases.

The common.postinst has handling exactly for the situation that the environment is a chroot:

if [ -z "$autoinstall_all_kernels" ]; then
    # If the current kernel is installed on the system or chroot
    if [ `_is_kernel_name_correct $CURRENT_KERNEL` = "yes" ]; then
        if [ -n "$NEWEST_KERNEL" ] && [ ${CURRENT_KERNEL} != ${NEWEST_KERNEL} ]; then
            KERNELS="$CURRENT_KERNEL $NEWEST_KERNEL"
        else
            KERNELS=$CURRENT_KERNEL
        fi
    # The current kernel is not useful as it's not installed
    else
        echo "It is likely that $CURRENT_KERNEL belongs to a chroot's host"

        # Let's use only the newest kernel if this is not a first installation
        # otherwise build for all kernels
        if [ -n "$NEWEST_KERNEL" -a -n "$UPGRADE" ]; then
            KERNELS="$NEWEST_KERNEL"
        fi
    fi
fi

So common.postinst can handle it somehow.
According to him it works in chroot.
The problem for me is, that I don't know a way in a post installation scriptlet, to tell dkms which kernel to use.
common.postinst has some logic to do that. How reliable that is, I don't know.
And I can't (doesn't really make sense) replicate all of common.postinst into the post installation scriptlet.
So I'm stuck with reverting my patch for zfs to use common.postinst?

Thanks for your input!

Edit: I never used chroot before, so I know like nothing about it :-(

@evelikov
Copy link
Collaborator Author

evelikov commented Mar 6, 2024

@AllKind can I ask you to open a new issue and add a very rough reproducer in there (if possible).

In the past I've seen cases where kernel arch/version isn't handled properly. Odds are this could be a either openzfs issue, dkms one or both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove hard-coded paths
4 participants