Demi Marie Obenour <demiobenour@gmail.com> writes:

> There are a few directories and symbolic links that a Linux system
> should always have.  Even if Spectrum OS itself does not use them,
> third-party dependencies and/or applications might rely on them.
> Create these in scripts/make-erofs.sh rather than separately in
> each VM's build scripts.  The creation of /run/lock assumes that
> s6-linux-init is being used, but that assumption is easy to fix later.
> This also enforces that the symlinks and directories were *not* created
> in other places.  The app VM build violated this rule, so fix it.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

This really seems like it's making things substantially more
complicated, especially with the need to remove links so they can later
be recreated again by make-erofs.sh.  If we really want to make sure we
don't forget certain directories, we could do that in a much simpler way
by just checking for existence once we've assembled the directory that
will become the image.

> ---
>  host/rootfs/Makefile   |  15 ++------
>  host/rootfs/bin        |   1 -
>  host/rootfs/lib        |   1 -
>  host/rootfs/sbin       |   1 -
>  img/app/Makefile       |   8 ++--
>  img/app/bin            |   1 -
>  img/app/default.nix    | 101 +++++++++++++++++++++++++++++--------------------
>  img/app/sbin           |   1 -
>  scripts/make-erofs.sh  |  34 +++++++++++++++++
>  vm/sys/net/Makefile    |   8 +---
>  vm/sys/net/bin         |   1 -
>  vm/sys/net/default.nix |   2 +
>  vm/sys/net/lib         |   1 -
>  vm/sys/net/sbin        |   1 -
>  vm/sys/net/var/run     |   1 -
>  15 files changed, 106 insertions(+), 71 deletions(-)
>
> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
> index dce78e60bc1a8c18f5f448aaa9aeed2c8a7da04e..6cdbac201257faedb70344bcfd5cf9d4fd25b507 100644
> --- a/host/rootfs/Makefile
> +++ b/host/rootfs/Makefile
> @@ -54,7 +54,6 @@ FILES = \
>  	etc/s6-linux-init/scripts/rc.init \
>  	etc/xdg/weston/autolaunch \
>  	etc/xdg/weston/weston.ini \
> -	usr/share/dbus-1/services/org.freedesktop.portal.Documents.service \
>  	usr/bin/assign-devices \
>  	usr/bin/create-vm-dependencies \
>  	usr/bin/run-appimage \
> @@ -63,10 +62,10 @@ FILES = \
>  	usr/bin/vm-import \
>  	usr/bin/vm-start \
>  	usr/bin/vm-stop \
> -	usr/bin/xdg-open
> +	usr/bin/xdg-open \
> +	usr/share/dbus-1/services/org.freedesktop.portal.Documents.service

Would nice for this sort of trivial fix to be a separate patch that
could be immediately applied.

>  
>  DIRS = \
> -	dev \
>  	etc/s6-linux-init/env \
>  	etc/s6-linux-init/run-image/configs \
>  	etc/s6-linux-init/run-image/service/dbus/instance \
> @@ -90,14 +89,11 @@ DIRS = \
>  	etc/s6-linux-init/run-image/service/xdg-desktop-portal-spectrum-host/instances \
>  	etc/s6-linux-init/run-image/service/xdg-desktop-portal-spectrum-host/template/data \
>  	etc/s6-linux-init/run-image/service/xdg-desktop-portal-spectrum-host/template/env \
> -	etc/s6-linux-init/run-image/user \
>  	etc/s6-linux-init/run-image/vm/by-id \
>  	etc/s6-linux-init/run-image/vm/by-name \
>  	etc/s6-linux-init/run-image/wait \
>  	ext \
> -	run \
> -	proc \
> -	sys \
> +	root \

I'm not sure what we'd want /root for?  Root's home directory is /.

>  	var
>  
>  FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
> @@ -105,11 +101,8 @@ FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo
>  # These are separate because they need to be included, but putting
>  # them as make dependencies would confuse make.
>  LINKS = \
> -	bin \
>  	etc/s6-linux-init/run-image/opengl-driver \
> -	etc/s6-linux-init/run-image/service/vmm/template/run \
> -	lib \
> -	sbin
> +	etc/s6-linux-init/run-image/service/vmm/template/run
>  
>  BUILD_FILES = build/etc/s6-rc
>  
> diff --git a/host/rootfs/bin b/host/rootfs/bin
> deleted file mode 120000
> index 1e881eda3a544eaa86b6019cbe7067ffc58bfafc..0000000000000000000000000000000000000000
> --- a/host/rootfs/bin
> +++ /dev/null
> @@ -1 +0,0 @@
> -usr/bin
> \ No newline at end of file
> diff --git a/host/rootfs/lib b/host/rootfs/lib
> deleted file mode 120000
> index 0d5487ba8608d4d1a7328cf8a4e0242d1988c491..0000000000000000000000000000000000000000
> --- a/host/rootfs/lib
> +++ /dev/null
> @@ -1 +0,0 @@
> -usr/lib
> \ No newline at end of file
> diff --git a/host/rootfs/sbin b/host/rootfs/sbin
> deleted file mode 120000
> index 1e881eda3a544eaa86b6019cbe7067ffc58bfafc..0000000000000000000000000000000000000000
> --- a/host/rootfs/sbin
> +++ /dev/null
> @@ -1 +0,0 @@
> -usr/bin
> \ No newline at end of file
> diff --git a/img/app/Makefile b/img/app/Makefile
> index c6b9a23ce8796582d6e2f5121c30c2269975aa2d..062082e35ba352a8f0520b28379690f5a2ba2ed3 100644
> --- a/img/app/Makefile
> +++ b/img/app/Makefile
> @@ -57,15 +57,15 @@ VM_FILES = \
>  	etc/wireplumber/wireplumber.conf.d/99_spectrum.conf \
>  	etc/xdg/xdg-desktop-portal/portals.conf
>  
> -VM_DIRS = dev run proc sys tmp var \
> +VM_DIRS = \
>  	etc/s6-linux-init/run-image/service \
> -	etc/s6-linux-init/run-image/user \
> -	etc/s6-linux-init/run-image/wait
> +	etc/s6-linux-init/run-image/wait \
> +	var
>  VM_FIFOS = etc/s6-linux-init/run-image/service/s6-linux-init-shutdownd/fifo
>  
>  # These are separate because they need to be included, but putting
>  # them as make dependencies would confuse make.
> -VM_LINKS = bin etc/ssl/certs/ca-certificates.crt sbin
> +VM_LINKS = etc/ssl/certs/ca-certificates.crt
>  
>  VM_BUILD_FILES = build/etc/s6-rc
>  
> diff --git a/img/app/bin b/img/app/bin
> deleted file mode 120000
> index 1e881eda3a544eaa86b6019cbe7067ffc58bfafc..0000000000000000000000000000000000000000
> --- a/img/app/bin
> +++ /dev/null
> @@ -1 +0,0 @@
> -usr/bin
> \ No newline at end of file
> diff --git a/img/app/default.nix b/img/app/default.nix
> index d3eed1f0accdc8968d1ba5bdec74ab597789082f..4daee260afd41de14de06a006b00c2c6db0f5e2a 100644
> --- a/img/app/default.nix
> +++ b/img/app/default.nix
> @@ -12,6 +12,42 @@ pkgsStatic.callPackage (
>  }:
>  
>  let
> +  kernelTarget =
> +    if stdenvNoCC.hostPlatform.isx86 then
> +      # vmlinux.bin is the stripped version of vmlinux.
> +      # Confusingly, compressed/vmlinux.bin is the stripped version of
> +      # the top-level vmlinux target, while the top-level vmlinux.bin
> +      # is the stripped version of compressed/vmlinux.  So we use
> +      # compressed/vmlinux.bin, since we want a stripped version of
> +      # the kernel that *hasn't* been built to be compressed.  Weird!
> +      "compressed/vmlinux.bin"
> +    else
> +      stdenvNoCC.hostPlatform.linux-kernel.target;
> +
> +  kernel = (linux_latest.override {
> +    structuredExtraConfig = with lib.kernel; {
> +      DRM_FBDEV_EMULATION = lib.mkForce no;
> +      EROFS_FS = yes;
> +      FONTS = lib.mkForce unset;
> +      FONT_8x8 = lib.mkForce unset;
> +      FONT_TER16x32 = lib.mkForce unset;
> +      FRAMEBUFFER_CONSOLE = lib.mkForce unset;
> +      FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER = lib.mkForce unset;
> +      FRAMEBUFFER_CONSOLE_DETECT_PRIMARY = lib.mkForce unset;
> +      FRAMEBUFFER_CONSOLE_ROTATION = lib.mkForce unset;
> +      RC_CORE = lib.mkForce unset;
> +      VIRTIO = yes;
> +      VIRTIO_BLK = yes;
> +      VIRTIO_CONSOLE = yes;
> +      VIRTIO_PCI = yes;
> +      VT = no;
> +    };
> +  }).overrideAttrs ({ installFlags ? [], ... }: {
> +    installFlags = installFlags ++ [
> +      "KBUILD_IMAGE=$(boot)/${kernelTarget}"
> +    ];
> +  });
> +
>    appimageFhsenv = (buildFHSEnv (appimageTools.defaultFhsEnvArgs // {
>      name = "vm-fhs-env";
>      targetPkgs = pkgs: appimageTools.defaultFhsEnvArgs.targetPkgs pkgs ++ [
> @@ -53,50 +89,33 @@ let
>        pkgs.wireplumber
>      ];
>    })).fhsenv;
> -in
>  
> -let

Another cleanup that would be really nice to have separately, so I don't
have to try to review two things at once.

>    packagesSysroot = runCommand "packages-sysroot" {} ''
> -    mkdir -p $out/etc/ssl/certs
> -    ln -s ${appimageFhsenv}/{lib64,usr} ${kernel}/lib $out
> -    ln -s ${cacert}/etc/ssl/certs/* $out/etc/ssl/certs
> +    set -eu
> +    mkdir -p -- "$out/etc/ssl/certs" "$out/usr/bin"
> +    # ../../scripts/make-erofs.sh will re-create these
> +    rm -f -- "$out/usr/lib64" "$out/usr/lib"
> +    source_dir=${lib.escapeShellArg appimageFhsenv}/usr
> +    for i in "$source_dir"/*; do
> +      subdir=''${i##*/}
> +      case $subdir in
> +      (bin|include|lib|lib64|libexec|sbin|share) :;;
> +      (*) printf 'Bad subdirectory %s\n' "$subdir" >&2; exit 1;;
> +      esac
> +    done
> +    if ! [ -h "$source_dir/lib" ]; then echo "FHSenv didn't make lib a symlink" >&2; exit 1; fi
> +    ln -s -- "$source_dir/include" "$source_dir/libexec" "$source_dir/share" "$out/usr"
> +    cp -RT -- "$source_dir/lib64" "$out/usr/lib"
> +    # Do this first so that the subsequent call to cp (without -T)
> +    # will create new entries in the existing bin directory.
> +    cp -RT -- "$source_dir/sbin" "$out/usr/bin"
> +    # with -T cp tries to delete the whole target directory first
> +    cp -R -- "$source_dir/bin" "$out/usr"
> +    # so that ln can make the symlink
> +    chmod -- 0755 "$out/usr/lib"
> +    ln -s -- ${lib.escapeShellArg kernel}/lib/modules "$out/usr/lib/"
> +    ln -s -- ${lib.escapeShellArg cacert}/etc/ssl/certs/* "$out/etc/ssl/certs"
>    '';
> -
> -  kernelTarget =
> -    if stdenvNoCC.hostPlatform.isx86 then
> -      # vmlinux.bin is the stripped version of vmlinux.
> -      # Confusingly, compressed/vmlinux.bin is the stripped version of
> -      # the top-level vmlinux target, while the top-level vmlinux.bin
> -      # is the stripped version of compressed/vmlinux.  So we use
> -      # compressed/vmlinux.bin, since we want a stripped version of
> -      # the kernel that *hasn't* been built to be compressed.  Weird!
> -      "compressed/vmlinux.bin"
> -    else
> -      stdenvNoCC.hostPlatform.linux-kernel.target;
> -
> -  kernel = (linux_latest.override {
> -    structuredExtraConfig = with lib.kernel; {
> -      DRM_FBDEV_EMULATION = lib.mkForce no;
> -      EROFS_FS = yes;
> -      FONTS = lib.mkForce unset;
> -      FONT_8x8 = lib.mkForce unset;
> -      FONT_TER16x32 = lib.mkForce unset;
> -      FRAMEBUFFER_CONSOLE = lib.mkForce unset;
> -      FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER = lib.mkForce unset;
> -      FRAMEBUFFER_CONSOLE_DETECT_PRIMARY = lib.mkForce unset;
> -      FRAMEBUFFER_CONSOLE_ROTATION = lib.mkForce unset;
> -      RC_CORE = lib.mkForce unset;
> -      VIRTIO = yes;
> -      VIRTIO_BLK = yes;
> -      VIRTIO_CONSOLE = yes;
> -      VIRTIO_PCI = yes;
> -      VT = no;
> -    };
> -  }).overrideAttrs ({ installFlags ? [], ... }: {
> -    installFlags = installFlags ++ [
> -      "KBUILD_IMAGE=$(boot)/${kernelTarget}"
> -    ];
> -  });
>  in
>  
>  stdenvNoCC.mkDerivation {
> diff --git a/img/app/sbin b/img/app/sbin
> deleted file mode 120000
> index 1e881eda3a544eaa86b6019cbe7067ffc58bfafc..0000000000000000000000000000000000000000
> --- a/img/app/sbin
> +++ /dev/null
> @@ -1 +0,0 @@
> -usr/bin
> \ No newline at end of file
> diff --git a/scripts/make-erofs.sh b/scripts/make-erofs.sh
> index d566a4ac7b30f55338fe9b8b6a94702686f6ddd1..5196394d405310971659b0dbc0c91cfcaaaf9118 100755
> --- a/scripts/make-erofs.sh
> +++ b/scripts/make-erofs.sh
> @@ -115,5 +115,39 @@ find "$root" \
>  find "$root/etc" "$root/var" ! -type l -execdir chmod u+w,go-w,ugo+rX -- '{}' +
>  chmod 0755 "$root"
>  
> +# Fix permissions on / so that the subsequent commands work
> +chmod 0755 "$root"
> +
> +# Create the basic mount points for pseudo-filesystems and tmpfs filesystems.
> +# These should always be mounted over, so use 0400 permissions for them.
> +# 0000 would be better, but it breaks mkfs.erofs as it tries to open the
> +# directories for reading.
> +mkdir -m 0400 "$root/dev" "$root/proc" "$root/run" "$root/sys" "$root/tmp"
> +
> +# Cause s6-linux-init to create /run/lock and /run/user
> +# with the correct mode (0755) and create /home,
> +# /var/cache, /var/log, and /var/spool directly.
> +mkdir -m 0755 \
> +	"$root/etc/s6-linux-init/run-image/lock" \
> +	"$root/etc/s6-linux-init/run-image/user" \
> +	"$root/home" \
> +	"$root/var/cache" \
> +	"$root/var/log" \
> +	"$root/var/spool"
> +
> +# Create symbolic links that are always expected to exist.
> +chmod 0755 "$root/usr"
> +ln -s ../proc/self/mounts "$root/etc/mtab"
> +ln -s ../run "$root/var/run"
> +ln -s ../run/lock "$root/var/lock"
> +ln -s ../tmp "$root/var/tmp"
> +ln -s bin "$root/usr/sbin"
> +ln -s lib "$root/usr/lib64"

This doesn't seem right as a generic thing.  Nix-built binaries won't
ever need this.  It's only in img/app for AppImage etc. compatibility.
Not relevant to other images.

> +ln -s usr/bin "$root/bin"
> +ln -s usr/bin "$root/sbin"
> +ln -s usr/lib "$root/lib"
> +ln -s usr/lib "$root/lib64"
> +chmod 0555 "$root/usr"
> +
>  # Make the erofs image.
>  mkfs.erofs -x-1 -b4096 --all-root "$@" "$root"
> diff --git a/vm/sys/net/Makefile b/vm/sys/net/Makefile
> index e6819400b2079e3eaa9d24737b2fc4b816a592c8..a8ad03862165a69f3f7dd3e49f668cfa887d817f 100644
> --- a/vm/sys/net/Makefile
> +++ b/vm/sys/net/Makefile
> @@ -39,11 +39,7 @@ VM_FILES = \
>  	etc/s6-linux-init/run-image/service/getty-hvc0/run \
>  	etc/s6-linux-init/scripts/rc.init \
>  	etc/sysctl.conf
> -VM_DIRS = dev etc/s6-linux-init/env run proc sys var/lib/connman
> -
> -# These are separate because they need to be included, but putting
> -# them as make dependencies would confuse make.
> -VM_LINKS = bin lib sbin var/run
> +VM_DIRS = etc/s6-linux-init/env var/lib/connman
>  
>  VM_BUILD_FILES = build/etc/s6-rc
>  
> @@ -53,7 +49,7 @@ build/empty:
>  build/rootfs.erofs: ../../../scripts/make-erofs.sh $(PACKAGES_FILE) $(VM_FILES) $(VM_BUILD_FILES) build/empty
>  	( \
>  	    cat $(PACKAGES_FILE) ;\
> -	    for file in $(VM_FILES) $(VM_LINKS); do printf '%s\n%s\n' $$file $$file; done ;\
> +	    for file in $(VM_FILES); do printf '%s\n%s\n' $$file $$file; done ;\
>  	    for file in $(VM_BUILD_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\
>  	    printf 'build/empty\n%s\n' $(VM_DIRS) ;\
>  	) | ../../../scripts/make-erofs.sh $@
> diff --git a/vm/sys/net/bin b/vm/sys/net/bin
> deleted file mode 120000
> index 1e881eda3a544eaa86b6019cbe7067ffc58bfafc..0000000000000000000000000000000000000000
> --- a/vm/sys/net/bin
> +++ /dev/null
> @@ -1 +0,0 @@
> -usr/bin
> \ No newline at end of file
> diff --git a/vm/sys/net/default.nix b/vm/sys/net/default.nix
> index b5873ebe1e80dd88c1ba997f7ebd3ee7369bb40f..a2c635e8ff09ab2b0ae4694344f3810c1b9739a5 100644
> --- a/vm/sys/net/default.nix
> +++ b/vm/sys/net/default.nix
> @@ -51,6 +51,8 @@ let
>      for pkg in ${lib.escapeShellArgs usrPackages}; do
>          lndir -ignorelinks -silent "$pkg" "$out/usr"
>      done
> +    [ -h "$out/usr/sbin" ]
> +    rm -f -- "$out/usr/sbin"
>    '';
>  
>    nixosAllHardware = nixos ({ modulesPath, ... }: {