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

Properly stop process started with sudo #144

Merged
merged 1 commit into from
Sep 29, 2024
Merged

Conversation

isamert
Copy link
Contributor

@isamert isamert commented Jul 14, 2024

Prodigy can't stop processes started with sudo because it does not
have the privileges to do that. This PR simply fixes that by running
the kill command with prodigy-start-sudo-process and with given
stop-signal. Partially fixes #111.

@Fuco1
Copy link
Collaborator

Fuco1 commented Jul 17, 2024

Maybe a different approach would be to set default directory using TRAMP and sudo protocol and then call the original code. I would as much as possible try to use the stop-process and similar Emacs commands instead of using utilities such as pgrep which aren't necessarily very portable.

@isamert
Copy link
Contributor Author

isamert commented Jul 17, 2024

While writing a response I figured out a way to not depending on pgrep, turns out sending the signal with appending - before the process group id redirects the signal to all children, which greatly simplified the implementation.

I agree that utilizing TRAMP would be the best possible solution but there are a few blockers:

  • Current :sudo t depends on calling sudo itself directly, so we need a whole refactor here to make it work with TRAMP.
  • Signalling the process directly does not work, because sudo creates a process group and sending a signal to this group is not passed to the children (I don't know why though, this is something about how sudo works). We still need to figure out what the child process id's are and signal them or we will use kill with the - trick again (but at least kill is more portable?).

Currently killing services with :sudo t does not work at all, this PR at least fixes that. With the latest change, it's even simpler.

@Fuco1
Copy link
Collaborator

Fuco1 commented Jul 18, 2024

Sounds good to me. Plus if it fixes the issue for at least some portion of the users that's a good change for me. It seems to be backward compatible with the non-sude tasks.

We can think about the refactor later, but I think there is already enough value to merge this. Thanks for looking into it!

@Fuco1
Copy link
Collaborator

Fuco1 commented Jul 18, 2024

Apparently this repo requires all commits to be signed:

image

Is this an option for you? If not, I can rebase them somehow so they get signed (it should still retain you as the author though, I don't want to take the credit)

This is the link: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

@isamert
Copy link
Contributor Author

isamert commented Jul 25, 2024

Sorry for getting back late, thanks for the heads up. Signed the commits and updated.

@isamert
Copy link
Contributor Author

isamert commented Sep 25, 2024

@Fuco1 just a nudge to remind you about this.

@DamienCassou
Copy link
Collaborator

@isamert Could you please squash your 2 commits into 1? I can do it if you prefer.

@isamert
Copy link
Contributor Author

isamert commented Sep 28, 2024

You can squash if it's okay for you, thanks!

@DamienCassou DamienCassou merged commit 18c0a3b into rejeep:master Sep 29, 2024
2 checks passed
@DamienCassou
Copy link
Collaborator

Done. Thank you very much for your work.

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.

sudo issues
3 participants