Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

xwayland: check executable exists on init #2314

Closed
wants to merge 1 commit into from

Conversation

emersion
Copy link
Member

@emersion emersion commented Jul 2, 2020

If the executable doesn't exist, fail early. This avoids setting an
invalid DISPLAY env variable when Xwayland isn't installed.

@emersion emersion requested a review from ddevault July 2, 2020 11:55
@ddevault
Copy link
Contributor

ddevault commented Jul 2, 2020

I don't like this patch. I would rather attempt to execute it and only set DISPLAY if it succeeds.

@emersion
Copy link
Member Author

emersion commented Jul 2, 2020

Problem is, we execute Xwayland lazily. So if Xwayland doesn't exist, DISPLAY will be set, then apps will try to connect to it and hang forever.

@ddevault
Copy link
Contributor

ddevault commented Jul 2, 2020

Maybe we can try running Xwayland -version to ascertain its existence/workitude?

A strict reading of POSIX allows execvp to run programs which are found outside of the PATH variable. I'm also not certain that this interprets PATH 100% to spec.

@emersion
Copy link
Member Author

emersion commented Jul 2, 2020

Maybe we can try running Xwayland -version to ascertain its existence/workitude?

-version is implemented but not yet released. Xwayland -help should work (with standard output/error set to /dev/null).

A strict reading of POSIX allows execvp to run programs which are found outside of the PATH variable. I'm also not certain that this interprets PATH 100% to spec.

True. And execvp has implementation-defined behavior when PATH is unset.

If the executable doesn't exist, fail early. This avoids setting an
invalid DISPLAY env variable when Xwayland isn't installed.
@emersion
Copy link
Member Author

emersion commented Jul 7, 2020

Updated the check to use exec. This has the downside of adding ~80ms to the startup time, which kind of negates the benefit of lazy xwayland I guess.

@ddevault
Copy link
Contributor

ddevault commented Jul 7, 2020

Meh. Maybe we should ditch lazy Xwayland, the benefits seem to be negligible.

@emersion
Copy link
Member Author

emersion commented Jul 8, 2020

Hm, after thinking about it, non-lazy Xwayland wouldn't work either. When Xwayland is ready it'll send SIGUSR1. We wouldn't be able to know whether Xwayland is available before the signal, so we'd need to block for a potentially long time.

@ddevault
Copy link
Contributor

ddevault commented Jul 8, 2020

So what is the right approach?

@ammen99
Copy link
Member

ammen99 commented Nov 3, 2020

Hm, after thinking about it, non-lazy Xwayland wouldn't work either. When Xwayland is ready it'll send SIGUSR1. We wouldn't be able to know whether Xwayland is available before the signal, so we'd need to block for a potentially long time.

What is the problem exactly? Here, you seem to be testing whether Xwayland runs at all, why do we need to wait for SIGUSR1?

@emersion
Copy link
Member Author

emersion commented Nov 3, 2020

Here, you seem to be testing whether Xwayland runs at all, why do we need to wait for SIGUSR1?

My goal was to find the best way to detect whether Xwayland is available before setting $DISPLAY. The potential solutions were:

  • Manually parse $PATH and try to find it: re-implements logic in exec, exec may behave differently depending on the libc implementation
  • Execute Xwayland -help: slow
  • Execute Xwayland normally and block until SIGUSR1: slow as well

None of those are great.

emersion added a commit to emersion/wlroots that referenced this pull request Feb 4, 2021
Instead of walking PATH like a previous proposal [1], this one
checks that the Xwayland path specified in the pkg-config file
exists.

I think this is a reasonable compromise:

- Users that don't have Xwayland installed system-wide won't get
  a bogus DISPLAY env variable set up.
- Users that have WLR_XWAYLAND set won't be affected by this check.
- Users that have Xwayland installed system-wide and a different
  Xwayland in their PATH still get their custom Xwayland.
- Users that don't have Xwayland installed system-wide but have it
  somewhere else in PATH are left out. But this is pretty niche,
  and they can just set WLR_XWAYLAND.

[1]: swaywm#2314
@emersion
Copy link
Member Author

emersion commented Feb 4, 2021

Closing in favor of #2717.

@emersion emersion closed this Feb 4, 2021
emersion added a commit to emersion/wlroots that referenced this pull request Feb 8, 2021
Instead of walking PATH like a previous proposal [1], this one
checks that the Xwayland path specified in the pkg-config file
exists.

I think this is a reasonable compromise:

- Users that don't have Xwayland installed system-wide won't get
  a bogus DISPLAY env variable set up.
- Users that have WLR_XWAYLAND set won't be affected by this check.
- Users that have Xwayland installed system-wide and a different
  Xwayland in their PATH still get their custom Xwayland.
- Users that don't have Xwayland installed system-wide but have it
  somewhere else in PATH are left out. But this is pretty niche,
  and they can just set WLR_XWAYLAND.

[1]: swaywm#2314
emersion added a commit to emersion/wlroots that referenced this pull request Feb 26, 2021
Instead of walking PATH like a previous proposal [1], this one
checks that the Xwayland path specified in the pkg-config file
exists.

I think this is a reasonable compromise:

- Users that don't have Xwayland installed system-wide won't get
  a bogus DISPLAY env variable set up.
- Users that have WLR_XWAYLAND set won't be affected by this check.
- Users that have Xwayland installed system-wide and a different
  Xwayland in their PATH still get their custom Xwayland.
- Users that don't have Xwayland installed system-wide but have it
  somewhere else in PATH are left out. But this is pretty niche,
  and they can just set WLR_XWAYLAND.

[1]: swaywm#2314
emersion added a commit to emersion/wlroots that referenced this pull request Feb 26, 2021
Instead of walking PATH like a previous proposal [1], this one
checks that the Xwayland path specified in the pkg-config file
exists.

I think this is a reasonable compromise:

- Users that don't have Xwayland installed system-wide won't get
  a bogus DISPLAY env variable set up.
- Users that have WLR_XWAYLAND set won't be affected by this check.
- Users that have Xwayland installed system-wide and a different
  Xwayland in their PATH still get their custom Xwayland.
- Users that don't have Xwayland installed system-wide but have it
  somewhere else in PATH are left out. But this is pretty niche,
  and they can just set WLR_XWAYLAND.

[1]: swaywm#2314
emersion added a commit that referenced this pull request Mar 3, 2021
Instead of walking PATH like a previous proposal [1], this one
checks that the Xwayland path specified in the pkg-config file
exists.

I think this is a reasonable compromise:

- Users that don't have Xwayland installed system-wide won't get
  a bogus DISPLAY env variable set up.
- Users that have WLR_XWAYLAND set won't be affected by this check.
- Users that have Xwayland installed system-wide and a different
  Xwayland in their PATH still get their custom Xwayland.
- Users that don't have Xwayland installed system-wide but have it
  somewhere else in PATH are left out. But this is pretty niche,
  and they can just set WLR_XWAYLAND.

[1]: #2314
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants