[pacman-dev] Scripts bombing with no args
Allan or Dave, mind taking a look at this? We should show usage info, as rankmirrors and pacman-key do without args, instead of this silly behavior. -Dan dmcgee@galway ~/projects/pacman (master) $ ./scripts/repo-add ==> ERROR: '' does not have a valid archive extension. dmcgee@galway ~/projects/pacman (master) $ ./scripts/pkgdelta ./scripts/pkgdelta: line 147: $1: unbound variable
On Thu, Jul 28, 2011 at 11:18:15AM -0500, Dan McGee wrote:
Allan or Dave, mind taking a look at this? We should show usage info, as rankmirrors and pacman-key do without args, instead of this silly behavior.
-Dan
dmcgee@galway ~/projects/pacman (master) $ ./scripts/repo-add ==> ERROR: '' does not have a valid archive extension.
We never check for $1 being unset before proceeding. Probably my mistake when I did some refactoring of repo-add.
dmcgee@galway ~/projects/pacman (master) $ ./scripts/pkgdelta ./scripts/pkgdelta: line 147: $1: unbound variable
'set -u' with 'set -e'? Ugly... I've got fixes for both on my 'scripts-fixup' branch https://github.com/falconindy/pacman/commit/e99b6a131ea08829da26d62d498c33f3... https://github.com/falconindy/pacman/commit/e42d97b7370c9e30d9ae66a33f42d274... I can squash these into a single commit as well, though the fixes aren't really related. d
On 29/07/11 03:14, Dave Reisner wrote:
On Thu, Jul 28, 2011 at 11:18:15AM -0500, Dan McGee wrote:
Allan or Dave, mind taking a look at this? We should show usage info, as rankmirrors and pacman-key do without args, instead of this silly behavior.
-Dan
dmcgee@galway ~/projects/pacman (master) $ ./scripts/repo-add ==> ERROR: '' does not have a valid archive extension.
We never check for $1 being unset before proceeding. Probably my mistake when I did some refactoring of repo-add.
dmcgee@galway ~/projects/pacman (master) $ ./scripts/pkgdelta ./scripts/pkgdelta: line 147: $1: unbound variable
'set -u' with 'set -e'? Ugly...
Care to explain why? Using this in our cleanup scripts would have prevented the Arch repos disappearing on more than one occasion....
I've got fixes for both on my 'scripts-fixup' branch
https://github.com/falconindy/pacman/commit/e99b6a131ea08829da26d62d498c33f3... https://github.com/falconindy/pacman/commit/e42d97b7370c9e30d9ae66a33f42d274...
why not: case ${1:-""} in which fixes the issue and leaves nounset. Allan
On Fri, Jul 29, 2011 at 07:01:29PM +1000, Allan McRae wrote:
On 29/07/11 03:14, Dave Reisner wrote:
On Thu, Jul 28, 2011 at 11:18:15AM -0500, Dan McGee wrote:
Allan or Dave, mind taking a look at this? We should show usage info, as rankmirrors and pacman-key do without args, instead of this silly behavior.
-Dan
dmcgee@galway ~/projects/pacman (master) $ ./scripts/repo-add ==> ERROR: '' does not have a valid archive extension.
We never check for $1 being unset before proceeding. Probably my mistake when I did some refactoring of repo-add.
dmcgee@galway ~/projects/pacman (master) $ ./scripts/pkgdelta ./scripts/pkgdelta: line 147: $1: unbound variable
'set -u' with 'set -e'? Ugly...
Care to explain why? Using this in our cleanup scripts would have prevented the Arch repos disappearing on more than one occasion....
Because the combination of these two flags causes scripts to fail in mysterious and unexpected ways. Also, it won't save you from set but empty vars, which I suspect would be just as dangerous as unset vars in the situation you allude to. Just validate the user inputs. It makes things more obvious to an outside reader of the code, as well as consumers of the script. dave
I've got fixes for both on my 'scripts-fixup' branch
https://github.com/falconindy/pacman/commit/e99b6a131ea08829da26d62d498c33f3... https://github.com/falconindy/pacman/commit/e42d97b7370c9e30d9ae66a33f42d274...
why not:
case ${1:-""} in
which fixes the issue and leaves nounset.
Allan
On 29/07/11 22:38, Dave Reisner wrote:
On Fri, Jul 29, 2011 at 07:01:29PM +1000, Allan McRae wrote:
On 29/07/11 03:14, Dave Reisner wrote:
On Thu, Jul 28, 2011 at 11:18:15AM -0500, Dan McGee wrote:
Allan or Dave, mind taking a look at this? We should show usage info, as rankmirrors and pacman-key do without args, instead of this silly behavior.
-Dan
dmcgee@galway ~/projects/pacman (master) $ ./scripts/repo-add ==> ERROR: '' does not have a valid archive extension.
We never check for $1 being unset before proceeding. Probably my mistake when I did some refactoring of repo-add.
dmcgee@galway ~/projects/pacman (master) $ ./scripts/pkgdelta ./scripts/pkgdelta: line 147: $1: unbound variable
'set -u' with 'set -e'? Ugly...
Care to explain why? Using this in our cleanup scripts would have prevented the Arch repos disappearing on more than one occasion....
Because the combination of these two flags causes scripts to fail in mysterious and unexpected ways. Also, it won't save you from set but empty vars, which I suspect would be just as dangerous as unset vars in the situation you allude to. Just validate the user inputs. It makes things more obvious to an outside reader of the code, as well as consumers of the script.
FYI, this is the story I am referring to: http://archlinux.me/brain0/2009/08/16/shit-happens-when-you-party-naked-or-u... I'm still a fan of keeping set -u and -e... It may not save you from everything, but it does stop some things. Allan
On Sat, Jul 30, 2011 at 10:58:46AM +1000, Allan McRae wrote:
On 29/07/11 22:38, Dave Reisner wrote:
On Fri, Jul 29, 2011 at 07:01:29PM +1000, Allan McRae wrote:
On 29/07/11 03:14, Dave Reisner wrote:
On Thu, Jul 28, 2011 at 11:18:15AM -0500, Dan McGee wrote:
Allan or Dave, mind taking a look at this? We should show usage info, as rankmirrors and pacman-key do without args, instead of this silly behavior.
-Dan
dmcgee@galway ~/projects/pacman (master) $ ./scripts/repo-add ==> ERROR: '' does not have a valid archive extension.
We never check for $1 being unset before proceeding. Probably my mistake when I did some refactoring of repo-add.
dmcgee@galway ~/projects/pacman (master) $ ./scripts/pkgdelta ./scripts/pkgdelta: line 147: $1: unbound variable
'set -u' with 'set -e'? Ugly...
Care to explain why? Using this in our cleanup scripts would have prevented the Arch repos disappearing on more than one occasion....
Because the combination of these two flags causes scripts to fail in mysterious and unexpected ways. Also, it won't save you from set but empty vars, which I suspect would be just as dangerous as unset vars in the situation you allude to. Just validate the user inputs. It makes things more obvious to an outside reader of the code, as well as consumers of the script.
FYI, this is the story I am referring to:
http://archlinux.me/brain0/2009/08/16/shit-happens-when-you-party-naked-or-u...
I'm still a fan of keeping set -u and -e... It may not save you from everything, but it does stop some things.
Allan
I think that story only reinforces my point that you should be validating what you take in as input. Should a script bomb out in the middle of operation due to an unbound var triggering errexit, you may be left in some unknown or inconsistant state. It then means you have to add an ERR trap, figure out where you are, and take the appropriate action, if any, to restore sanity. It's unwieldy and vague, and explicit validation at key checkpoints will always be a more robust (and more readable) solution. I'll leave it up to you to decide which direction to go in. We really just wanted to fix the lack of usage on bad command line args. imo, pkgdelta needs a lot more help than just validation of input, but that's for another patch(set). dave
On 30/07/11 11:16, Dave Reisner wrote:
On Sat, Jul 30, 2011 at 10:58:46AM +1000, Allan McRae wrote:
On 29/07/11 22:38, Dave Reisner wrote:
On Fri, Jul 29, 2011 at 07:01:29PM +1000, Allan McRae wrote:
On 29/07/11 03:14, Dave Reisner wrote:
On Thu, Jul 28, 2011 at 11:18:15AM -0500, Dan McGee wrote:
Allan or Dave, mind taking a look at this? We should show usage info, as rankmirrors and pacman-key do without args, instead of this silly behavior.
-Dan
dmcgee@galway ~/projects/pacman (master) $ ./scripts/repo-add ==> ERROR: '' does not have a valid archive extension.
We never check for $1 being unset before proceeding. Probably my mistake when I did some refactoring of repo-add.
dmcgee@galway ~/projects/pacman (master) $ ./scripts/pkgdelta ./scripts/pkgdelta: line 147: $1: unbound variable
'set -u' with 'set -e'? Ugly...
Care to explain why? Using this in our cleanup scripts would have prevented the Arch repos disappearing on more than one occasion....
Because the combination of these two flags causes scripts to fail in mysterious and unexpected ways. Also, it won't save you from set but empty vars, which I suspect would be just as dangerous as unset vars in the situation you allude to. Just validate the user inputs. It makes things more obvious to an outside reader of the code, as well as consumers of the script.
FYI, this is the story I am referring to:
http://archlinux.me/brain0/2009/08/16/shit-happens-when-you-party-naked-or-u...
I'm still a fan of keeping set -u and -e... It may not save you from everything, but it does stop some things.
Allan
I think that story only reinforces my point that you should be validating what you take in as input. Should a script bomb out in the middle of operation due to an unbound var triggering errexit, you may be left in some unknown or inconsistant state. It then means you have to add an ERR trap, figure out where you are, and take the appropriate action, if any, to restore sanity. It's unwieldy and vague, and explicit validation at key checkpoints will always be a more robust (and more readable) solution.
I'll leave it up to you to decide which direction to go in. We really just wanted to fix the lack of usage on bad command line args. imo, pkgdelta needs a lot more help than just validation of input, but that's for another patch(set).
I will give you patch an "ack" as in the end I have never actually used the pkgdelta script so really do not care that much! :P Allan
participants (3)
-
Allan McRae
-
Dan McGee
-
Dave Reisner