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

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

Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet

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

Contents

  Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet Anthony Fok <foka@debian.org> - 2020-07-17 22:20 +0200
    Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet "Gabriel F. T. Gomes" <gabriel@inconstante.net.br> - 2020-07-28 03:20 +0200
      Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet Sergio Durigan Junior <sergiodj@debian.org> - 2020-07-31 06:00 +0200
        Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet "Gabriel F. T. Gomes" <gabriel@inconstante.net.br> - 2020-07-31 19:00 +0200
          Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet Sergio Durigan Junior <sergiodj@debian.org> - 2020-07-31 20:00 +0200
            Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet "Gabriel F. T. Gomes" <gabriel@inconstante.net.br> - 2020-07-31 21:10 +0200

#1018374 — Bug#965225: bash-completion: dh_bash-completion at debhelper-compat (= 13) chokes on "${foo}" in proper snippet

Package: bash-completion
Version: 1:2.10-1
Severity: normal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hello,

I ran into the following error while packaging the latest version of
hugo package:

    dh_bash-completion: error: Cannot resolve variable "${BASH_COMP_DEBUG_FILE}" in debian/hugo.bash-completion (line 5)

where debian/hugo.bash-completion is a proper completion snippet.

It turns out that the error was triggered when I bumped
"debhelper-compat (= 12)" to "debhelper-compat (= 13)" in debian/control.

This is a behavioural change in debhelper v13.  From debhelper(7):

  - Many dh_* tools now support limited variable expansion via
    the ${foo} syntax.  In many cases, this can be used to
    reference paths that contain either spaces or
    dpkg-architecture(1) values.  While this can reduce the
    need for dh-exec(1) in some cases, it is not a replacement
    dh-exec(1) in general.  If you need filtering, renaming,
    etc., the package will still need dh-exec(1).

    Please see "Substitutions in debhelper config files" for
    syntax and available substitution variables.  To dh_* tool
    writers, substitution expansion occurs as a part of the
    filearray and filedoublearray functions.

dh_bash-completion currently uses filedoublearray() to detect if
debian/package.bash-completion is a list of files or not.
While it has worked well in the past, it has now become a lot more
flakey and error-prone as debhelper v13+ now always attempt
such variable substitution.

While one could try to work around the issue by changing
"${foo}" to "$foo" in the proper completion snippet,
quoting variables with curly braces like ${foo} is nonetheless
perfectly valid bash syntax, and it is often unavoidable
in cases such as "${foo}bar" or "${foo}_bar".

Perhaps there are better ways to distinguish whether
debian/package.bash-completion is a file list or proper snippet
than sending it to filedoublearray() and see if it fails or not?

Cheers,

Anthony

- -- System Information:
Debian Release: bullseye/sid
  APT prefers unstable
  APT policy: (500, 'unstable'), (1, 'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 5.7.0-1-amd64 (SMP w/4 CPU threads)
Kernel taint flags: TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=en_CA.UTF-8, LC_CTYPE=en_CA.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

- -- no debconf information

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEFCQhsZrUqVmW+VBy6iUAtBLFms8FAl8SBnIACgkQ6iUAtBLF
ms9mWw//V/Z4sOwhrWnHT/L2KTKf5IgiK8k+1PlWL1OQXfpAJxUWXUeBSL07mxlW
R3MXl0cef3s7ShvHBpFtJLk19cXWvfeGY8GHtXf3XNxvKCz3kKQsETNZH7BibMqg
Npt1ZyfWDwilxTpB8eWJ+ZdBT+vlfi57G1tvhdUEvkmfRw9HU7R0ephySuGCWAdc
3e2NoaI7OyVQ/WzkgcdW44ZeaC4GUkBgoHJiF1E+22K/fgMV95vqyUtr8nh8Kz5q
W1Px7CafdDPuMT0vxxMu/YWfVOApRKssKOqoVvxwV1MBzPXCau3AjcHe3Z84W9Tc
6ko3hm9tMUZGUMuaVE+GV11wL+mEfB5u7ZvRG1SADmcyFLR5mjdkgp89LroKe4Dx
TQT609B9nPYz85ROTVhDkKR5jOU1CInObTUnlW+g1/xXb/kwyKl/Qt50DfD7Du45
CmDiPEWoc0Ptn9LoXUYO7MoEXit3XyOYJYhOS5jsgqpgvCJjFnpl7IR6jFtPfVGN
0dQDAf7+FXT1CBmaCMevPX9BZnfPpwbbn41lqsoK3eq7IiWCniUtqWVMRGs2OwPV
JvOVIFxQsS1ENzU38m4wTQPwGOgmA6Xz1J5FV6j6ItpDS+ZaOvyYXk0Ab9JMb10L
3SaZfv8BbsBIBjtF6zgPqv/cyXTAfIaUwXF/skSgfbE6yf1US1Y=
=Ez1T
-----END PGP SIGNATURE-----

[toc] | [next] | [standalone]

#1019543

Control: severity -1 important
Control: tags -1 + confirmed

Hi, Anthony,

thanks for the report!

On 17 Jul 2020, Anthony Fok wrote:

>I ran into the following error while packaging the latest version of
>hugo package:
>
>    dh_bash-completion: error: Cannot resolve variable "${BASH_COMP_DEBUG_FILE}" in debian/hugo.bash-completion (line 5)
>
>where debian/hugo.bash-completion is a proper completion snippet.

How to reproduce (note to myself):

  On a system with debhelper >= 13:
  $ git clone https://salsa.debian.org/go-team/packages/hugo.git
  $ cd hugo
  # apt install hugo
  $ hugo gen autocomplete --completionfile=debian/hugo.bash-completion
  $ dh_bash-completion 
  dh_bash-completion: error: Cannot resolve variable "${BASH_COMP_DEBUG_FILE}" in debian/hugo.bash-completion (line 5)

>It turns out that the error was triggered when I bumped
>"debhelper-compat (= 12)" to "debhelper-compat (= 13)" in debian/control.
>
>This is a behavioural change in debhelper v13.  From debhelper(7):
>
>  - Many dh_* tools now support limited variable expansion via
>    the ${foo} syntax.  In many cases, this can be used to
>    reference paths that contain either spaces or
>    dpkg-architecture(1) values.  While this can reduce the
>    need for dh-exec(1) in some cases, it is not a replacement
>    dh-exec(1) in general.  If you need filtering, renaming,
>    etc., the package will still need dh-exec(1).
>
>    Please see "Substitutions in debhelper config files" for
>    syntax and available substitution variables.  To dh_* tool
>    writers, substitution expansion occurs as a part of the
>    filearray and filedoublearray functions.
>
>dh_bash-completion currently uses filedoublearray() to detect if
>debian/package.bash-completion is a list of files or not.
>While it has worked well in the past, it has now become a lot more
>flakey and error-prone as debhelper v13+ now always attempt
>such variable substitution.

Thanks for the analysis. :)

>While one could try to work around the issue by changing
>"${foo}" to "$foo" in the proper completion snippet,
>quoting variables with curly braces like ${foo} is nonetheless
>perfectly valid bash syntax, and it is often unavoidable
>in cases such as "${foo}bar" or "${foo}_bar".

I agree, it would be terrible to forbid scripts from using it. 

>Perhaps there are better ways to distinguish whether
>debian/package.bash-completion is a file list or proper snippet
>than sending it to filedoublearray() and see if it fails or not?

I really suck at perl programming, but, as far as I can tell, that's
not the only reason to call filedoublearray(). dh_bash-completion uses
the *output* of filedoublearray(), not its return code, to detect
whether the file is a proper snippet. However, since debhelper 13,
filedoublearray() fails during variable expansion (if we track the
failure down, it happens in _variable_substitution()), so
dh_bash-completion doesn't actually get a chance to check if the file
is a proper snippet or a list of files.

I'll try and find a solution for this, but I might need help from perl
experts. :)

Cheers,
Gabriel

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

#1019885

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

On Monday, July 27 2020, Gabriel F. T. Gomes wrote:

> On 17 Jul 2020, Anthony Fok wrote:
>
>>I ran into the following error while packaging the latest version of
>>hugo package:
>>
>>    dh_bash-completion: error: Cannot resolve variable "${BASH_COMP_DEBUG_FILE}" in debian/hugo.bash-completion (line 5)
>>
>>where debian/hugo.bash-completion is a proper completion snippet.
>
> How to reproduce (note to myself):
>
>   On a system with debhelper >= 13:
>   $ git clone https://salsa.debian.org/go-team/packages/hugo.git
>   $ cd hugo
>   # apt install hugo
>   $ hugo gen autocomplete --completionfile=debian/hugo.bash-completion
>   $ dh_bash-completion 
>   dh_bash-completion: error: Cannot resolve variable "${BASH_COMP_DEBUG_FILE}" in debian/hugo.bash-completion (line 5)

Thanks for this :-).

>>While one could try to work around the issue by changing
>>"${foo}" to "$foo" in the proper completion snippet,
>>quoting variables with curly braces like ${foo} is nonetheless
>>perfectly valid bash syntax, and it is often unavoidable
>>in cases such as "${foo}bar" or "${foo}_bar".
>
> I agree, it would be terrible to forbid scripts from using it. 

+1.

IMHO, the problem here was that debhelper chose to use the ${}-format
without considering dh_bash-completion's case.  Of course, with the
ever-growing number of debhelper scripts it's impossible to take every
corner case into account, even though ${} is a pretty overloaded format
for addressing variables ;-).

When I read this bug the first time I thought that it might have been
good if they'd provided a filedoublearray_noexpand or some such, but
then it occurred to me that we do want to support debhelper variables
inside file-lists, so...

>>Perhaps there are better ways to distinguish whether
>>debian/package.bash-completion is a file list or proper snippet
>>than sending it to filedoublearray() and see if it fails or not?
>
> I really suck at perl programming, but, as far as I can tell, that's
> not the only reason to call filedoublearray(). dh_bash-completion uses
> the *output* of filedoublearray(), not its return code, to detect
> whether the file is a proper snippet. However, since debhelper 13,
> filedoublearray() fails during variable expansion (if we track the
> failure down, it happens in _variable_substitution()), so
> dh_bash-completion doesn't actually get a chance to check if the file
> is a proper snippet or a list of files.
>
> I'll try and find a solution for this, but I might need help from perl
> experts. :)

I'm far from being a Perl expert, but I *think* I came up with a
solution.

So, here's the thing.  We can't blindly rely on debhelper's
filedoublearray anymore, because of the problem you guys pointed out
above.  Which means that bash-completion will probably have to have its
own stripped-down, poor-man's version of filedoublearray.  Actually,
given the way dh_bash-completion works, it should be enough to have a
function that tries to determine whether the file being examined is (a)
a bash-completion script, or (b) a file-list.  If (a), then the file
itself should be installed.  If (b), then we install each file listed in
it.

In order to hack this new function I used a little bit of what
filedoublearray does in the beginning, and then I crafted a few regexes
that will perform a "heuristic" to see if we catch some well-known bash
constructions in the file.  If we succeed, then just assume that the
file is a bash-completion script and be done with it.  Otherwise, it's
(probably) a file-list.

Now, in order to test this approach, here's what I did (inside a schroot
session):

- Go to sources.d.o, look for all the packages that depend on
  bash-completion.

- Download the list, iterate over it and do an "apt source" on all of
  them.

- Check for a few interesting cases in this list.  Examples I could find
  are: consul, cargo, caffe, 2ping, xkcdpass, virtualenvwrapper.

- Enter their directories and perform a "dh_bash-completion -v".

As far as I have tested, everything works OK.  Of course, this is a
heuristic approach and it is possible to craft a problematic file that
will cause an error, but it's better than what we have now, IMHO.

BTW: while I was hacking I had another idea, which is to use a
try...catch block around filedoublearray and see if it "throws" anything
like ".*cannot resolve variable.*", but that doesn't really work: if the
error is triggered, it might mean that we're dealing with a
bash-completion script, *but* it might also mean that the file-list file
is broken (e.g., referencing a wrong or non-existent variable), which
means that we would need to perform some kind of parsing to determine
what's really happening anyway (assuming that we want to do a good job
at detecting the error).

Hopefully this will help.  I'm not tagging this as "+patch" because I'd
like to hear your opinions first.

Cheers,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

diff --git a/debian/extra/debhelper/dh_bash-completion b/debian/extra/debhelper/dh_bash-completion
index f96704b..ffc1854 100755
--- a/debian/extra/debhelper/dh_bash-completion
+++ b/debian/extra/debhelper/dh_bash-completion
@@ -33,6 +33,60 @@ completion snippet after. The file format is as follows:
 
 =cut
 
+# This helper function tries to determine (using some poor man's
+# heuristics) whether $file (its first and only argument) is a
+# filelist containing a list of files to be installed by us, or a
+# bash-completion script, which should itself be installed.
+#
+# If we're dealing with a filelist, return 1.  Otherwise, return 0.
+sub is_filelist {
+	# The file to be checked.
+	my ($file) = @_;
+
+	open (DH_FILE, '<', $file) || error("cannot read $file: $!");
+
+	while (<DH_FILE>) {
+		# Get rid of lines containing just spaces or comments.
+		chomp;
+		s/^\s++//;
+		next if /^#/;
+		s/\s++$//;
+
+		# We always ignore/permit empty lines
+		next if $_ eq '';
+
+		# This is the heart of the script.  Here, we check for some
+		# well-known idioms on bash scripts, and try to determine if
+		# they're present in the file we're examining.  We assume that
+		# if they are, then this means the file is a bash-completion
+		# script.
+		#
+		# The regexes check:
+		#
+		# - If we're calling the bash function "complete", which is a
+		#   pretty common thing to do in bash-completion scripts, or
+		#
+		# - If we're using the $(program) way of executing a program.
+		#   We don't take into account multi-line statements.  Or
+		#
+		# - If we're calling the bash function "compgen", which is
+		#   also a pretty common thing that bash-completion scripts
+		#   do.  Or
+		#
+		# - If we see an "if...then" construction in the file.  We
+		#   take into account multi-line statements.
+		if (/\s*complete.*-[A-Za-z].*/
+			|| /\$\(.*\)/
+			|| /\s*compgen.*-[A-Za-z].*/
+			|| /\s*if.*;.*then/s) {
+			return 0;
+		}
+	}
+
+	# If we reached the end, this is not a bash-completion script.
+	return 1;
+}
+
 init();
 
 my $srcdir = '.';
@@ -53,6 +107,15 @@ PKG: foreach my $package (@{$dh{DOPACKAGES}}) {
 	if ($completions) {
 		install_dir($bc_dir);
 
+		# Invoke our heuristic function to try and determine
+		# if we're dealing with a filelist or with a
+		# bash-completion script.
+		if (!is_filelist($completions)) {
+			verbose_print "detected $completions as a bash-completion script";
+			install_file($completions, "$bc_dir/$package");
+			next PKG
+		}
+
 		# try parsing a list of files
 		@install = filedoublearray($completions);
 		foreach my $set (@install) {

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

#1019976

Control: tags -1 + patch

Hi, Sergio,

<3 S2 <3 S2

On Thu, 30 Jul 2020, Sergio Durigan Junior wrote:
> 
> So, here's the thing.  We can't blindly rely on debhelper's
> filedoublearray anymore, because of the problem you guys pointed out
> above.  Which means that bash-completion will probably have to have its
> own stripped-down, poor-man's version of filedoublearray.  Actually,
> given the way dh_bash-completion works, it should be enough to have a
> function that tries to determine whether the file being examined is (a)
> a bash-completion script, or (b) a file-list.  If (a), then the file
> itself should be installed.  If (b), then we install each file listed in
> it.

I really like this approach. It lets filedoublearray be free to do
whatever it wants, while still working for bash-completion on the file
list case.

> In order to hack this new function I used a little bit of what
> filedoublearray does in the beginning, and then I crafted a few regexes
> that will perform a "heuristic" to see if we catch some well-known bash
> constructions in the file.  If we succeed, then just assume that the
> file is a bash-completion script and be done with it.  Otherwise, it's
> (probably) a file-list.

I like the heuristic you came up with; it should be enough to cover all
completion files I have seen so far and it is so well-documented that
it should be easy to fix if problems show up.

> Now, in order to test this approach, here's what I did (inside a schroot
> session):
> 
> - Go to sources.d.o, look for all the packages that depend on
>   bash-completion.
> 
> - Download the list, iterate over it and do an "apt source" on all of
>   them.
> 
> - Check for a few interesting cases in this list.  Examples I could find
>   are: consul, cargo, caffe, 2ping, xkcdpass, virtualenvwrapper.
> 
> - Enter their directories and perform a "dh_bash-completion -v".

Thanks for being so careful about testing and for sharing the steps
with us. I'll adopt it for my workflows.

> As far as I have tested, everything works OK.  Of course, this is a
> heuristic approach and it is possible to craft a problematic file that
> will cause an error, but it's better than what we have now, IMHO.

+1

I have tested a couple of reverse-build-dependencies of
bash-completion. Most of them use file lists and the patched debhelper
script works as before when compat level is updated to 13. I also found
one package that has a proper completion snippet (pidcat), which also
works correctly with the patched debhelper script and debhelper 13.

I haven't tested all reverse build dependencies, though. The list is
too large.

> BTW: while I was hacking I had another idea, which is to use a
> try...catch block around filedoublearray and see if it "throws" anything
> like ".*cannot resolve variable.*", but that doesn't really work: if the
> error is triggered, it might mean that we're dealing with a
> bash-completion script, *but* it might also mean that the file-list file
> is broken (e.g., referencing a wrong or non-existent variable), which
> means that we would need to perform some kind of parsing to determine
> what's really happening anyway (assuming that we want to do a good job
> at detecting the error).

thanks for sharing. :)

> Hopefully this will help.  I'm not tagging this as "+patch" because I'd
> like to hear your opinions first.

Now tagged.

I'll incorporate this change to the repository, do a little more testing,
then upload to unstable. May I set git commit authorship to
"Sergio Durigan Junior <sergiodj@debian.org>" ?


Cheers,
Gabriel

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

#1019979

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

On Friday, July 31 2020, Gabriel F. T. Gomes wrote:

> Hi, Sergio,
>
> <3 S2 <3 S2

:-D

> On Thu, 30 Jul 2020, Sergio Durigan Junior wrote:
>> 
>> So, here's the thing.  We can't blindly rely on debhelper's
>> filedoublearray anymore, because of the problem you guys pointed out
>> above.  Which means that bash-completion will probably have to have its
>> own stripped-down, poor-man's version of filedoublearray.  Actually,
>> given the way dh_bash-completion works, it should be enough to have a
>> function that tries to determine whether the file being examined is (a)
>> a bash-completion script, or (b) a file-list.  If (a), then the file
>> itself should be installed.  If (b), then we install each file listed in
>> it.
>
> I really like this approach. It lets filedoublearray be free to do
> whatever it wants, while still working for bash-completion on the file
> list case.

Yep, that's the idea.  Avoid calling filedoublearray if we already know
what we're dealing with.

>> In order to hack this new function I used a little bit of what
>> filedoublearray does in the beginning, and then I crafted a few regexes
>> that will perform a "heuristic" to see if we catch some well-known bash
>> constructions in the file.  If we succeed, then just assume that the
>> file is a bash-completion script and be done with it.  Otherwise, it's
>> (probably) a file-list.
>
> I like the heuristic you came up with; it should be enough to cover all
> completion files I have seen so far and it is so well-documented that
> it should be easy to fix if problems show up.

Cool!  I wasn't sure if you'd like it; it's a bit hacky ;-).  But yeah,
I tried to be careful with the comments because I know that there's a
chance that the function might need to be expanded in order to
accomodate other cases.

>> As far as I have tested, everything works OK.  Of course, this is a
>> heuristic approach and it is possible to craft a problematic file that
>> will cause an error, but it's better than what we have now, IMHO.
>
> +1
>
> I have tested a couple of reverse-build-dependencies of
> bash-completion. Most of them use file lists and the patched debhelper
> script works as before when compat level is updated to 13. I also found
> one package that has a proper completion snippet (pidcat), which also
> works correctly with the patched debhelper script and debhelper 13.
>
> I haven't tested all reverse build dependencies, though. The list is
> too large.

Yeah, the list is large, indeed.  I guess we'll find out whether this
breaks something or not when it reaches unstable and starts to be used
:-).

>> Hopefully this will help.  I'm not tagging this as "+patch" because I'd
>> like to hear your opinions first.
>
> Now tagged.
>
> I'll incorporate this change to the repository, do a little more testing,
> then upload to unstable. May I set git commit authorship to
> "Sergio Durigan Junior <sergiodj@debian.org>" ?

Sure thing!  Feel free to mention in the d/changelog entry as well.

Thanks :-).

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
https://sergiodj.net/

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

#1019984

On Fri, 31 Jul 2020, Sergio Durigan Junior wrote:
>
> Yeah, the list is large, indeed.  I guess we'll find out whether this
> breaks something or not when it reaches unstable and starts to be used
> :-).

And if it breaks *before* people update debhelper's compat level, then
we know we broke it backwards, hehehe. I believe we haven't. :)


> > May I set git commit authorship to
> > "Sergio Durigan Junior <sergiodj@debian.org>" ?  
> 
> Sure thing!  Feel free to mention in the d/changelog entry as well.

Done. :)
https://salsa.debian.org/debian/bash-completion/-/commit/9230db41952b8cf95e3b814455c7f268d485829a


I'm no Queen, but I now make you the Earl of Perl! :)
Thank you very much.

[toc] | [prev] | [standalone]

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

csiph-web