fix: preserve environment variables through fork to fix Waybar launch#20
fix: preserve environment variables through fork to fix Waybar launch#20Curious-Keeper wants to merge 3 commits intohyprwm:mainfrom
Conversation
This branch combines all three patch branches:
1. Waybar launch fix:
- Preserve environment variables (HYPRLAND_INSTANCE_SIGNATURE,
XDG_RUNTIME_DIR, WAYLAND_DISPLAY) through double-fork
- Fixes hang when launched from Waybar menu
2. SDDM+NVIDIA fix:
- Add --no-fork option to skip daemonization
- Add --vt option for VT switching after exit
- Fixes black screen on NVIDIA+SDDM systems
3. Code quality fixes:
- AppState.cpp: Add validation for empty addresses and invalid PIDs
- HyprlandIPC.cpp: Use ScopeGuard for automatic socket cleanup
Files changed:
- src/main.cpp: Environment preservation + --no-fork/--vt options
- src/state/AppState.cpp: Validation fixes
- src/state/HyprlandIPC.cpp: ScopeGuard for socket cleanup
When hyprshutdown is launched from Waybar menu items, it hangs with a black screen because the double-fork daemonization process loses critical environment variables needed for Wayland connection and Hyprland IPC. This fix captures and restores the following environment variables: - HYPRLAND_INSTANCE_SIGNATURE (required for Hyprland socket) - XDG_RUNTIME_DIR (required for socket path resolution) - WAYLAND_DISPLAY (required for Wayland backend connection) The environment variables are captured before forking and restored after each fork in the daemonization process, ensuring they remain available for IPC and Wayland operations. Fixes: [issue number/URL] Note: This branch was tested with additional changes from other PRs (code-quality and sddm-nvidia fixes) to verify compatibility. Those changes have been removed to keep this PR focused on the single issue.
|
let me know if you want this as a clean PR with no other history and I can branch and resubmit. thanks. |
vaxerski
left a comment
There was a problem hiding this comment.
wait wait wait, a better question is why are the env variables lost? Fork shouldnt lose any?
|
when launched form terminal or direct exec keybinds, the parent is wayland/hyprland, my shell = full login session, so all env var are present. However, when launched from waybar or rofi, the parent is no longer wayland it's waybar or that rofi menu, so it likely doesn't have all the variables to begin with, or they may be in a different state... so fork() just copies what the parent is, so parent=waybar=missing env vars=can't close correctly |
|
if waybar had no env it wouldnt be able to connect to the compositor to open. |
|
Update: Full disclaimer, I am not an expert and if I am wrong, please tell me why and I will fix or adjust. I am more concerned with learning and helping. Also, trying to explain things is not my best. So my incomplete and simplified way of describing what I think is going on earlier, here is what I think. The parent process context does matter. When launched from a terminal or direct Hyprland keybind, the process inherits a full login environment, so the relevant variables are already present and stable. When launched from Waybar or rofi, the parent is Waybar/rofi instead, and the environment may be incomplete or in a transitional state. What I didn’t say, because who want's to read a book on a PR. Here is a really long way of explaining what I think The real issue (I think) shows up when that environment state intersects with static initialization timing inside the IPC code. If the environment is read too early even briefly the static variable captures a NULL/empty value and never updates, regardless of what the environment looks like later. So the parent process explanation still applies, but the timing of when those variables are first read is what actually makes the bug stick. My patch is essentially a workaround for those. or what I think are contributing to the issue. It works by capturing and explicitly restoring the relevant values, which avoids timing-related problems. The main issue seems to come from static initialization timing in static const auto HIS = getenv("HYPRLAND_INSTANCE_SIGNATURE");This is evaluated once, on the first call to Here’s what I think is actually causing the problem:
The patch works because it captures the critical environment variables (
basically a defensive patch for processes that depend on environment context. A maybe fix could be changing this: static const auto HIS = getenv("HYPRLAND_INSTANCE_SIGNATURE");to: const char* getHIS() {
static const char* HIS = getenv("HYPRLAND_INSTANCE_SIGNATURE");
return HIS;
}Or better yet, pass it as a parameter or make it non-static entirely. The patch I suggested may not even be the ideal long-term solution, it’s just a valid workaround. Plus I didn't test if changing timing or static fixed anything. I just added the patch to main and submitted. I needed it to work for my build and someone here had the same issue so I patched, tested, and pushed. |
|
a very long way of saying, this is the best I got. hope it makes sense. |
During investigation, we discovered that hyprshutdown works correctly from waybar and rofi unless you are using an SDDM greeter. The SDDM greeter requires WAYLAND_DISPLAY to be preserved through the fork process to maintain proper Wayland context. This change simplifies the environment variable preservation to only keep WAYLAND_DISPLAY, which is sufficient for SDDM compatibility while maintaining functionality. The other environment variables (HYPRLAND_INSTANCE_SIGNATURE and XDG_RUNTIME_DIR) are not needed for the patch.
|
after getting some help, this PR is needed to keep SDDM happy. hyprland cleans up WAYLAND_DISPLAY, and sddm relies on it. removed extra variables. |
|
As far as I know, env is passed once to the process and a fork() should inherit it. Something else resets the env. |
|
really not sure what else it can be. I am stuck here for now, unless someone else knows where to look. I know the patch works, but if what you are saying is correct, I am not sure what is un-setting envs. |
|
Perhaps this #15 and issue on hyprpanel can give more context to the situation? |
yea, I ref 15 in the description. the issue isn't a crash for me. It doesn't "crash", it hangs on a black screen, because it doesn't reinstate sddm to complete the logout sequence for a user, even though hyprshutdown actually did work correctly, it's getting to sddm greeter from hyprshutdown that's the issue. You can use Since hyprshutdown is only meant to shutdown hyprland and return to login greet (for me that is sddm) I made the patch that stores the WAYLAND_DISPLAY env, then calls it at the end so it properly initiates sddm. Note: This only seems to be an issue with sddm greeter, when used from tuigreet, it logs out as expected, but tuigreet is tty based, not vt. this likely plays a roll but I am not sure how to better trouble shoot it. Also note, that this only happens from waybar or rofi menus, I can exec hyprshutdown normally and it works fine, it is only from a different parent that it seems to matter I already pushed one change for sddm providing a vt flag, in 16 but that doesn't fix this issue. Even though they both resulted in the same black screen prior to patch. |
Fixes #15
Problem
When
hyprshutdownis launched from Waybar menu items (or other launchers like rofi), it hangs with a black screen instead of showing the UI and closing apps properly. The workaround was to usehyprctl dispatch exec hyprshutdown, but this should work directly.Root Cause
SDDM relies on the WAYLAND_DISPLAY variable and hyprland cleans it up
Solution
Capture the environment variable before forking and restore after each fork in the daemonization process. This ensures they remain available for IPC and Wayland operations.
Changes
captureEnvVars()to save environment variables before forkingrestoreEnvVars()to restore them after each forkforkoff()to accept and restore environment variablesTesting
Tested on both NVIDIA and non-NVIDIA systems. Works correctly when launched from:
Files Changed
src/main.cpp- Environment variable preservation logicCloses #15