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 cmd_env and cmd_env_unset to set and unset environment variables at config and runtime #8327

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

Conversation

slonkazoid
Copy link

@slonkazoid slonkazoid commented Sep 3, 2024

me and a few friends have been using this patch since forever and i wanted to try my luck at upstreaming it… i know this is an opinionated topic and i don't 100% expect this pr to be accepted. it just makes managing environment variables so much easier if you have multiple compositors or if you are using a DM.

update: forgot documentation

@Nefsen402
Copy link
Member

Nefsen402 commented Sep 3, 2024

This is something that can cause unexpected results especially when sway/wlroots consumes the environment variables themselves. Debug environment variables that wlroots uses for e.g. wlr_scene will not honor these environment variables when they are set during runtime.

If the other contributors support this idea, it would probably be preferable that the environment variables here only affect applications that are launched using the exec command.

@slonkazoid
Copy link
Author

the volatile keyword may be needed in the definition of child_envp as it is used across threads, but i am not sure.

@kennylevinsen
Copy link
Member

This is something that can cause unexpected results especially when sway/wlroots consumes the environment variables themselves. Debug environment variables that wlroots uses for e.g. wlr_scene will not honor these environment variables when they are set during runtime.

That it does not affect debug variables isn't unexpected, but having WLR_RENDERER set later take effect when a new GPU shows up could be surprising.

Making it strictly for child processes is fine though.

the volatile keyword may be needed in the definition of child_envp as it is used across threads, but i am not sure.

There are no threads here. After the fork, the child process is referencing a copy of all parent process memory and will not see any further changes. No volatile needed.


We have historically said no to this. However, arguments about this not being an i3 feature isn't entirely fair as i3 can be restarted with new environment variables mid-session, which cannot be done with sway. But on the other hand, you can already get easy runtime environment adjustment the traditional unix way with:

set $exec exec ~/bin/sway-exec.sh
bindsym mod4+return $exec foot

So I'm split between "this is a simple feature so why not", and "this doesn't allow anything new so why deviate from i3" - especially considering how I'm one to preach wrapper scripts all the time.

Maybe I'm slightly in favor because it's so trivial a feature, and the time saved on people asking about how to set environment variables is worth something too.

... But not with a glib dependency. :)

@slonkazoid
Copy link
Author

slonkazoid commented Sep 9, 2024

But not with a glib dependency. :)

sway already (though, indirectly) depends on glib

@slonkazoid
Copy link
Author

But not with a glib dependency. :)

sway already (though, indirectly) depends on glib

i did not have to add that glib dependency, the methods i needed were already in scope, but, i did so if you drop one of the dependencies that also depend on glib it wouldn't break

@kennylevinsen
Copy link
Member

i did not have to add that glib dependency, the methods i needed were already in scope, but, i did so if you drop one of the dependencies that also depend on glib it wouldn't break

To be clear, the issue is not the modification of the meson.build file, but using the library. We don't use glib in sway, even if some of our dependencies do, and we don't add dependencies for trivial things that are only a handful of lines on its own.

@slonkazoid
Copy link
Author

alright i have implemented the part of glib the command needed myself in questionable quality C code. can someone please review this code for soundness? i tested it with ASAN and it seemingly works perfectly.

no style checking has been done yet, accepting complaints about that

@slonkazoid slonkazoid changed the title add cmd_env to set environment variables at config and runtime add cmd_env and cmd_env_unset to set and unset environment variables at config and runtime Sep 12, 2024
@slonkazoid
Copy link
Author

can i request a review on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants