Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.bugs.dist > #1224455 > unrolled thread

Bug#1090358: sbuild: please pre-set config variables with their defaults

Back to article view | Back to linux.debian.bugs.dist

Contents

  Bug#1090358: sbuild: please pre-set config variables with their defaults Chris Hofstaedtler <zeha@debian.org> - 2024-12-17 14:30 +0100
    Bug#1090358: sbuild: please pre-set config variables with their defaults Chris Hofstaedtler <zeha@debian.org> - 2024-12-17 15:10 +0100
      Bug#1090358: sbuild: please pre-set config variables with their defaults Chris Hofstaedtler <zeha@debian.org> - 2024-12-17 15:30 +0100
    Bug#1090358: sbuild: please pre-set config variables with their defaults Johannes Schauer Marin Rodrigues <josch@debian.org> - 2024-12-17 15:30 +0100
      Bug#1090358: sbuild: please pre-set config variables with their defaults Chris Hofstaedtler <zeha@debian.org> - 2024-12-17 16:00 +0100
      Bug#1090358: sbuild: please pre-set config variables with their defaults Chris Hofstaedtler <zeha@debian.org> - 2024-12-17 22:20 +0100
        Bug#1090358: sbuild: please pre-set config variables with their defaults Johannes Schauer Marin Rodrigues <josch@debian.org> - 2024-12-18 00:10 +0100
          Bug#1090358: sbuild: please pre-set config variables with their defaults Chris Hofstaedtler <zeha@debian.org> - 2024-12-18 09:00 +0100
            Bug#1090358: sbuild: please pre-set config variables with their defaults Chris Hofstaedtler <zeha@debian.org> - 2024-12-18 09:20 +0100
              Bug#1090358: sbuild: please pre-set config variables with their defaults Johannes Schauer Marin Rodrigues <josch@debian.org> - 2024-12-18 17:10 +0100

#1224455 — Bug#1090358: sbuild: please pre-set config variables with their defaults

Package: sbuild
Severity: wishlist

I wish sbuild would allow me to extend its config settings in
.sbuildrc or .config/sbuild/config.pl, without having to re-state
the previous defaults.

As an example, I want to extend $unshare_mmdebstrap_extra_args with
some option. But I want to keep the existing defaults too!

This bug report is mostly here to escape the salsa comment mess;
the traces are https://salsa.debian.org/debian/sbuild/-/merge_requests/104#note_559900
and some previous bug report that figured out one can use $conf in
the config file.

In https://salsa.debian.org/debian/sbuild/-/merge_requests/104#note_559973
josch@ provided a draft patch to sbuild:

--- a/lib/Sbuild/ConfBase.pm
+++ b/lib/Sbuild/ConfBase.pm
@@ -502,7 +502,12 @@ sub read ($$$$) {
        next if $conf->_get_group($key) =~ m/^__/;
 
        my $varname = $conf->_get_varname($key);
-       $script .= "my \$$varname = undef;\n";
+        my $vardefault = $conf->_get_default($key);
+        if (defined $vardefault) {
+           $script .= "my " . Data::Dumper->Dump([$vardefault], [$varname]);
+        } else {
+            $script .= "my \$$varname = undef;\n";
+        }
     }
 
     # For compatibility only.  Non-scalars are deprecated.

But also stated:

> I would bet a lot on this breaking something. The problem are
> settings which have a GET function which is allowed to perform
> some magic. The GET function is called if the value behind a
> setting is requested but the user has set it to undef in their
> ~/.sbuildrc. If all values that have defaults are now set, then
> the GET function is not called anymore which I guess can have
> undesired consequences in some cases? Can you try this patch and
> see what happens?

Chris

[toc] | [next] | [standalone]

#1224461

On Tue, Dec 17, 2024 at 02:20:16PM +0100, Chris Hofstaedtler wrote:
> josch@ wrote on salsa:
> > I would bet a lot on this breaking something. The problem are
> > settings which have a GET function which is allowed to perform
> > some magic.

> > Can you try this patch and see what happens?

I have to say that results in some "interesting" breakage.

For one, autopkgtest is unhappy. Running
   sbuild -s --no-clean-source -d unstable hello
gives:

| autopkgtest
| -----------
| 
| autopkgtest [14:56:35]: starting date and time: 2024-12-17 14:56:35+0100
| autopkgtest [14:56:35]: version 5.42
| autopkgtest [14:56:35]: host tiksta; command line: /usr/bin/autopkgtest /home/ch/Debian/tryout/hello_2.10-3_arm64.changes --apt-upgrade -- unshare --release unstable --arch arm64
| autopkgtest [14:56:36]: testbed dpkg architecture: arm64
| autopkgtest [14:56:36]: testbed apt version: 2.9.17
| autopkgtest [14:56:36]: @@@@@@@@@@@@@@@@@@@@ test bed setup
| autopkgtest [14:56:36]: testbed release detected to be: None
| autopkgtest [14:56:36]: updating testbed package index (apt update)
| Get:1 http://deb.debian.org/debian unstable InRelease [202 kB]
| Get:2 http://deb.debian.org/debian unstable/main arm64 Packages [9977 kB]
| Get:3 http://deb.debian.org/debian unstable/main Translation-en [7349 kB]
| Fetched 17.5 MB in 2s (11.6 MB/s)
| Reading package lists...
| autopkgtest [14:56:38]: upgrading testbed (apt dist-upgrade and autopurge)
| Reading package lists...
| Building dependency tree...
| Reading state information...
| Calculating upgrade...Starting pkgProblemResolver with broken count: 0
| Starting 2 pkgProblemResolver with broken count: 0
| Done
| Entering ResolveByKeep
| 
| 0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
| Reading package lists...
| Building dependency tree...
| Reading state information...
| Starting pkgProblemResolver with broken count: 0
| Starting 2 pkgProblemResolver with broken count: 0
| Done
| 0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
| autopkgtest [14:56:39]: testbed running kernel: Linux 6.11.10-arm64 #1 SMP Debian 6.11.10-1 (2024-11-23)
| autopkgtest [14:56:39]: @@@@@@@@@@@@@@@@@@@@ source /home/ch/Debian/tryout/hello_2.10-3.dsc
| Unexpected error:
| Traceback (most recent call last):
|   File "/usr/share/autopkgtest/lib/VirtSubproc.py", line 833, in mainloop
|     command()
|   File "/usr/share/autopkgtest/lib/VirtSubproc.py", line 762, in command
|     r = f(c, ce)
|         ^^^^^^^^
|   File "/usr/share/autopkgtest/lib/VirtSubproc.py", line 696, in cmd_copydown
|     copyupdown(c, ce, False)
|   File "/usr/share/autopkgtest/lib/VirtSubproc.py", line 584, in copyupdown
|     copyupdown_internal(ce[0], c[1:], upp)
|   File "/usr/share/autopkgtest/lib/VirtSubproc.py", line 611, in copyupdown_internal
|     copydown_shareddir(sd[0], sd[1], dirsp, downtmp_host)
|   File "/usr/share/autopkgtest/lib/VirtSubproc.py", line 566, in copydown_shareddir
|     shutil.copy(host, host_tmp)
|   File "/usr/lib/python3.12/shutil.py", line 435, in copy
|     copyfile(src, dst, follow_symlinks=follow_symlinks)
|   File "/usr/lib/python3.12/shutil.py", line 260, in copyfile
|     with open(src, 'rb') as fsrc:
|          ^^^^^^^^^^^^^^^
| FileNotFoundError: [Errno 2] No such file or directory: '/home/ch/Debian/tryout/hello_2.10.orig.tar.gz'
| autopkgtest [14:56:39]: ERROR: testbed failure: unexpected eof from the testbed
| 
| E: Autopkgtest run failed.

Which is certainly weird. I think this might be some interaction
between -s and $source_only_changes=1. I don't have any more details
yet, need to dig into testing/diffing logs more.

Chris

[toc] | [prev] | [next] | [standalone]

#1224462

On Tue, Dec 17, 2024 at 03:03:44PM +0100, Chris Hofstaedtler wrote:
> > > Can you try this patch and see what happens?
> 
> For one, autopkgtest is unhappy. Running
>    sbuild -s --no-clean-source -d unstable hello
> gives:
> 
[..]
> | FileNotFoundError: [Errno 2] No such file or directory: '/home/ch/Debian/tryout/hello_2.10.orig.tar.gz'
> | autopkgtest [14:56:39]: ERROR: testbed failure: unexpected eof from the testbed
> | 
> | E: Autopkgtest run failed.

The good news is: this also happens with sbuild 0.88.1 from
unstable. The bad news is: this happens ;-)

Chris

[toc] | [prev] | [next] | [standalone]

#1224464

[Multipart message — attachments visible in raw view] — view raw

Hi,

Quoting Chris Hofstaedtler (2024-12-17 14:20:16)
> I wish sbuild would allow me to extend its config settings in
> .sbuildrc or .config/sbuild/config.pl, without having to re-state
> the previous defaults.

me too.

> This bug report is mostly here to escape the salsa comment mess;

Thank you! I also prefer the BTS for this. :)

> > I would bet a lot on this breaking something. The problem are
> > settings which have a GET function which is allowed to perform
> > some magic. The GET function is called if the value behind a
> > setting is requested but the user has set it to undef in their
> > ~/.sbuildrc. If all values that have defaults are now set, then
> > the GET function is not called anymore which I guess can have
> > undesired consequences in some cases? Can you try this patch and
> > see what happens?

The question is how to proceed. I fear that things will break. But I tested
this and nothing broke. But I know that people are *very* creative when it
comes to their ~/.sbuildrc... I fear that the only way to find out what breaks
is to upload and break things... :/

Thanks!

cheers, josch

[toc] | [prev] | [next] | [standalone]

#1224469

On Tue, Dec 17, 2024 at 03:18:41PM +0100, Johannes Schauer Marin Rodrigues wrote:
> > > I would bet a lot on this breaking something. The problem are
> > > settings which have a GET function which is allowed to perform
> > > some magic. The GET function is called if the value behind a
> > > setting is requested but the user has set it to undef in their
> > > ~/.sbuildrc. If all values that have defaults are now set, then
> > > the GET function is not called anymore which I guess can have
> > > undesired consequences in some cases? Can you try this patch and
> > > see what happens?
> 
> The question is how to proceed. I fear that things will break. But I tested
> this and nothing broke. But I know that people are *very* creative when it
> comes to their ~/.sbuildrc... I fear that the only way to find out what breaks
> is to upload and break things... :/

I fear that is indeed the only way to find that out. :/

Chris

[toc] | [prev] | [next] | [standalone]

#1224518

Hi,

* Johannes Schauer Marin Rodrigues <josch@debian.org> [241217 15:18]:
> Quoting Chris Hofstaedtler (2024-12-17 14:20:16)
> > > I would bet a lot on this breaking something. The problem are
> > > settings which have a GET function which is allowed to perform
> > > some magic. The GET function is called if the value behind a
> > > setting is requested but the user has set it to undef in their
> > > ~/.sbuildrc. If all values that have defaults are now set, then
> > > the GET function is not called anymore which I guess can have
> > > undesired consequences in some cases? Can you try this patch and
> > > see what happens?
> 
> The question is how to proceed. I fear that things will break. But I tested
> this and nothing broke.

Please test this scenario, and see if that works for you / what
happens:

cd /var/tmp
apt source hello
cd hello-2.10
sbuild --chroot-mode=unshare --no-run-lintian
test -e hello_*changes && echo "whoops, wrote to wrong dir"

Chris

[toc] | [prev] | [next] | [standalone]

#1224532

[Multipart message — attachments visible in raw view] — view raw

Hi,

Quoting Chris Hofstaedtler (2024-12-17 22:09:58)
> Please test this scenario, and see if that works for you / what happens:
> 
> cd /var/tmp
> apt source hello
> cd hello-2.10
> sbuild --chroot-mode=unshare --no-run-lintian
> test -e hello_*changes && echo "whoops, wrote to wrong dir"

thank you. This is because of the convenience feature that sbuild gained in
2011 with commit 601af3d6f3a7028b88e1b88e7e3afeeeef4877bb. If you run sbuild
inside an unpacked source directory, then sbuild will automatically run
"dpkg-source -b ." for you and place the build artifacts in the parent.  It
does so by changing the working directory of the process to the parent and
setting the *default* of BUILD_DIR to that directory. But at that point,
the ~/.sbuildrc has long since been evaluated. And at the point where it
got evaluated, the working directory was still the unpacked source
directory and not its parent. The patch in this bug report sets the value
of BUILD_DIR to the unpacked source dir and thus, changing the default of
BUILD_DIR later on has no effect anymore because the value was already
set to the default from back then...

The fix is not trivial considering that we don't want the fix to this to break
more things...

Thanks!

cheers, josch

[toc] | [prev] | [next] | [standalone]

#1224707

* Johannes Schauer Marin Rodrigues <josch@debian.org> [241217 23:59]:
> The patch in this bug report sets the value
> of BUILD_DIR to the unpacked source dir and thus, changing the default of
> BUILD_DIR later on has no effect anymore because the value was already
> set to the default from back then...
> 
> The fix is not trivial considering that we don't want the fix to this to break
> more things...

I haven't yet looked at what code touches BUILD_DIR exactly, but an
interim thing that one could ponder:

  --- a/lib/Sbuild/ConfBase.pm
  +++ b/lib/Sbuild/ConfBase.pm
  @@ -502,7 +502,13 @@ sub read ($$$$) {
          next if $conf->_get_group($key) =~ m/^__/;
  
          my $varname = $conf->_get_varname($key);
  -       $script .= "my \$$varname = undef;\n";
  +        my $vardefault = $conf->_get_default($key);
  +        my $varget = $conf->_get_property_value($key, 'DEFAULT');
  +        if (!defined $varget and defined $vardefault) {
  +            $script .= "my " . Data::Dumper->Dump([$vardefault], [$varname]);
  +        } else {
  +            $script .= "my \$$varname = undef;\n";
  +        }
       }
  
       # For compatibility only.  Non-scalars are deprecated.

This may be wrong for other reasons.

But if that works, afterwards 'GET'-functions could maybe be
deprecated altogether? (This is unfounded speculation; I really need
to look at Conf.pm and see what these things all do.)

Chris
  

[toc] | [prev] | [next] | [standalone]

#1224709

* Chris Hofstaedtler <zeha@debian.org> [241218 08:48]:
> * Johannes Schauer Marin Rodrigues <josch@debian.org> [241217 23:59]:
> > The patch in this bug report sets the value
> > of BUILD_DIR to the unpacked source dir and thus, changing the default of
> > BUILD_DIR later on has no effect anymore because the value was already
> > set to the default from back then...
> > 
> > The fix is not trivial considering that we don't want the fix to this to break
> > more things...
> 
> I haven't yet looked at what code touches BUILD_DIR exactly, but an
> interim thing that one could ponder:
> 
>   --- a/lib/Sbuild/ConfBase.pm
>   +++ b/lib/Sbuild/ConfBase.pm
>   @@ -502,7 +502,13 @@ sub read ($$$$) {
>           next if $conf->_get_group($key) =~ m/^__/;
>   
>           my $varname = $conf->_get_varname($key);
>   -       $script .= "my \$$varname = undef;\n";
>   +        my $vardefault = $conf->_get_default($key);
>   +        my $varget = $conf->_get_property_value($key, 'DEFAULT');
>   +        if (!defined $varget and defined $vardefault) {
>   +            $script .= "my " . Data::Dumper->Dump([$vardefault], [$varname]);
>   +        } else {
>   +            $script .= "my \$$varname = undef;\n";
>   +        }
>        }
>   
>        # For compatibility only.  Non-scalars are deprecated.
> 
> This may be wrong for other reasons.

It is. Not sure why my config still evaluates at all.

It it was just about BUILD_DIR, maybe we can get away with ignoring
vars that have IGNORE_DEFAULT => 1?

Chris

[toc] | [prev] | [next] | [standalone]

#1224754

[Multipart message — attachments visible in raw view] — view raw

Hi,

Quoting Chris Hofstaedtler (2024-12-18 09:12:08)
> It it was just about BUILD_DIR, maybe we can get away with ignoring vars that
> have IGNORE_DEFAULT => 1?

that would overload the meaning of a setting that is intended for a completely
different purpose. How about this instead:


--- a/lib/Sbuild/Conf.pm
+++ b/lib/Sbuild/Conf.pm
@@ -904,7 +904,16 @@ $unshare_mmdebstrap_extra_args = [
            TYPE => 'STRING',
            VARNAME => 'build_dir',
            GROUP => 'Core options',
-           DEFAULT => cwd(),
+           DEFAULT => undef,
+           GET => sub {
+               my $conf = shift;
+               my $entry = shift;
+               my $retval = $conf->_get($entry->{'NAME'});
+               if (!defined($retval)) {
+                   $retval = cwd();
+               }
+               return $retval;
+           },
            IGNORE_DEFAULT => 1, # Don't dump class to config
            EXAMPLE => '$build_dir = \'/home/pete/build\';',
            CHECK => $validate_directory,


Thanks!

cheers, josch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.bugs.dist

csiph-web