pmem / issues

Old issues repo for PMDK.
http://pmem.io
13 stars 7 forks source link

Drop interpreter from bash completion #860

Closed panlinux closed 6 years ago

panlinux commented 6 years ago

While packaging pmdk 1.4, I got this lintian error about src/tools/pmempool/pmempool.sh:

https://lintian.debian.org/tags/script-not-executable.html

W: pmdk-tools: script-not-executable usr/share/bash-completion/completions/pmempool
N: 
N:    This file starts with the #! sequence that marks interpreted scripts,
N:    but it is not executable.
N:    
N:    Severity: normal, Certainty: certain
N:    
N:    Check: scripts, Type: binary

Turns out bash completion scripts should not have the interpreter line, and that was the case with the nvml 1.3.1 release. If you check all the files in /usr/share/bash-completion/completions/, none have it, and none are executable either.

So I propose to simply drop the interpreter line from src/tools/pmempool/pmempool.sh:

--- a/src/tools/pmempool/pmempool.sh
+++ b/src/tools/pmempool/pmempool.sh
@@ -1,4 +1,3 @@
-#!/usr/bin/env bash
 #
 # Copyright 2014-2017, Intel Corporation
 #
marcinslusarz commented 6 years ago

It was added by @gaweinbergi in pmem/pmdk#2133. Glenn: any comment on the proposed change?

gaweinbergi commented 6 years ago

If the file is truly not executable, then it's fine not to have the #! line - although I'd also suggest executable and non-executable scripts should not both use the .sh suffix.

panlinux commented 6 years ago

Correct about the .sh suffix. I forgot to mention that I'm installing the file without it, otherwise bash completion won't source the file.

In the "old" location in /etc/bash_completion.d, the .sh suffix was allowed (as was any other), but in the new recommended path at /usr/share/bash-completion/completions/ that's not the case. There, only .bash or no extension at all work. See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=814599

marcinslusarz commented 6 years ago

Thanks guys, @plebioda volunteered to fix this (drop .sh suffix, drop shebang & fix installation)

marcinslusarz commented 6 years ago

Merged to master and stable-1.4. Will be part of 1.4.1 release.