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

Don't litter #168

Merged
merged 3 commits into from
Feb 27, 2022
Merged

Don't litter #168

merged 3 commits into from
Feb 27, 2022

Conversation

subnut
Copy link
Contributor

@subnut subnut commented Aug 15, 2021

chdir to the directory padd.sh is stored in before executing the rest of
the script. Prevents the formation of PADD.pid and piHoleVersion files
everywhere.

Fixes #62

By submitting this pull request, I confirm the following:
please fill any appropriate checkboxes, e.g: [X]

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

@subnut
Copy link
Contributor Author

subnut commented Feb 19, 2022

Rebased on master

@yubiuser
Copy link
Member

Please address stickler errors.

@subnut
Copy link
Contributor Author

subnut commented Feb 19, 2022

Stickler CI is telling me to

-cd "$(dirname "$0")"
+cd "$(dirname "$0")" || exit 1

Should I?


If we go for it, I would suggest something like -

cd "$(dirname "$0")" || {
    EC=$?
    echo >&2 "Could not chdir to the directory containing padd.sh (exit code $EC)"
    exit $EC
}

@subnut
Copy link
Contributor Author

subnut commented Feb 19, 2022

@yubiuser I just saw your comment after I posted mine. 😅

What change should I make?

This?

cd "$(dirname "$0")" || exit 1

Or this?

cd "$(dirname "$0")" || exit $?

Or this?

cd "$(dirname "$0")" || {
    EC=$?
    echo >&2 "Could not chdir to the directory containing padd.sh (exit code $EC)"
    exit $EC
}

Or something else?

@yubiuser
Copy link
Member

I like

cd "$(dirname "$0")" || {
    EC=$?
    echo >&2 "Could not chdir to the directory containing padd.sh (exit code $EC)"
    exit $EC
}

Because it gives a reason why it's failing.

@subnut
Copy link
Contributor Author

subnut commented Feb 19, 2022

@yubiuser Implemented.

@yubiuser
Copy link
Member

Thanks.

I just read the issue thread again and I'm not sure if the proper solution is cd but what was suggested by @Bogdan107

Proposal: create piHoleVersion file under the /tmp directory.

What do you think?
__

Additionally, we create the PADD.pid file since this commit: ed23cda

However, I don't see that we use that file in any way. I thinks the whole PADD.pid stuff can go.

@thomasmerz
Copy link
Contributor

My apologies for interfering, but I think:

padd.sh
1035:    pid=$$
1037:    echo ${pid} > ./PADD.pid
1070:    pid=$$
1071:    echo "- Writing PID (${pid}) to file."
1072:    echo ${pid} > ./PADD.pid
1110:    pid=$$
1111:    echo "- Writing PID (${pid}) to file..."
1112:    echo ${pid} > ./PADD.pid

@subnut
Copy link
Contributor Author

subnut commented Feb 20, 2022

Proposal: create piHoleVersion file under the /tmp directory.

What do you think?

I think that it is much easier to simply instruct users to keep padd.sh in a writeable directory. And also, in almost all cases, the directory is going to be writable anyway (since padd.sh was created in that directory!).

I don't see that we use that file in any way. I thinks the whole PADD.pid stuff can go.

Maybe it was added to ensure that no race conditions occur when padd.sh is updating piHoleVersion?
But that would be better left for a different PR.

@subnut
Copy link
Contributor Author

subnut commented Feb 20, 2022

/run would fit much better than /tmp

FWIW, I am currently using Void Linux, and it does not have a user-writable /run, but it has a user-writable /tmp

@subnut
Copy link
Contributor Author

subnut commented Feb 20, 2022

Rebased on master.

@yubiuser
Copy link
Member

Did you intend to modify .gitignore as well?

@yubiuser
Copy link
Member

But that would be better left for a different PR.

#197

@subnut
Copy link
Contributor Author

subnut commented Feb 20, 2022

Did you intend to modify .gitignore as well?

Yeah, because the ./piHoleVersion file must not be tracked by git.
And since it contained many un-related entries, I removed them too. (There is never gonna be any C object files or libraries created by a shell script 🤡 )

@thomasmerz
Copy link
Contributor

FWIW, I am currently using Void Linux

I ❤️ "standards" 🤣 - ok,/tmp/ should be writeable on all linux distros.

chdir to the directory padd.sh is stored in before executing the rest of
the script. Prevents the formation of PADD.pid and piHoleVersion files
everywhere.

Fixes #62
@subnut
Copy link
Contributor Author

subnut commented Feb 21, 2022

Rebased on master

@subnut subnut changed the base branch from master to development February 27, 2022 03:20
@subnut
Copy link
Contributor Author

subnut commented Feb 27, 2022

Changed PR base to development

@subnut subnut requested review from yubiuser and DL6ER February 27, 2022 03:23
@yubiuser yubiuser merged commit 1eb132e into pi-hole:development Feb 27, 2022
@subnut subnut deleted the no-litter branch February 27, 2022 09:47
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.

Error when started from r/o directory
4 participants