tools icon indicating copy to clipboard operation
tools copied to clipboard

Fix local setup

Open pagdot opened this issue 4 years ago • 5 comments

  • Set $pkg instead of $script with package
  • Shift arguments

pagdot avatar Jan 18 '21 22:01 pagdot

This looks like a reasonable fix to me - is there a reason it hasn't been accepted?

petemoore avatar Dec 26 '24 11:12 petemoore

Inadequate justification of the change and explanation of what it does?

Have you tested it? At first glance I'm not sure it does what is expected (but the original looks broken as well).

pelwell avatar Dec 26 '24 16:12 pelwell

Sorry Phil, on closer inspection, indeed it does look a bit suspect. On first glance it looked reasonable, but when I look more closely, the original script looks broken in ways I think this patch doesn't address, e.g. these lines:

    [ "_$1" = "_-p" -o "_$1" = "_--package" ] && { pkgs="$1"; shift; }
    [ "_$1" = "_-b" -o "_$1" = "_--bin" ] && { scripts="$1"; shift; }

e.g. if $1 is -p then pkgs will be set to -p whereas presumably pkgs would be the parameter following the -p or --package argument (and you would expect 2 shifts, not one). It also wasn't completely clear to me what this script is for or how it should be used.

Full disclosure: I was raising a PR myself (#134) and tend to look at open PRs before submitting one, just to see if the repo is actively maintained. I saw this PR from 4 years ago that looked reasonable, so I was curious why nobody had responded (I thought maybe it had accidentally gone unnoticed).

petemoore avatar Dec 26 '24 22:12 petemoore

I thought maybe it had accidentally gone unnoticed

There may be an element of that - I can't remember that far back. Unless a patch is obviously correct and useful, or addresses a clearly explained problem, it's easy to put them on one side for later.

pelwell avatar Dec 27 '24 09:12 pelwell

As this is a few years back, I don't remember really, but would guess that the script was broken and I fixed its issues and made a low effort PR so maybe the next person would have it working. In the end I've decided against making my own custom RP image and using ansible instead (like on all other systems) and it wasn't important enough to check it again. If there is interest, Ill take a look at it again and improve the documentation of the PR

pagdot avatar Dec 27 '24 16:12 pagdot