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

Implement -U for Linux #2049

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Implement -U for Linux #2049

merged 6 commits into from
Nov 5, 2024

Conversation

g0mb4
Copy link
Contributor

@g0mb4 g0mb4 commented Oct 2, 2024

If this argument is provided Conky won't start if another Conky process is already running.

Conky starts multiple times on my system at startup, and it is bothering me. I'm too lazy to check all of my config files, so I added this mechanism to prevent this behavior.

If this argument is provided Conky won't start if another Conky process
is already running.
@github-actions github-actions bot added documentation Issue or PR that suggests documentation improvements sources PR modifies project sources labels Oct 2, 2024
Copy link

netlify bot commented Oct 2, 2024

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 6cef51f
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/672882b14e86b400080e37a4

@brndnmtthws
Copy link
Owner

A couple issues: /proc is generally Linux-only (see https://en.wikipedia.org/wiki/Procfs), so we can't assume it's there on BSDs and macOS.

Another problem, getting the process name from /proc/[pid]/cmdline isn't a great idea because the command could have a symlink or some other strangeness. Better to use either /proc/[pid]/stat or follow the /proc[pid]/exe symlink.

@g0mb4
Copy link
Contributor Author

g0mb4 commented Oct 2, 2024

Thanks! It's the typical case of "it works on my machine"...
I'll try to figure out a POSIX solution (maybe using ps).

The previous method used `/proc` to check the running processes,
but it is a Linux-specific solution.

This method uses `ps`, hopefully in a POSIX-compliant way.

Linux: https://man7.org/linux/man-pages/man1/ps.1.html
FreeBSD: https://man.freebsd.org/cgi/man.cgi?ps(1)
OpenBSD: https://man.openbsd.org/ps.1
A random process named `conky11` won't stop Conky to start when
`-U` is provided.
@brndnmtthws
Copy link
Owner

brndnmtthws commented Oct 28, 2024

I know I'm a bit late on getting back to you on this, and I apologize for that.

I am not a big fan of shelling out to ps in this way, it also feels like something that is likely to break and probably not consistent across platforms. I'm more inclined to take the original patch you submitted, and just refactor it so that it's Linux-only, and if people want to port it to the BSDs/macOS then so be it.

And to clarify, when I say "original patch" I mean reading from /proc, except rather than parsing /proc/[pid]/cmdline we can pull the name from /proc/[pid]/stat.

@g0mb4
Copy link
Contributor Author

g0mb4 commented Oct 28, 2024

No problem at all.

I tested the ps on FreeBSD and on Linux and the output was the same. I read that the Mac implementation should work as expected (based on some man pages).

It feels a "good enough" soultion for me as well, but this was the most POSIX-way I can came up with.

@g0mb4
Copy link
Contributor Author

g0mb4 commented Oct 28, 2024

I can do the /proc/[pid]/stat method in a couple of days if it is okay.

g0mb4 and others added 2 commits October 29, 2024 14:26
Use `/proc/[pid]/stat` to check if conky is already running.
This will only work on systems where there is a `/proc`.
@Caellian
Copy link
Collaborator

Caellian commented Nov 1, 2024

I just added ifdef for Linux around everything and note in the manual, to avoid issue reports for it not working on non-Linux systems.

Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

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

Overall looks good, I have one minor request (to move the Linux-only code to src/linux.cc.

src/main.cc Outdated Show resolved Hide resolved
@brndnmtthws brndnmtthws merged commit b01485b into brndnmtthws:main Nov 5, 2024
40 checks passed
@LinuxOnTheDesktop
Copy link

This option seems like a useful addition. For, one can find a few reports on the web of people ending up with weird-looking conkies that turn out to be one conky superimposed upon another. I've encountered that myself. So there's even a case for making this option the default.

@brndnmtthws brndnmtthws added the feature New feature PR or issue label Nov 5, 2024
@g0mb4
Copy link
Contributor Author

g0mb4 commented Nov 7, 2024

Thanks! I can try to implement this feature on some BSDs if @brndnmtthws finds it as useful as you.

@Caellian
Copy link
Collaborator

Caellian commented Nov 7, 2024

So there's even a case for making this option the default.

After triaging issues I'd say changing the defaults will likely cause more confusion than having the option be opt-in. A lot of setups also spawn several conky instances to show information in different corners of the screen for example.

I can try to implement this feature on some BSDs if [Brandon] finds it as useful as you.

It would be a nice feature-parity addition, but it's up to you. I opened #2072, so you can use it as a guide on which platforms need to be supported if you decide to add support any other. I will also redirect people there if they open new issues about the feature not being supported for one of those platforms.

@Caellian Caellian changed the title Implement -U. Implement -U for Linux Nov 7, 2024
@LinuxOnTheDesktop
Copy link

@g0mb4

Question: will conky -U run Conky if another user is running a Conky? For, consider so-called fast-user switching.

@g0mb4
Copy link
Contributor Author

g0mb4 commented Nov 13, 2024

@LinuxOnTheDesktop

It depends on how /proc was mounted. If it allows the current user to see the processes of the other user, then conky won't start.

@g0mb4 g0mb4 deleted the unique branch November 13, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue or PR that suggests documentation improvements feature New feature PR or issue sources PR modifies project sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants