Bug 201340 – [GTK][WPE] Do not run the Bubblewrap executable when configuring for cross-compilation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

.

[Patrick Griffis]

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.

[Philippe Normand]

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.

[Adrian Perez]

(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.

[Adrian Perez]

Created attachment 378803 [details]
Patch

[Konstantin Tokarev]

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.

[Konstantin Tokarev]

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})

[Adrian Perez]

(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!

[Adrian Perez]

Created attachment 378809 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 378809 [details]
Patch for landing

Clearing flags on attachment: 378809

Committed r249883: <https://trac.webkit.org/changeset/249883>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.