Re: [PATCH 08/20] Standardize directories and symlinks in images

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 16350 bytes --]

On 9/8/25 04:59, Alyssa Ross wrote:
> 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.

I decided that it was simpler to make all of the links in the same place
so that it would be easier to add or remove them in the future.  Moving
creation to common code seems more complexity than two rm commands.

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

Will send later.
  
>>  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 /.

It is certainly /root on my systems.

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

Will send separately.

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

I decided it was better to add all of these now to avoid any sort of
problems later on.  The size impact is tiny and the cost of debugging
a problem later on would not be.  In particular, contributors not so
used to NixOS might assume these exist.

>> +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, ... }: {


-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]