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 SysVinit start script #187

Closed
Braintoe opened this issue Nov 19, 2023 · 14 comments
Closed

Add SysVinit start script #187

Braintoe opened this issue Nov 19, 2023 · 14 comments
Labels
enhancement New feature or request

Comments

@Braintoe
Copy link
Contributor

Hello there,

I am a little unsure if this fits better here or in the MX Linux forum - but I guess here might be better since there is more than one distribution that might benefit from this... so just in case someone needs it: while I searched for the reason why wsdd did not load on my MX Linux 23, I created a new SysVinit script for wsdd based on the existing openrc script here and the old one that was used for MX Linux 21: wsdd.zip.

Maybe this could be added to the repository?

The script combines the nice workgroup extraction from smb.conf the openrc init script features with some basic logging functionality - wsdd does not detach itself which seems to require a start parameter for the daemon that makes stdout inaccessible, therefore the log I got before adding this was just empty.

@Braintoe
Copy link
Contributor Author

Edit: I just noticed I uploaded the wrong version of the script - status does not work for that one. Here is the correct one:
wsdd.zip

@christgau
Copy link
Owner

Would you mind to open up a pull request for this. That is a better place than an issue.

@christgau christgau added the enhancement New feature or request label Nov 19, 2023
@Braintoe
Copy link
Contributor Author

Braintoe commented Nov 19, 2023

Would you mind to open up a pull request for this. That is a better place than an issue.

I have to admit I have no clue how to do that... I just did such a thing once, a couple of years ago, and I admit I do not remember how. Therefore sure, if you like, but I definitely need help on that topic (ideally in german...).

@Braintoe
Copy link
Contributor Author

Thanks! Let me try... :-)

@Braintoe
Copy link
Contributor Author

Okay, it seems I have succeeded...

@JedMeister
Copy link
Contributor

To preface my comments here, I have nothing to do with this software (beyond using it).

I'm just a random person passing through. So whilst I like to think that my input will be useful, @christgau is the arbiter of how things work for this repo and perhaps I'm off the mark? So please do not feel any obligation to follow my suggestions (unless of course if @christgau explicitly agrees).

OTOH I doubt my suggestion(s) would be problematic, so if you think my suggestions make sense, then my guess is that you could apply them.

Finally, as a general rule most GitHub code repos will have a CONTRIBUTING` (or similar) file in the top level detailing the requirements. I note that it doesn't give any relevant guidance here specifically but it's good to double check IMO,


I'd recommend reopening this issue and editing the top post of your PR to include this (on it's own line - above or below - doesn't really matter which):

closes #187

Then when your PR is merged by @christgau, this issue will be (automatically) closed. That way it's easy to be sure whether this "issue" is resolved in the master branch codebase or not.

Also, a couple of nitpicks (which @christgau may or may not agree with):

  • IMO the dir for your new files should be sysv-init (or perhaps sysvinit) rather than SysVinit). Unlike Windows Linux filesystems are case sensitive by default and as a heavy Linux commandline user, caps in this context "look ugly" and are a bit of a PITA because (my) muscle memory always defaults to lower case<tab> (e.g. s\<tab> - to tab complete sysv-init filepath).
  • the convention for the relevant filenames (at least for sysv-init ones in Debian/Ubuntu) are: wsdd.defaults and wsdd.init. So long as the directory they are in is descriptive enough (which it is in this case), the files don't need to be in sub dirs. E.g. see https://github.com/christgau/wsdd/tree/master/etc/systemd

@Braintoe
Copy link
Contributor Author

Braintoe commented Dec 4, 2023

@JedMeister thanks for the remark. I reopened the issue and added the "closes" remark to the pull request as you suggested.

As a German, I am maybe more used to using capital letters than you. Apart from that, I named the files as they should be in MX Linux - which is "wsdd" in both cases, hence the subfolders. In the "/etc/openrc" folder here, the structure is set up in the very same way.
But that is academical - any folder or file naming logic is up to @christgau since it is his very own project, therefore I will wait and change it to whatever he wants ;-)

@christgau
Copy link
Owner

First of all, thanks for the discussion here. @JedMeister I appreciate your input, as always

Here is advice for he PR:

  1. please use sysvinit as directory name
  2. the subfolder structure is ok, I was thinking of the openrc directory as well when reading the changes. IMO having the subfolders is fine. Could be it is a little inconsistent w.r.t. systemd, but it's ok 😉
  3. Change the commit message header for the PR (see CONTRIBUTING for details 😉)

@Braintoe
Copy link
Contributor Author

Braintoe commented Dec 9, 2023

  1. please use sysvinit as directory name

That one is clear, and after a while I also found how to do that on this website. Done!

  1. Change the commit message header for the PR (see CONTRIBUTING for details 😉)

While I have no clue for the reason (probably for automating... things... changelogs?), I hope I understood at least what you asked me to do... changed according to best guess. Please tell me if something is still wrong. I can safely say Github is not my world ;-)

@christgau
Copy link
Owner

christgau commented Dec 9, 2023

  1. please use sysvinit as directory name

That one is clear, and after a while I also found how to do that on this website. Done!

Great. Thanks.

Just as a comment: Although the website may offer that service, you may also consider using git on the command line, if you want to. It would have been as easy as git mv etc/SysVinit etc/sysvinit

  1. Change the commit message header for the PR (see CONTRIBUTING for details 😉)

While I have no clue for the reason (probably for automating... things... changelogs?),

In principle it's about consistency in the repo's history and a question of style. Following a certain style of commit messages enables to quickly detect what a commit is about. It also allows for automation but that's currently not in use in that repo.

I hope I understood at least what you asked me to do... changed according to best guess. Please tell me if something is still wrong. I can safely say Github is not my world ;-)

You're welcome anyways. The primary reason I pointed to the contribution guide was the commit message, it should follow conventional commit message style. In this case, you add a new feature, so something like feat(etc): add SysVinit start script would be appropriate.

@Braintoe
Copy link
Contributor Author

Braintoe commented Dec 9, 2023

Thanks! Then I should have done the nright thing to that message.

Regarding git: yes, I have heard from that ans also found lots of corresponding notes when I searched for how to rename a folder. But I admit I don't really see a sense in installing something that I probably won't use much more than once. Even the content of the start script probably won't change much for the forseeable future. Those folder names probably won't ever :-)

@christgau
Copy link
Owner

"merged" with 628afd7, although the PR #188 is "victim" of kind of an git accident 🙈

@JedMeister
Copy link
Contributor

Regarding git: yes, I have heard from that ans also found lots of corresponding notes when I searched for how to rename a folder. But I admit I don't really see a sense in installing something that I probably won't use much more than once.

If you are using Linux (which your use of wsdd suggests you are) it should be as simple as:

apt install git

(Obviously the exact command will depend on your distro - but that will work for Debian/Ubuntu and derivatives).

Although git is a fairly common tool in Linux these days, so it's possible that you already have it installed...

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

Successfully merging a pull request may close this issue.

3 participants