[gentoo-dev] [PATCH 0/6] virtualx.eclass: Use Xvfb -displayfd ... + cleanup

[gentoo-dev] [PATCH 0/6] virtualx.eclass: Use Xvfb -displayfd ... + cleanup

[Michał Górny]

Hi,

With modern versions of Xvfb, we can get rid of our custom logic of
trying to find a free DISPLAY.  This patchset does that + some
refactoring.  It also removes the hack of disabling sandbox that
apparently dates back to 2002 and has never been really explained.
According to my quick testing, Xvfb works just fine without it.


Michał Górny (6):
  virtualx.eclass: Add quoting to workaround vim syntax hl bug
  virtualx.eclass: Pass xvfbargs as an array
  virtualx.eclass: Remove unused XHOST
  virtualx.eclass: Inline XVFB
  virtualx.eclass: Let Xvfb figure out the free DISPLAY
  virtualx.eclass: Do not disable sandbox

 eclass/virtualx.eclass | 79 +++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 55 deletions(-)

-- 
2.35.1

[gentoo-dev] [PATCH 1/6] virtualx.eclass: Add quoting to workaround vim syntax hl bug

[Michał Górny]

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/virtualx.eclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/eclass/virtualx.eclass b/eclass/virtualx.eclass
index b7a25c897b4a..5c4b39164cff 100644
--- a/eclass/virtualx.eclass
+++ b/eclass/virtualx.eclass
@@ -53,7 +53,7 @@ case ${VIRTUALX_REQUIRED} in
 	*)
 		BDEPEND="${VIRTUALX_REQUIRED}? ( ${VIRTUALX_DEPEND} )"
 		IUSE="${VIRTUALX_REQUIRED}"
-		[[ ${VIRTUALX_REQUIRED} == test ]] &&
+		[[ ${VIRTUALX_REQUIRED} == "test" ]] &&
 			RESTRICT+=" !test? ( test )"
 		;;
 esac
-- 
2.35.1

[gentoo-dev] [PATCH 2/6] virtualx.eclass: Pass xvfbargs as an array

[Michał Górny]

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/virtualx.eclass | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/eclass/virtualx.eclass b/eclass/virtualx.eclass
index 5c4b39164cff..d7b82e12cdd5 100644
--- a/eclass/virtualx.eclass
+++ b/eclass/virtualx.eclass
@@ -109,7 +109,7 @@ virtx() {
 	local retval=0
 	local OLD_SANDBOX_ON="${SANDBOX_ON}"
 	local XVFB XHOST XDISPLAY
-	local xvfbargs="-screen 0 1280x1024x24 +extension RANDR"
+	local xvfbargs=( -screen 0 1280x1024x24 +extension RANDR )
 	XVFB=$(type -p Xvfb) || die
 	XHOST=$(type -p xhost) || die
 
@@ -134,24 +134,24 @@ virtx() {
 	# We really do not want SANDBOX enabled here
 	export SANDBOX_ON="0"
 
-	debug-print "${FUNCNAME}: ${XVFB} :${XDISPLAY} ${xvfbargs}"
-	${XVFB} :${XDISPLAY} ${xvfbargs} &>/dev/null &
+	debug-print "${FUNCNAME}: ${XVFB} :${XDISPLAY} ${xvfbargs[*]}"
+	${XVFB} :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
 	sleep 2
 
 	local start=${XDISPLAY}
 	while [[ ! -f /tmp/.X${XDISPLAY}-lock ]]; do
 		# Stop trying after 15 tries
 		if ((XDISPLAY - start > 15)) ; then
-			eerror "'${XVFB} :${XDISPLAY} ${xvfbargs}' returns:"
+			eerror "'${XVFB} :${XDISPLAY} ${xvfbargs[*]}' returns:"
 			echo
-			${XVFB} :${XDISPLAY} ${xvfbargs}
+			${XVFB} :${XDISPLAY} "${xvfbargs[@]}"
 			echo
 			eerror "If possible, correct the above error and try your emerge again."
 			die "Unable to start Xvfb"
 		fi
 			((XDISPLAY++))
-		debug-print "${FUNCNAME}: ${XVFB} :${XDISPLAY} ${xvfbargs}"
-		${XVFB} :${XDISPLAY} ${xvfbargs} &>/dev/null &
+		debug-print "${FUNCNAME}: ${XVFB} :${XDISPLAY} ${xvfbargs[*]}"
+		${XVFB} :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
 		sleep 2
 	done
 
-- 
2.35.1

[gentoo-dev] [PATCH 3/6] virtualx.eclass: Remove unused XHOST

[Michał Górny]

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/virtualx.eclass | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/eclass/virtualx.eclass b/eclass/virtualx.eclass
index d7b82e12cdd5..05b89e814a42 100644
--- a/eclass/virtualx.eclass
+++ b/eclass/virtualx.eclass
@@ -108,10 +108,9 @@ virtx() {
 	local i=0
 	local retval=0
 	local OLD_SANDBOX_ON="${SANDBOX_ON}"
-	local XVFB XHOST XDISPLAY
+	local XVFB XDISPLAY
 	local xvfbargs=( -screen 0 1280x1024x24 +extension RANDR )
 	XVFB=$(type -p Xvfb) || die
-	XHOST=$(type -p xhost) || die
 
 	debug-print "${FUNCNAME}: running Xvfb hack"
 	export XAUTHORITY=
-- 
2.35.1

[gentoo-dev] [PATCH 4/6] virtualx.eclass: Inline XVFB

[Michał Górny]

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/virtualx.eclass | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/eclass/virtualx.eclass b/eclass/virtualx.eclass
index 05b89e814a42..d1c05e20b246 100644
--- a/eclass/virtualx.eclass
+++ b/eclass/virtualx.eclass
@@ -108,9 +108,8 @@ virtx() {
 	local i=0
 	local retval=0
 	local OLD_SANDBOX_ON="${SANDBOX_ON}"
-	local XVFB XDISPLAY
+	local XDISPLAY
 	local xvfbargs=( -screen 0 1280x1024x24 +extension RANDR )
-	XVFB=$(type -p Xvfb) || die
 
 	debug-print "${FUNCNAME}: running Xvfb hack"
 	export XAUTHORITY=
@@ -133,24 +132,24 @@ virtx() {
 	# We really do not want SANDBOX enabled here
 	export SANDBOX_ON="0"
 
-	debug-print "${FUNCNAME}: ${XVFB} :${XDISPLAY} ${xvfbargs[*]}"
-	${XVFB} :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
+	debug-print "${FUNCNAME}: Xvfb :${XDISPLAY} ${xvfbargs[*]}"
+	Xvfb :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
 	sleep 2
 
 	local start=${XDISPLAY}
 	while [[ ! -f /tmp/.X${XDISPLAY}-lock ]]; do
 		# Stop trying after 15 tries
 		if ((XDISPLAY - start > 15)) ; then
-			eerror "'${XVFB} :${XDISPLAY} ${xvfbargs[*]}' returns:"
+			eerror "'Xvfb :${XDISPLAY} ${xvfbargs[*]}' returns:"
 			echo
-			${XVFB} :${XDISPLAY} "${xvfbargs[@]}"
+			Xvfb :${XDISPLAY} "${xvfbargs[@]}"
 			echo
 			eerror "If possible, correct the above error and try your emerge again."
 			die "Unable to start Xvfb"
 		fi
 			((XDISPLAY++))
-		debug-print "${FUNCNAME}: ${XVFB} :${XDISPLAY} ${xvfbargs[*]}"
-		${XVFB} :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
+		debug-print "${FUNCNAME}: Xvfb :${XDISPLAY} ${xvfbargs[*]}"
+		Xvfb :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
 		sleep 2
 	done
 
-- 
2.35.1

[gentoo-dev] [PATCH 5/6] virtualx.eclass: Let Xvfb figure out the free DISPLAY

[Michał Górny]

Replace the antiquated search mechanism for a free DISPLAY with Xvfb's
-displayfd option that makes Xvfb choose one itself and print it to
given fd.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/virtualx.eclass | 62 ++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 42 deletions(-)

diff --git a/eclass/virtualx.eclass b/eclass/virtualx.eclass
index d1c05e20b246..c0af095e89a5 100644
--- a/eclass/virtualx.eclass
+++ b/eclass/virtualx.eclass
@@ -108,65 +108,43 @@ virtx() {
 	local i=0
 	local retval=0
 	local OLD_SANDBOX_ON="${SANDBOX_ON}"
-	local XDISPLAY
 	local xvfbargs=( -screen 0 1280x1024x24 +extension RANDR )
 
 	debug-print "${FUNCNAME}: running Xvfb hack"
 	export XAUTHORITY=
-	# The following is derived from Mandrake's hack to allow
-	# compiling without the X display
-
-	einfo "Scanning for an open DISPLAY to start Xvfb ..."
-	# If we are in a chrooted environment, and there is already a
-	# X server started outside of the chroot, Xvfb will fail to start
-	# on the same display (most cases this is :0 ), so make sure
-	# Xvfb is started, else bump the display number
-	#
-	# Azarah - 5 May 2002
-	# GNOME GDM may have started X on DISPLAY :0 with a
-	# lock file /tmp/.X1024-lock, therefore start the search at 1.
-	# Else a leftover /tmp/.X1-lock will prevent finding an available display.
-	XDISPLAY=$(i=1; while [[ -f /tmp/.X${i}-lock ]] ; do ((i++));done; echo ${i})
-	debug-print "${FUNCNAME}: XDISPLAY=${XDISPLAY}"
+
+	einfo "Starting Xvfb ..."
 
 	# We really do not want SANDBOX enabled here
 	export SANDBOX_ON="0"
 
-	debug-print "${FUNCNAME}: Xvfb :${XDISPLAY} ${xvfbargs[*]}"
-	Xvfb :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
-	sleep 2
-
-	local start=${XDISPLAY}
-	while [[ ! -f /tmp/.X${XDISPLAY}-lock ]]; do
-		# Stop trying after 15 tries
-		if ((XDISPLAY - start > 15)) ; then
-			eerror "'Xvfb :${XDISPLAY} ${xvfbargs[*]}' returns:"
-			echo
-			Xvfb :${XDISPLAY} "${xvfbargs[@]}"
-			echo
-			eerror "If possible, correct the above error and try your emerge again."
-			die "Unable to start Xvfb"
-		fi
-			((XDISPLAY++))
-		debug-print "${FUNCNAME}: Xvfb :${XDISPLAY} ${xvfbargs[*]}"
-		Xvfb :${XDISPLAY} "${xvfbargs[@]}" &>/dev/null &
-		sleep 2
-	done
+	debug-print "${FUNCNAME}: Xvfb -displayfd 1 ${xvfbargs[*]}"
+	local logfile=${T}/Xvfb.log
+	local pidfile=${T}/Xvfb.pid
+	# NB: bash command substitution blocks until Xvfb prints fd to stdout
+	# and then closes the fd; only then it backgrounds properly
+	export DISPLAY=:$(
+		Xvfb -displayfd 1 "${xvfbargs[@]}" 2>"${logfile}" &
+		echo "$!" > "${pidfile}"
+	)
+
+	if [[ ${DISPLAY} == : ]]; then
+		eerror "Xvfb failed to start, reprinting error log"
+		cat "${logfile}"
+		die "Xvfb failed to start"
+	fi
 
 	# Now enable SANDBOX again if needed.
 	export SANDBOX_ON="${OLD_SANDBOX_ON}"
 
-	einfo "Starting Xvfb on \$DISPLAY=${XDISPLAY} ..."
-
-	export DISPLAY=:${XDISPLAY}
-	# Do not break on error, but setup $retval, as we need
-	# to kill Xvfb
+	# Do not break on error, but setup $retval, as we need to kill Xvfb
+	einfo "Xvfb started on DISPLAY=${DISPLAY}"
 	debug-print "${FUNCNAME}: $@"
 	nonfatal "$@"
 	retval=$?
 
 	# Now kill Xvfb
-	kill $(cat /tmp/.X${XDISPLAY}-lock)
+	kill "$(<"${pidfile}")"
 
 	# die if our command failed
 	[[ ${retval} -ne 0 ]] && die "Failed to run '$@'"
-- 
2.35.1

[gentoo-dev] [PATCH 6/6] virtualx.eclass: Do not disable sandbox

[Michał Górny]

The code responsible for disabling sandbox dates back to 2002 and does
not include any explanation.  Let's try if we still need it.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/virtualx.eclass | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/eclass/virtualx.eclass b/eclass/virtualx.eclass
index c0af095e89a5..e04834d39bd1 100644
--- a/eclass/virtualx.eclass
+++ b/eclass/virtualx.eclass
@@ -107,7 +107,6 @@ virtx() {
 
 	local i=0
 	local retval=0
-	local OLD_SANDBOX_ON="${SANDBOX_ON}"
 	local xvfbargs=( -screen 0 1280x1024x24 +extension RANDR )
 
 	debug-print "${FUNCNAME}: running Xvfb hack"
@@ -115,9 +114,6 @@ virtx() {
 
 	einfo "Starting Xvfb ..."
 
-	# We really do not want SANDBOX enabled here
-	export SANDBOX_ON="0"
-
 	debug-print "${FUNCNAME}: Xvfb -displayfd 1 ${xvfbargs[*]}"
 	local logfile=${T}/Xvfb.log
 	local pidfile=${T}/Xvfb.pid
@@ -134,9 +130,6 @@ virtx() {
 		die "Xvfb failed to start"
 	fi
 
-	# Now enable SANDBOX again if needed.
-	export SANDBOX_ON="${OLD_SANDBOX_ON}"
-
 	# Do not break on error, but setup $retval, as we need to kill Xvfb
 	einfo "Xvfb started on DISPLAY=${DISPLAY}"
 	debug-print "${FUNCNAME}: $@"
-- 
2.35.1

Re: [gentoo-dev] [PATCH 5/6] virtualx.eclass: Let Xvfb figure out the free DISPLAY

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]



> On 27 Jul 2022, at 08:17, Michał Górny <mgorny@gentoo.org> wrote:
> 
> Replace the antiquated search mechanism for a free DISPLAY with Xvfb's
> -displayfd option that makes Xvfb choose one itself and print it to
> given fd.
> 

Tag https://bugs.gentoo.org/494244 given it's likely to fix it.

> Signed-off-by: Michał Górny <mgorny@gentoo.org>
> ---
> eclass/virtualx.eclass | 62 ++++++++++++++----------------------------
> 1 file changed, 20 insertions(+), 42 deletions(-)


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [gentoo-dev] [PATCH 6/6] virtualx.eclass: Do not disable sandbox

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]



> On 27 Jul 2022, at 08:17, Michał Górny <mgorny@gentoo.org> wrote:
> 
> The code responsible for disabling sandbox dates back to 2002 and does
> not include any explanation.  Let's try if we still need it.
> 
> Signed-off-by: Michał Górny <mgorny@gentoo.org>
> ---

I'd tag https://bugs.gentoo.org/107479.

> eclass/virtualx.eclass | 7 -------
> 1 file changed, 7 deletions(-)


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [gentoo-dev] [PATCH 1/6] virtualx.eclass: Add quoting to workaround vim syntax hl bug

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 197 bytes --]

>>>>> On Wed, 27 Jul 2022, Michał Górny wrote:

> -		[[ ${VIRTUALX_REQUIRED} == test ]] &&
> +		[[ ${VIRTUALX_REQUIRED} == "test" ]] &&

Really? You should rather fix vim, or use Emacs. :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

Re: [gentoo-dev] [PATCH 1/6] virtualx.eclass: Add quoting to workaround vim syntax hl bug

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]



> On 27 Jul 2022, at 08:17, Michał Górny <mgorny@gentoo.org> wrote:
> 
> Signed-off-by: Michał Górny <mgorny@gentoo.org>
> ---
> eclass/virtualx.eclass | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 

Please reference the vim bug (guessing it's https://github.com/vim/vim/issues/10673)
in the commit message and in the eclass I guess so that we know when it's fixed.

I love my bug links, okay?

> diff --git a/eclass/virtualx.eclass b/eclass/virtualx.eclass
> index b7a25c897b4a..5c4b39164cff 100644
> --- a/eclass/virtualx.eclass
> +++ b/eclass/virtualx.eclass
> @@ -53,7 +53,7 @@ case ${VIRTUALX_REQUIRED} in
> 	*)
> 		BDEPEND="${VIRTUALX_REQUIRED}? ( ${VIRTUALX_DEPEND} )"
> 		IUSE="${VIRTUALX_REQUIRED}"
> -		[[ ${VIRTUALX_REQUIRED} == test ]] &&
> +		[[ ${VIRTUALX_REQUIRED} == "test" ]] &&
> 			RESTRICT+=" !test? ( test )"
> 		;;
> esac
> --
> 2.35.1
> 
> 


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]