Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix local setup #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix local setup #119

wants to merge 1 commit into from

Conversation

pagdot
Copy link

@pagdot pagdot commented Jan 18, 2021

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

- Set $pkg instead of $script with package
- Shift arguments
@petemoore
Copy link

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

@pelwell
Copy link
Contributor

pelwell commented Dec 26, 2024

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).

@petemoore
Copy link

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).

@pelwell
Copy link
Contributor

pelwell commented Dec 27, 2024

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.

@pagdot
Copy link
Author

pagdot commented Dec 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants