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

Add FreeBSD service #2333

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add FreeBSD service #2333

wants to merge 10 commits into from

Conversation

DtxdF
Copy link

@DtxdF DtxdF commented Jul 25, 2022

Description

Service support for FreeBSD.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

contrib/FreeBSD/README Outdated Show resolved Hide resolved
Copy link
Member

@half-duplex half-duplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

It's not clear who the target is for the readme, but I think answering that question might let it be more concise and less duplicative of the contents of the script.

contrib/FreeBSD/README Outdated Show resolved Hide resolved
contrib/FreeBSD/README Outdated Show resolved Hide resolved
profile="$1"; shift

run_rc_command stop "${profile}" $@ &&
sleep 1 &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sleep 1?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - will stop ever return before sopel has finished stopping? If not, is there a reason for the sleep?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, some IRC servers may block an IP with two or more simultaneous connections to prevent flooding. The message Session limit exceeded should be printed in the IRC client. I will use sleep 1 because daemon(8) will run sopel in the background without waiting. I know sopel has a flag called --fork, but the daemon is convenient in context because the output is redirected to /dev/null (sopel already prints the output to /var/log/sopel, so there is no need to have two outputs).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, but I think that belongs in the start function - and it looks like there already is one there, making this one unnecessary, unless I'm misunderstanding run_rc_command or something.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to a slow connection or a network problem that delays a packet before disconnecting it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally Sopel sends a QUIT message and waits before exiting. Unless this stop doesn't wait for sopel to exit, or you know of routine cases I don't, you should be able to assume that if the process exits you're all set.

def quit(self, message: Optional[str] = None) -> None:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in another comment, the service uses daemon(8) which does not wait for the program to run.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you agree with the change I mentioned?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting two:
echo;sleep;fork;echo;sleep;fork;echo;sleep;fork
Restarting two:
stop;wait_for_pids;sleep;echo;sleep;fork;stop;wait_for_pids;sleep;echo;sleep;fork

So for each, to start it simply waits 1 second and then forks, for a total delay between instances of one second.
To stop, it's sending a signal, waiting for Sopel to exit, waiting one second, then in start it's waiting another second, then forking the new one. The total delay here is 2 seconds between each instance.

I still don't see why there needs to be twice the delay when restarting vs starting.

contrib/FreeBSD/sopel-default.cfg Show resolved Hide resolved
contrib/FreeBSD/README Outdated Show resolved Hide resolved
@DtxdF
Copy link
Author

DtxdF commented Jul 26, 2022

Thanks for the PR!

It's not clear who the target is for the readme, but I think answering that question might let it be more concise and less duplicative of the contents of the script.

It is for FreeBSD users. The README explains how to set variables for sopel using the FreeBSD tools and explains how to get more variables to configure sopel. My intention is to show how to configure sopel as another FreeBSD service through /etc/rc.conf.

Jesús Daniel Colmenares Oviedo added 2 commits July 26, 2022 10:38
* Delete parse variables in the docs
* Reduce long lines to keep between 80-120 chars
Copy link
Member

@half-duplex half-duplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the readme is intended for end users (rather than packagers), I think even fewer of the options need to be specifically pointed out. If someone has unusual needs (in particular, changing sopel_prefix or sopel_interpreter) they probably understand how to do that just by the sopel_profiles instruction and the list of defaults.

The average end user will be reading documentation with one goal in mind, making it work. They're looking for "pip install sopel, then copy this file here, that file there, and run these two commands. For these common changes do [...], or for less common changes apply that pattern to one of [variable list]", and the doc should be structured and written to support them in that endeavor.

contrib/FreeBSD/README.md Outdated Show resolved Hide resolved
contrib/FreeBSD/README.md Outdated Show resolved Hide resolved
contrib/FreeBSD/README.md Show resolved Hide resolved
contrib/FreeBSD/README.md Outdated Show resolved Hide resolved
DtxdF and others added 5 commits July 26, 2022 14:30
Co-authored-by: mal <mal@sec.gd>
The `sopel_prefix` is intended to packagers, not end-users
…explicit about the scripting language used.
@dgw
Copy link
Member

dgw commented Jul 29, 2022

@half-duplex When this is satisfactory, are you up to walking OP through the process to squash & amend commit message to meet project standards?

@DtxdF
Copy link
Author

DtxdF commented Jul 29, 2022

@half-duplex When this is satisfactory, are you up to walking OP through the process to squash & amend commit message to meet project standards?

I apologize, I will comply with rule 5 in CONTRIBUTING.md.

@dgw
Copy link
Member

dgw commented Jul 29, 2022

@DtxdF Don't worry about it just yet. We allow time to squash/tweak the history after PR approval. 🙂

@dgw
Copy link
Member

dgw commented Jan 2, 2023

Did this one fall by the wayside? I would defer to @half-duplex's expertise here, so just looking for the 👍 that it's ready and a squash of the intermediate commits + messages meeting guidelines.

@dgw dgw added the Stale Mostly used for PRs that no longer work and need updating before re-review/merge. label Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Stale Mostly used for PRs that no longer work and need updating before re-review/merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants