.
Yes it is technically only required at runtime but there are two things gained by checking for it at build time. - We gain an absolute path to the binary we can rely on. - There will be no mistakes made by distros missing the dependency. xdg-dbus-proxy is a new project nobody packages and the sandbox is disabled by default so I guarantee all distros would skip this dependency if we didn't error loudly.
Checking for it at build-time doesn't guarantee the runtime host will have the program located at the same place though. As Adrian pointed out, the CMake stuff should allow for an option specifying the absolute path of the program.
(In reply to Philippe Normand from comment #2) > Checking for it at build-time doesn't guarantee the runtime host will have > the program located at the same place though. > > As Adrian pointed out, the CMake stuff should allow for an option specifying > the absolute path of the program. I have checked things a bit in this regard. I have both “yay” and “meh” news: - The good news is that it is already possible to override the path passing “-DDBUS_PROXY_EXECUTABLE=/path/in/target/xdg-dbus-proxy” when invoking CMake—if the variable is set on the command line, then find_program() will skip guessing the location of the program. - On the other hand, we still need to avoid running programs which will be used only at run-time during CMake configuration *when cross-compiling*, because a.) they may not be installed in the build host, and b.) if they are available in the build host, they may not be the same version installed in the target. The second one is currently an issue because we are currently running “bwrap --version” from “Source/cmake/BubblewrapSandboxChecks.cmake” unconditionally. Let's re-purpose this bug for this.
Created attachment 378803 [details] Patch
Comment on attachment 378803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378803&action=review > Source/cmake/BubblewrapSandboxChecks.cmake:26 > + string(REGEX MATCH "([0-9]+.[0-9]+.[0-9]+)" BWRAP_VERSION "${BWRAP_OUTPUT}") Seems like there is a bug in regex - it should check for literal dot in version, so dot must be escaped.
Comment on attachment 378803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378803&action=review >> Source/cmake/BubblewrapSandboxChecks.cmake:26 >> + string(REGEX MATCH "([0-9]+.[0-9]+.[0-9]+)" BWRAP_VERSION "${BWRAP_OUTPUT}") > > Seems like there is a bug in regex - it should check for literal dot in version, so dot must be escaped. Should be string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" BWRAP_VERSION ${BWRAP_OUTPUT})
(In reply to Konstantin Tokarev from comment #6) > Comment on attachment 378803 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378803&action=review > > >> Source/cmake/BubblewrapSandboxChecks.cmake:26 > >> + string(REGEX MATCH "([0-9]+.[0-9]+.[0-9]+)" BWRAP_VERSION "${BWRAP_OUTPUT}") > > > > Seems like there is a bug in regex - it should check for literal dot in version, so dot must be escaped. > > Should be > > string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" BWRAP_VERSION ${BWRAP_OUTPUT}) This was already wrong before my changes. Good catch, I will fix it before landing, thanks!
Created attachment 378809 [details] Patch for landing
Comment on attachment 378809 [details] Patch for landing Clearing flags on attachment: 378809 Committed r249883: <https://trac.webkit.org/changeset/249883>
All reviewed patches have been landed. Closing bug.