Re: [pve-devel] [PATCH container] setup: Fix architecture detection for NixOS containers

[Christoph Heiss <c.heiss@proxmox.com>]

Thanks for the review!

On Mon, Aug 21, 2023 at 02:50:28PM +0200, Fiona Ebner wrote:
>
> Am 28.02.23 um 11:59 schrieb Christoph Heiss:
> > diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
> > index 891231f..4346c5e 100644
> > --- a/src/PVE/LXC/Setup.pm
> > +++ b/src/PVE/LXC/Setup.pm
> > @@ -131,6 +131,20 @@ sub new {
> >  	$plugin->{rootgid} = $rootgid;
> >      }
> >
> > +    # if arch is unset, we try to autodetect it
> > +    if (!defined($conf->{arch})) {
> > +	my $arch = eval { $self->protected_call(sub { $plugin->detect_architecture() }) };
> > +
>
> The error from the eval should be logged here.
Ack, will do.

>
> > +	if (!defined($arch)) {
> > +	    $arch = 'amd64';
> > +	    print "Falling back to $arch.\nUse `pct set VMID --arch ARCH` to change.\n";
> > +	} else {
> > +	    print "Detected container architecture: $arch\n";
> > +	}
> > +
>
> (...)
>
> > diff --git a/src/PVE/LXC/Setup/NixOS.pm b/src/PVE/LXC/Setup/NixOS.pm
> > index 845d2d5..4f7b93e 100644
> > --- a/src/PVE/LXC/Setup/NixOS.pm
> > +++ b/src/PVE/LXC/Setup/NixOS.pm
> > @@ -5,6 +5,7 @@ use warnings;
> >
> >  use File::Path 'make_path';
> >
> > +use PVE::LXC::Setup;
>
> This is a cyclic include, please let us avoid those. We could either use
> a new helper module for the detect_architecture() helper or otherwise
> move it to pve-guest-common or even pve-common. The detection itself is
> pretty generic after all.
I will probably go this route and simply move it to another module in
pve-container. As long as it is not needed anywhere else, I think it is
sensible to letting it stay close it is only user.

>
> An alternative to that would be to just add a function get_elf_fn() to
> the plugin interface and change the existing detect_architecture() to
> use that.
>
> >  use PVE::LXC::Setup::Base;
> >
> >  use base qw(PVE::LXC::Setup::Base);