Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
patch 8.1.0215: no error if configure --with-x cannot configure X 
Problem:    No error if configure --with-x cannot configure X.
Solution:   Check that when --with-x is used X can be configured.
  • Loading branch information
@brammool
brammool committed Jul 27, 2018
1 parent 83ec2a7 commit d2a0549
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/auto/configure
View file
Expand Up @@ -4466,6 +4466,8 @@ fi
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $fail_if_missing" >&5
$as_echo "$fail_if_missing" >&6; }

with_x_arg="$with_x"

if test -z "$CFLAGS"; then
CFLAGS="-O"
test "$GCC" = yes && CFLAGS="-O2 -fno-strength-reduce -Wall"
Expand Down Expand Up @@ -9003,6 +9005,10 @@ $as_echo "$ac_cv_small_wchar_t" >&6; }
fi
fi

if test "x$with_x" = xno -a "x$with_x_arg" = xyes; then
as_fn_error $? "could not configure X" "$LINENO" 5
fi

test "x$with_x" = xno -a "x$MACOS_X" != "xyes" -a "x$QNX" != "xyes" && enable_gui=no

{ $as_echo "$as_me:${as_lineno-$LINENO}: checking --enable-gui argument" >&5
Expand Down
8 changes: 8 additions & 0 deletions src/configure.ac
View file
Expand Up @@ -72,6 +72,9 @@ AC_ARG_ENABLE(fail_if_missing,
[fail_if_missing="no"])
AC_MSG_RESULT($fail_if_missing)

dnl Keep original value to check later.
with_x_arg="$with_x"

dnl Set default value for CFLAGS if none is defined or it's empty
if test -z "$CFLAGS"; then
CFLAGS="-O"
Expand Down Expand Up @@ -2283,6 +2286,11 @@ else
fi
fi

dnl Check if --with-x was given but it doesn't work.
if test "x$with_x" = xno -a "x$with_x_arg" = xyes; then
AC_MSG_ERROR([could not configure X])
fi

test "x$with_x" = xno -a "x$MACOS_X" != "xyes" -a "x$QNX" != "xyes" && enable_gui=no

AC_MSG_CHECKING(--enable-gui argument)
Expand Down
2 changes: 2 additions & 0 deletions src/version.c
View file
Expand Up @@ -798,6 +798,8 @@ static char *(features[]) =

static int included_patches[] =
{ /* Add new patch number below this line */
/**/
215,
/**/
214,
/**/
Expand Down

7 comments on commit d2a0549

@RandomDSdevel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brammool:

     This commit, as pulled downstream to Homebrew by Homebrew/homebrew-core#30881 (most of the time, Homebrew pulls new Vim releases down every 50th patch to prevent spamming the core tap, or package repository, with a large number of commits from the same upstream project,) has exposed that the Vim's configure script's ac_x_header_dirs variable:

vim/src/auto/configure

Lines 8101 to 8140 in af559d2

# Standard set of common directories for X headers.
# Check X11 before X11Rn because it is often a symlink to the current release.
ac_x_header_dirs='
/usr/X11/include
/usr/X11R7/include
/usr/X11R6/include
/usr/X11R5/include
/usr/X11R4/include
/usr/include/X11
/usr/include/X11R7
/usr/include/X11R6
/usr/include/X11R5
/usr/include/X11R4
/usr/local/X11/include
/usr/local/X11R7/include
/usr/local/X11R6/include
/usr/local/X11R5/include
/usr/local/X11R4/include
/usr/local/include/X11
/usr/local/include/X11R7
/usr/local/include/X11R6
/usr/local/include/X11R5
/usr/local/include/X11R4
/usr/X386/include
/usr/x386/include
/usr/XFree86/include/X11
/usr/include
/usr/local/include
/usr/unsupported/include
/usr/athena/include
/usr/local/x11r5/include
/usr/lpp/Xamples/include
/usr/openwin/include
/usr/openwin/share/include'

doesn't include anything referencing /opt/X11, the standard X11 prefix under the X11 distribution provided to users of OS X/macOS (Darwin) by the third-party XQuartz project.
     This makes the X include-/library-path detection code that follows overlook X headers and libraries from directories inside that location, leaving your configure script's X header and library search-path cache values empty. In the end, this causes brew install vim, when invoked with the relevant downstream build formula's, or recipe's, --with-client-server option (which, as seen here, passes Vim's configure script its --with-x option,) to fail. That downstream code could be fixed there to pass the correct --x-includes and --x-libraries options needed to cover this case, but I thought it might end up being a better idea to get this fixed upstream and then pull a new release down. I'd just submit a PR to add /opt/X11/include to ac_x_header_dirs, but a first look at that part of the script suggests that doing that might only take care of looking up the location of X headers, but not libraries. I can open an issue for this if you'd prefer not to continue discussion here.
     (Previously, I suppose Vim's configure was just silently ignoring --with-x in the case I describe here, which was why nobody had picked up on this issue before now…unless some older behavior handled things correctly and nobody noticed when they broke it…? 🤷‍♂️)

@nuko8
Copy link

@nuko8 nuko8 commented on d2a0549 Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the X include-/library-path detection code that follows overlook X headers and libraries from directories inside that location, leaving your configure script's X header and library search-path cache values empty. In the end, this causes brew install vim, when invoked with the relevant downstream build formula's, or recipe's, --with-client-server option (which, as seen here, passes Vim's configure script its --with-x option,) to fail.

That shouldn't happen since, when XQuartz is installed successfully, it creates the file /usr/X11 which is a symbolic link to /opt/X11, so that the resulting installation will comply with the standard X11 installation which can be seen with other Unix systems. For instance, on my macOS, I have

% ls -ld /usr/X11
lrwxr-xr-x 1 root wheel 8 Oct 31  2016 /usr/X11 -> /opt/X11/

and I've never experienced the failure you mentioned, though I'm not sure the details of it.

So the mentioned failure with Homebrew might come from other reasons and hence for further discussion, a concrete example of the failure would be helpful.

@RandomDSdevel
Copy link

@RandomDSdevel RandomDSdevel commented on d2a0549 Aug 9, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nuko8:

     Whoops, guess I overlooked that and/or forgot that happened. Maybe this is a Homebrew issue after all, then… In that case, I'll open an issue downstream (with logs attached, naturally) and CC you and @brammool on it.

@brammool
Copy link
Member Author

@brammool brammool commented on d2a0549 Aug 9, 2018 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RandomDSdevel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brammool:

⋮

If it was previously failing, …

     I didn't know it was, though, until this commit interacted with my build/installation environment to prove otherwise. (Previously, it failed silently, ignoring the problem and proceeding on without any other hiccups.)

…then why not remove --with-x ?

⋮

     I'd like to restore my current configuration (modulo updates) to full working order if at all possible. That is another solution, however, and is what I'll have to do if we can't resolve this issue (which, however, I doubt will be a problem.)

⋮

We can also try to make Vim find the files…

     Agreed; hopefully this turns out to be simple like one would suspect.

…, but that's secondary…

     See the second part of this reply.

…(I suppose they could also be missing).

⋮

     I can take a look in a minute and see what's what.

@RandomDSdevel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brammool, @nuko8: Issue filed downstream as Homebrew/homebrew-core#30949.

@RandomDSdevel
Copy link

@RandomDSdevel RandomDSdevel commented on d2a0549 Aug 9, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brammool:

     Sorry to keep you waiting; here's what those parts of my filesystem look like:

Bryces-HD-1-TB:~ zadmin$ ls -@aHhlO /opt/X11/
total 0
drwxr-xr-x    9 root  wheel  -       306B Sep 27  2016 .
drwxr-xr-x@   4 root  wheel  hidden  136B Nov 14  2016 ..
	com.apple.FinderInfo	  32B 
drwxr-xr-x  128 root  wheel  -       4.3K Nov 14  2016 bin
drwxr-xr-x    4 root  wheel  -       136B Oct 29  2016 etc
drwxr-xr-x   19 root  wheel  -       646B Nov 14  2016 include
drwxr-xr-x  204 root  wheel  -       6.8K Nov 14  2016 lib
drwxr-xr-x    4 root  wheel  -       136B Oct 26  2016 libexec
drwxr-xr-x   14 root  wheel  -       476B Sep 27  2016 share
drwxr-xr-x    5 root  wheel  -       170B Sep 27  2016 var
Bryces-HD-1-TB:~ zadmin$ ls -@aHhlO /usr/X11
X11/   X11R6/ 
Bryces-HD-1-TB:~ zadmin$ ls -@aHhlO /usr/X11/
total 0
drwxr-xr-x    9 root  wheel  -       306B Sep 27  2016 .
drwxr-xr-x@   4 root  wheel  hidden  136B Nov 14  2016 ..
	com.apple.FinderInfo	  32B 
drwxr-xr-x  128 root  wheel  -       4.3K Nov 14  2016 bin
drwxr-xr-x    4 root  wheel  -       136B Oct 29  2016 etc
drwxr-xr-x   19 root  wheel  -       646B Nov 14  2016 include
drwxr-xr-x  204 root  wheel  -       6.8K Nov 14  2016 lib
drwxr-xr-x    4 root  wheel  -       136B Oct 26  2016 libexec
drwxr-xr-x   14 root  wheel  -       476B Sep 27  2016 share
drwxr-xr-x    5 root  wheel  -       170B Sep 27  2016 var
Bryces-HD-1-TB:~ zadmin$ ls -@aHhlO /usr/X11R6/
total 0
drwxr-xr-x    9 root  wheel  -       306B Sep 27  2016 .
drwxr-xr-x@   4 root  wheel  hidden  136B Nov 14  2016 ..
	com.apple.FinderInfo	  32B 
drwxr-xr-x  128 root  wheel  -       4.3K Nov 14  2016 bin
drwxr-xr-x    4 root  wheel  -       136B Oct 29  2016 etc
drwxr-xr-x   19 root  wheel  -       646B Nov 14  2016 include
drwxr-xr-x  204 root  wheel  -       6.8K Nov 14  2016 lib
drwxr-xr-x    4 root  wheel  -       136B Oct 26  2016 libexec
drwxr-xr-x   14 root  wheel  -       476B Sep 27  2016 share
drwxr-xr-x    5 root  wheel  -       170B Sep 27  2016 var

So it looks to me like everything's there. Maybe this is a(n) (downstream) issue with Homebrew's sandbox…? 🤔🤷‍♂️

Please sign in to comment.