bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

It's a combination that won't compile, so it's probably best to error
out during configure.

diff --git a/ChangeLog b/ChangeLog
index 4365668..dec9ce0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-20  rpluim  <rpluim@gmail.com>
+
+	* configure.ac: Error out if with-file-notification=w32 is
+	specified on cygwin.
+
 2015-02-13  Jan Djärv  <jan.h.d@swipnet.se>
 
 	* configure.ac: Set locallisppath to empty for NS self contained,
diff --git a/configure.ac b/configure.ac
index 0bcc55c..048168a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2659,6 +2659,10 @@ fi
 
 dnl MS Windows native file monitor is available for mingw32 only.
 case $with_file_notification,$opsys in
+  w32,cygwin)
+    AC_MSG_ERROR([`--with-file-notification=w32' was specified, but
+    this is only supported on MS-Windows native and MinGW32 builds])
+    ;;
   w32,* | yes,mingw32)
     AC_CHECK_HEADER(windows.h)
     if test "$ac_cv_header_windows_h" = yes ; then

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 20 Feb 2015 10:47:36 +0100
> 
> It's a combination that won't compile, so it's probably best to error
> out during configure.

Why doesn't it compile?  Perhaps it's better to allow it to compile,
than to error out.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 20 Feb 2015 10:47:36 +0100
>> 
>> It's a combination that won't compile, so it's probably best to error
>> out during configure.
>
> Why doesn't it compile?  Perhaps it's better to allow it to compile,
> than to error out.

I guess whoever put the following in w32.h had his reasons:

#ifdef CYGWIN
#error "w32.h is not compatible with Cygwin"
#endif


In file included from w32notify.c:95:0:
w32.h:23:2: error: #error "w32.h is not compatible with Cygwin"
 #error "w32.h is not compatible with Cygwin"
  ^
In file included from ../lib/sys/types.h:28:0,
                 from ../lib/signal.h:49,
                 from /usr/include/machine/setjmp.h:357,
                 from /usr/include/setjmp.h:10,
                 from lisp.h:24,
                 from w32notify.c:92:

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 20 Feb 2015 11:19:16 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Robert Pluim <rpluim@gmail.com>
> >> Date: Fri, 20 Feb 2015 10:47:36 +0100
> >> 
> >> It's a combination that won't compile, so it's probably best to error
> >> out during configure.
> >
> > Why doesn't it compile?  Perhaps it's better to allow it to compile,
> > than to error out.
> 
> I guess whoever put the following in w32.h had his reasons:
> 
> #ifdef CYGWIN
> #error "w32.h is not compatible with Cygwin"
> #endif

According to the comment in w32notify.c, w32.h is included only for
w32_strerror.  Could you try commenting out the w32.h inclusion, and
instead copy the w32_strerror prototype into w32notify.c?  Does Emacs
compile then as part of the Cygwin-w32 build, and do file
notifications work?  (I don't have Cygwin installed to try that,
sorry.)

If getting the prototype of w32_strerror is the only problem, then
it's easy to solve in a way that will be compatible with the
Cygwin-w32 build.

Thanks.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 20 Feb 2015 11:19:16 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Robert Pluim <rpluim@gmail.com>
>> >> Date: Fri, 20 Feb 2015 10:47:36 +0100
>> >> 
>> >> It's a combination that won't compile, so it's probably best to error
>> >> out during configure.
>> >
>> > Why doesn't it compile?  Perhaps it's better to allow it to compile,
>> > than to error out.
>> 
>> I guess whoever put the following in w32.h had his reasons:
>> 
>> #ifdef CYGWIN
>> #error "w32.h is not compatible with Cygwin"
>> #endif
>
> According to the comment in w32notify.c, w32.h is included only for
> w32_strerror.  Could you try commenting out the w32.h inclusion, and
> instead copy the w32_strerror prototype into w32notify.c?  Does Emacs
> compile then as part of the Cygwin-w32 build, and do file
> notifications work?  (I don't have Cygwin installed to try that,
> sorry.)
>

No, that doesn't compile. w32notify.c needs (at least)
w32_unicode_filenames, filename_to_utf16(), filename_to_ansi() and
w32_valid_pointer_p() from w32.c.

I don't think adding w32.c into the build on cygwin is going to work.

Regards

Robert

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 20 Feb 2015 15:16:49 +0100
> 
> > According to the comment in w32notify.c, w32.h is included only for
> > w32_strerror.  Could you try commenting out the w32.h inclusion, and
> > instead copy the w32_strerror prototype into w32notify.c?  Does Emacs
> > compile then as part of the Cygwin-w32 build, and do file
> > notifications work?  (I don't have Cygwin installed to try that,
> > sorry.)
> >
> 
> No, that doesn't compile. w32notify.c needs (at least)
> w32_unicode_filenames, filename_to_utf16(), filename_to_ansi() and
> w32_valid_pointer_p() from w32.c.

Right.  But those problems are quite simple to solve.

Ken, would you like to try to solve them?  I could suggest a patch,
but it would be 100% untested.

> I don't think adding w32.c into the build on cygwin is going to work.

Because of the above, or because of something else?

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 20 Feb 2015 15:16:49 +0100
>> 
>> > According to the comment in w32notify.c, w32.h is included only for
>> > w32_strerror.  Could you try commenting out the w32.h inclusion, and
>> > instead copy the w32_strerror prototype into w32notify.c?  Does Emacs
>> > compile then as part of the Cygwin-w32 build, and do file
>> > notifications work?  (I don't have Cygwin installed to try that,
>> > sorry.)
>> >
>> 
>> No, that doesn't compile. w32notify.c needs (at least)
>> w32_unicode_filenames, filename_to_utf16(), filename_to_ansi() and
>> w32_valid_pointer_p() from w32.c.
>
> Right.  But those problems are quite simple to solve.
>
> Ken, would you like to try to solve them?  I could suggest a patch,
> but it would be 100% untested.
>
>> I don't think adding w32.c into the build on cygwin is going to work.
>
> Because of the above, or because of something else?

Because (perhaps mistakenly) I think that calling emacs code that
assumes it's running under Windows native will not interact well with
emacs code that's running under cygwin.

Having said that, I'm willing to test any patches over the next week or
so (after that I'm going to be without a cygwin environment for a couple
of weeks).

Robert

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Ken Brown]

On 2/20/2015 10:10 AM, Eli Zaretskii wrote:
>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 20 Feb 2015 15:16:49 +0100
>>
>>> According to the comment in w32notify.c, w32.h is included only for
>>> w32_strerror.  Could you try commenting out the w32.h inclusion, and
>>> instead copy the w32_strerror prototype into w32notify.c?  Does Emacs
>>> compile then as part of the Cygwin-w32 build, and do file
>>> notifications work?  (I don't have Cygwin installed to try that,
>>> sorry.)
>>>
>>
>> No, that doesn't compile. w32notify.c needs (at least)
>> w32_unicode_filenames, filename_to_utf16(), filename_to_ansi() and
>> w32_valid_pointer_p() from w32.c.
>
> Right.  But those problems are quite simple to solve.
>
> Ken, would you like to try to solve them?  I could suggest a patch,
> but it would be 100% untested.

I'm busy with other things at the moment and can't get to it right away. 
  If it's easy for you to suggest a patch, why don't you go ahead, and 
Robert can test it.  Or you can wait a couple weeks until I can look at it.

BTW, file-notification=gfile works fine in the Cygwin-w32 build, so I'm 
not sure there's any need to put a lot of effort into this.

Ken

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 20 Feb 2015 16:46:48 +0100
> 
> >> I don't think adding w32.c into the build on cygwin is going to work.
> >
> > Because of the above, or because of something else?
> 
> Because (perhaps mistakenly) I think that calling emacs code that
> assumes it's running under Windows native will not interact well with
> emacs code that's running under cygwin.

Sorry, maybe I'm confused, but isn't your Cygwin build configured
"--with-w32"?  If so, it already uses some of the Windows native code.

> Having said that, I'm willing to test any patches over the next week or
> so

Thanks.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Fri, 20 Feb 2015 16:46:48 +0100
>> 
>> >> I don't think adding w32.c into the build on cygwin is going to work.
>> >
>> > Because of the above, or because of something else?
>> 
>> Because (perhaps mistakenly) I think that calling emacs code that
>> assumes it's running under Windows native will not interact well with
>> emacs code that's running under cygwin.
>
> Sorry, maybe I'm confused, but isn't your Cygwin build configured
> "--with-w32"?  If so, it already uses some of the Windows native code.
>

Yes, but only the bits relevant to the GUI. I don't know what the rest
of it does :)

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

Ken Brown <kbrown@cornell.edu> writes:

> On 2/20/2015 10:10 AM, Eli Zaretskii wrote:
>>> From: Robert Pluim <rpluim@gmail.com>
>>> Date: Fri, 20 Feb 2015 15:16:49 +0100
>>>
>>>> According to the comment in w32notify.c, w32.h is included only for
>>>> w32_strerror.  Could you try commenting out the w32.h inclusion, and
>>>> instead copy the w32_strerror prototype into w32notify.c?  Does Emacs
>>>> compile then as part of the Cygwin-w32 build, and do file
>>>> notifications work?  (I don't have Cygwin installed to try that,
>>>> sorry.)
>>>>
>>>
>>> No, that doesn't compile. w32notify.c needs (at least)
>>> w32_unicode_filenames, filename_to_utf16(), filename_to_ansi() and
>>> w32_valid_pointer_p() from w32.c.
>>
>> Right.  But those problems are quite simple to solve.
>>
>> Ken, would you like to try to solve them?  I could suggest a patch,
>> but it would be 100% untested.
>
> I'm busy with other things at the moment and can't get to it right
> away. If it's easy for you to suggest a patch, why don't you go ahead,
> and Robert can test it.  Or you can wait a couple weeks until I can
> look at it.
>
> BTW, file-notification=gfile works fine in the Cygwin-w32 build, so
> I'm not sure there's any need to put a lot of effort into this.

I didn't know that.  I'll give it a go.  Perhaps the following slight
modification to my original patch?


diff --git a/ChangeLog b/ChangeLog
index 4365668..dec9ce0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-20  rpluim  <rpluim@gmail.com>
+
+	* configure.ac: Error out if with-file-notification=w32 is
+	specified on cygwin.
+
 2015-02-13  Jan Djärv  <jan.h.d@swipnet.se>
 
 	* configure.ac: Set locallisppath to empty for NS self contained,
diff --git a/configure.ac b/configure.ac
index 0bcc55c..19d9c20 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2659,6 +2659,11 @@ fi
 
 dnl MS Windows native file monitor is available for mingw32 only.
 case $with_file_notification,$opsys in
+  w32,cygwin)
+    AC_MSG_ERROR([`--with-file-notification=w32' was specified, but
+    this is only supported on MS-Windows native and MinGW32 builds.
+    Consider using gfile instead.])
+    ;;
   w32,* | yes,mingw32)
     AC_CHECK_HEADER(windows.h)
     if test "$ac_cv_header_windows_h" = yes ; then

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 20 Feb 2015 17:44:30 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Robert Pluim <rpluim@gmail.com>
> >> Date: Fri, 20 Feb 2015 16:46:48 +0100
> >> 
> >> >> I don't think adding w32.c into the build on cygwin is going to work.
> >> >
> >> > Because of the above, or because of something else?
> >> 
> >> Because (perhaps mistakenly) I think that calling emacs code that
> >> assumes it's running under Windows native will not interact well with
> >> emacs code that's running under cygwin.
> >
> > Sorry, maybe I'm confused, but isn't your Cygwin build configured
> > "--with-w32"?  If so, it already uses some of the Windows native code.
> 
> Yes, but only the bits relevant to the GUI. I don't know what the rest
> of it does :)

Most of native Windows code in Emacs is related to GUI.  That includes
drawing GUI frames and accepting input from the Windows message queue;
both are used by the Cygwin build --with-w32.

The rest is emulation of Posix functions, which Cygwin doesn't need.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> Date: Fri, 20 Feb 2015 11:15:18 -0500
> From: Ken Brown <kbrown@cornell.edu>
> CC: bug-gnu-emacs@gnu.org, Robert Pluim <rpluim@gmail.com>
> 
> I'm busy with other things at the moment and can't get to it right away. 
>   If it's easy for you to suggest a patch, why don't you go ahead, and 
> Robert can test it.  Or you can wait a couple weeks until I can look at it.

OK, I will try to find time.

> BTW, file-notification=gfile works fine in the Cygwin-w32 build, so I'm 
> not sure there's any need to put a lot of effort into this.

If it turns out to need a lot of effort, I'll agree with you.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Ken Brown]

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

On 2/20/2015 12:18 PM, Eli Zaretskii wrote:
>> Date: Fri, 20 Feb 2015 11:15:18 -0500
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: bug-gnu-emacs@gnu.org, Robert Pluim <rpluim@gmail.com>
>>
>> I'm busy with other things at the moment and can't get to it right away.
>>    If it's easy for you to suggest a patch, why don't you go ahead, and
>> Robert can test it.  Or you can wait a couple weeks until I can look at it.
>
> OK, I will try to find time.
>
>> BTW, file-notification=gfile works fine in the Cygwin-w32 build, so I'm
>> not sure there's any need to put a lot of effort into this.
>
> If it turns out to need a lot of effort, I'll agree with you.

It looks like it might not take much effort.  What I did for now, just 
for testing purposes, is to copy into w32notify.c whatever was necessary 
to make the Cygwin-w32 build compile with w32 file notification.  I 
tested it briefly and it seems to work.  Obviously it needs much more 
testing to make sure this doesn't break anything else, and then we can 
worry about how to do this right.

Robert, please test when you get a chance (patch attached).

Ken

[-- Attachment #2: cygw32_notify.patch --]
[-- Type: text/plain, Size: 5852 bytes --]

diff --git a/src/w32notify.c b/src/w32notify.c
index ab6cd12..71ee1a3 100644
--- a/src/w32notify.c
+++ b/src/w32notify.c
@@ -92,12 +92,216 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "lisp.h"
 #include "w32term.h"	/* for enter_crit/leave_crit and WM_EMACS_FILENOTIFY */
 #include "w32common.h"	/* for OS version data */
-#include "w32.h"	/* for w32_strerror */
 #include "coding.h"
 #include "keyboard.h"
 #include "frame.h"	/* needed by termhooks.h */
 #include "termhooks.h"	/* for FILE_NOTIFY_EVENT */
 
+/* Experimental code for Cygwin-w32 build */
+
+extern char * w32_strerror (int error_no);
+
+int (WINAPI *pMultiByteToWideChar)(UINT,DWORD,LPCSTR,int,LPWSTR,int);
+int (WINAPI *pWideCharToMultiByte)(UINT,DWORD,LPCWSTR,int,LPSTR,int,LPCSTR,LPBOOL);
+
+#define MAXPATHLEN      MAX_PATH
+#define MAX_UTF8_PATH   (MAXPATHLEN * 4)
+
+static int file_name_codepage;
+
+static int
+codepage_for_filenames (CPINFO *cp_info)
+{
+  /* A simple cache to avoid calling GetCPInfo every time we need to
+     encode/decode a file name.  The file-name encoding is not
+     supposed to be changed too frequently, if ever.  */
+  static Lisp_Object last_file_name_encoding;
+  static CPINFO cp;
+  Lisp_Object current_encoding;
+
+  current_encoding = Vfile_name_coding_system;
+  if (NILP (current_encoding))
+    current_encoding = Vdefault_file_name_coding_system;
+
+  if (!EQ (last_file_name_encoding, current_encoding))
+    {
+      /* Default to the current ANSI codepage.  */
+      file_name_codepage = w32_ansi_code_page;
+
+      if (NILP (current_encoding))
+	{
+	  char *cpname = SDATA (SYMBOL_NAME (current_encoding));
+	  char *cp = NULL, *end;
+	  int cpnum;
+
+	  if (strncmp (cpname, "cp", 2) == 0)
+	    cp = cpname + 2;
+	  else if (strncmp (cpname, "windows-", 8) == 0)
+	    cp = cpname + 8;
+
+	  if (cp)
+	    {
+	      end = cp;
+	      cpnum = strtol (cp, &end, 10);
+	      if (cpnum && *end == '\0' && end - cp >= 2)
+		file_name_codepage = cpnum;
+	    }
+	}
+
+      if (!file_name_codepage)
+	file_name_codepage = CP_ACP; /* CP_ACP = 0, but let's not assume that */
+
+      if (!GetCPInfo (file_name_codepage, &cp))
+	{
+	  file_name_codepage = CP_ACP;
+	  if (!GetCPInfo (file_name_codepage, &cp))
+	    emacs_abort ();
+	}
+    }
+  if (cp_info)
+    *cp_info = cp;
+
+  return file_name_codepage;
+}
+
+int
+filename_to_utf16 (const char *fn_in, wchar_t *fn_out)
+{
+  int result = pMultiByteToWideChar (CP_UTF8, MB_ERR_INVALID_CHARS, fn_in, -1,
+				     fn_out, MAX_PATH);
+
+  if (!result)
+    {
+      DWORD err = GetLastError ();
+
+      switch (err)
+	{
+	case ERROR_INVALID_FLAGS:
+	case ERROR_INVALID_PARAMETER:
+	  errno = EINVAL;
+	  break;
+	case ERROR_INSUFFICIENT_BUFFER:
+	case ERROR_NO_UNICODE_TRANSLATION:
+	default:
+	  errno = ENOENT;
+	  break;
+	}
+      return -1;
+    }
+  return 0;
+}
+
+int
+filename_from_utf16 (const wchar_t *fn_in, char *fn_out)
+{
+  int result = pWideCharToMultiByte (CP_UTF8, 0, fn_in, -1,
+				     fn_out, MAX_UTF8_PATH, NULL, NULL);
+
+  if (!result)
+    {
+      DWORD err = GetLastError ();
+
+      switch (err)
+	{
+	case ERROR_INVALID_FLAGS:
+	case ERROR_INVALID_PARAMETER:
+	  errno = EINVAL;
+	  break;
+	case ERROR_INSUFFICIENT_BUFFER:
+	case ERROR_NO_UNICODE_TRANSLATION:
+	default:
+	  errno = ENOENT;
+	  break;
+	}
+      return -1;
+    }
+  return 0;
+}
+
+int
+filename_to_ansi (const char *fn_in, char *fn_out)
+{
+  wchar_t fn_utf16[MAX_PATH];
+
+  if (filename_to_utf16 (fn_in, fn_utf16) == 0)
+    {
+      int result;
+      int codepage = codepage_for_filenames (NULL);
+
+      result  = pWideCharToMultiByte (codepage, 0, fn_utf16, -1,
+				      fn_out, MAX_PATH, NULL, NULL);
+      if (!result)
+	{
+	  DWORD err = GetLastError ();
+
+	  switch (err)
+	    {
+	    case ERROR_INVALID_FLAGS:
+	    case ERROR_INVALID_PARAMETER:
+	      errno = EINVAL;
+	      break;
+	    case ERROR_INSUFFICIENT_BUFFER:
+	    case ERROR_NO_UNICODE_TRANSLATION:
+	    default:
+	      errno = ENOENT;
+	      break;
+	    }
+	  return -1;
+	}
+      return 0;
+    }
+  return -1;
+}
+
+int
+filename_from_ansi (const char *fn_in, char *fn_out)
+{
+  wchar_t fn_utf16[MAX_PATH];
+  int codepage = codepage_for_filenames (NULL);
+  int result = pMultiByteToWideChar (codepage, MB_ERR_INVALID_CHARS, fn_in, -1,
+				     fn_utf16, MAX_PATH);
+
+  if (!result)
+    {
+      DWORD err = GetLastError ();
+
+      switch (err)
+	{
+	case ERROR_INVALID_FLAGS:
+	case ERROR_INVALID_PARAMETER:
+	  errno = EINVAL;
+	  break;
+	case ERROR_INSUFFICIENT_BUFFER:
+	case ERROR_NO_UNICODE_TRANSLATION:
+	default:
+	  errno = ENOENT;
+	  break;
+	}
+      return -1;
+    }
+  return filename_from_utf16 (fn_utf16, fn_out);
+}
+
+int
+w32_valid_pointer_p (void *p, int size)
+{
+  SIZE_T done;
+  HANDLE h = OpenProcess (PROCESS_VM_READ, FALSE, GetCurrentProcessId ());
+
+  if (h)
+    {
+      unsigned char *buf = alloca (size);
+      int retval = ReadProcessMemory (h, p, buf, size, &done);
+
+      CloseHandle (h);
+      return retval;
+    }
+  else
+    return -1;
+}
+
+/* End experimental code for Cygwin-w32 build */
+
 #define DIRWATCH_SIGNATURE 0x01233210
 
 struct notification {
diff --git a/src/w32term.c b/src/w32term.c
index d415b13..061c9c3 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -3195,7 +3195,6 @@ lispy_file_action (DWORD action)
   return retval;
 }
 
-#ifdef WINDOWSNT
 /* Put file notifications into the Emacs input event queue.  This
    function runs when the WM_EMACS_FILENOTIFY message arrives from a
    watcher thread.  */
@@ -3271,7 +3270,6 @@ queue_notifications (struct input_event *event, W32Msg *msg, struct frame *f,
   /* We've stuffed all the events ourselves, so w32_read_socket shouldn't.  */
   event->kind = NO_EVENT;
 }
-#endif	/* WINDOWSNT */
 #endif	/* HAVE_W32NOTIFY */
 
 \f

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> Date: Fri, 27 Feb 2015 17:25:54 -0500
> From: Ken Brown <kbrown@cornell.edu>
> CC: bug-gnu-emacs@gnu.org, rpluim@gmail.com
> 
> It looks like it might not take much effort.  What I did for now, just 
> for testing purposes, is to copy into w32notify.c whatever was necessary 
> to make the Cygwin-w32 build compile with w32 file notification.  I 
> tested it briefly and it seems to work.  Obviously it needs much more 
> testing to make sure this doesn't break anything else, and then we can 
> worry about how to do this right.

Thanks.  A few comments:

  . Cygwin doesn't need to call MultiByteToWideChar and
    WideCharToMultiByte via function pointers, it can call them
    directly.

  . I think there shouldn't be a need to call filename_to_utf16 and
    its ilk, but instead use the to_unicode etc., like the rest of
    Cygwin-w32 code does.  E.g., just use GUI_ENCODE_FILE defined on
    w32term.h.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Ken Brown]

On 2/28/2015 2:59 AM, Eli Zaretskii wrote:
>> Date: Fri, 27 Feb 2015 17:25:54 -0500
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: bug-gnu-emacs@gnu.org, rpluim@gmail.com
>>
>> It looks like it might not take much effort.  What I did for now, just
>> for testing purposes, is to copy into w32notify.c whatever was necessary
>> to make the Cygwin-w32 build compile with w32 file notification.  I
>> tested it briefly and it seems to work.  Obviously it needs much more
>> testing to make sure this doesn't break anything else, and then we can
>> worry about how to do this right.
>
> Thanks.  A few comments:
>
>    . Cygwin doesn't need to call MultiByteToWideChar and
>      WideCharToMultiByte via function pointers, it can call them
>      directly.
>
>    . I think there shouldn't be a need to call filename_to_utf16 and
>      its ilk, but instead use the to_unicode etc., like the rest of
>      Cygwin-w32 code does.  E.g., just use GUI_ENCODE_FILE defined on
>      w32term.h.

It turns out that filename_to_utf16 wasn't even being called in the 
Cygwin-w32 build, because w32_unicode_filenames == 0.

It also turns out that when I "tested" the build, which I did via M-x 
auto-revert-tail-mode, I wasn't actually testing w32notify, because I 
forgot to do this:

diff --git a/src/emacs.c b/src/emacs.c
index ca5633d..342d0fe 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -1334,11 +1334,12 @@ Using an Emacs configured with 
--with-x-toolkit=lucid does not have this problem
    globals_of_gfilenotify ();
  #endif

-#ifdef WINDOWSNT
-  globals_of_w32 ();
  #ifdef HAVE_W32NOTIFY
    globals_of_w32notify ();
  #endif
+
+#ifdef WINDOWSNT
+  globals_of_w32 ();
    /* Initialize environment from registry settings.  */
    init_environment (argv);
    init_ntproc (dumping); /* must precede init_editfns.  */
@@ -1512,11 +1513,12 @@ Using an Emacs configured with 
--with-x-toolkit=lucid does not have this problem
        syms_of_dbusbind ();
  #endif /* HAVE_DBUS */

-#ifdef WINDOWSNT
-      syms_of_ntterm ();
  #ifdef HAVE_W32NOTIFY
        syms_of_w32notify ();
-#endif /* HAVE_W32NOTIFY */
+#endif
+
+#ifdef WINDOWSNT
+      syms_of_ntterm ();
  #endif /* WINDOWSNT */

        syms_of_profiler ();


When I add that patch, auto-revert-tail-mode no longer works; there's no 
error message, but the buffer doesn't get auto-reverted when something 
is appended.

I think I have again lost motivation to pursue this further, since gfile 
works fine.  And glib is required for the default Cygwin-w32 build 
anyway, because ImageMagick brings it in, so why not just use it?

Ken

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> Date: Sat, 28 Feb 2015 11:32:29 -0500
> From: Ken Brown <kbrown@cornell.edu>
> CC: bug-gnu-emacs@gnu.org, rpluim@gmail.com
> 
> I think I have again lost motivation to pursue this further, since gfile 
> works fine.  And glib is required for the default Cygwin-w32 build 
> anyway, because ImageMagick brings it in, so why not just use it?

Perhaps the OP could explain why he wanted that.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

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

On Sat, Feb 28, 2015 at 5:41 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Sat, 28 Feb 2015 11:32:29 -0500
> > From: Ken Brown <kbrown@cornell.edu>
> > CC: bug-gnu-emacs@gnu.org, rpluim@gmail.com
> >
> > I think I have again lost motivation to pursue this further, since gfile
> > works fine.  And glib is required for the default Cygwin-w32 build
> > anyway, because ImageMagick brings it in, so why not just use it?
>
> Perhaps the OP could explain why he wanted that.
>

[-- Attachment #2: Type: text/html, Size: 990 bytes --]

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

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

@#$ web interface, sorry about the duplicate

On Sat, Feb 28, 2015 at 5:41 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Sat, 28 Feb 2015 11:32:29 -0500
> > From: Ken Brown <kbrown@cornell.edu>
> > CC: bug-gnu-emacs@gnu.org, rpluim@gmail.com
> >
> > I think I have again lost motivation to pursue this further, since gfile
> > works fine.  And glib is required for the default Cygwin-w32 build
> > anyway, because ImageMagick brings it in, so why not just use it?
>
> Perhaps the OP could explain why he wanted that.
>

If you're referring to me, my motivation was to prevent a compilation error
for an incorrect configure combination. I believe you started us all down
the path of trying to get w32 notifying to work. I have no problem with
using glib.

Robert

[-- Attachment #2: Type: text/html, Size: 1329 bytes --]

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> Date: Sat, 28 Feb 2015 17:56:28 +0100
> From: Robert Pluim <rpluim@gmail.com>
> Cc: Ken Brown <kbrown@cornell.edu>, bug-gnu-emacs@gnu.org
> 
>     Perhaps the OP could explain why he wanted that.
> 
> If you're referring to me, my motivation was to prevent a compilation error for
> an incorrect configure combination. I believe you started us all down the path
> of trying to get w32 notifying to work. I have no problem with using glib.

No, I meant your reasons for specifying --with-file-notification=w32.
You could only bump into this issue if you specified it, AFAIU.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Ken Brown]

On 2/28/2015 12:57 PM, Eli Zaretskii wrote:
>> Date: Sat, 28 Feb 2015 17:56:28 +0100
>> From: Robert Pluim <rpluim@gmail.com>
>> Cc: Ken Brown <kbrown@cornell.edu>, bug-gnu-emacs@gnu.org
>>
>>      Perhaps the OP could explain why he wanted that.
>>
>> If you're referring to me, my motivation was to prevent a compilation error for
>> an incorrect configure combination. I believe you started us all down the path
>> of trying to get w32 notifying to work. I have no problem with using glib.
>
> No, I meant your reasons for specifying --with-file-notification=w32.
> You could only bump into this issue if you specified it, AFAIU.

Based on http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19909#36, I think 
he specified --with-file-notification=w32 because he wanted file 
notification and didn't know he could use gfile.  Robert, please correct 
me if I'm misinterpreting what you wrote.

I'm inclined to go ahead with the revised patch he submitted in the 
above URL.  I'll commit it (with Robert as author and with the change 
marked as a "tiny change") if that's OK with you, Eli.

Two questions:

1. Shouldn't the name in the ChangeLog be "Robert Pluim" instead of 
"rpluim" to match all the other entries?

2. I tested making a git commit with

   --author="Robert Pluim <rpluim@gmail.com>  (tiny change)"

but git stripped away the "(tiny change)" part.  What should I do to get 
"tiny change" into the git history?  Maybe it's not so important now, 
because I can put it into the ChangeLog, but I'm thinking about the 
future, when ChangeLogs are automatically generated.

Ken

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Eli Zaretskii]

> Date: Sat, 28 Feb 2015 13:44:38 -0500
> From: Ken Brown <kbrown@cornell.edu>
> CC: bug-gnu-emacs@gnu.org
> 
> > No, I meant your reasons for specifying --with-file-notification=w32.
> > You could only bump into this issue if you specified it, AFAIU.
> 
> Based on http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19909#36, I think 
> he specified --with-file-notification=w32 because he wanted file 
> notification and didn't know he could use gfile.

Isn't gfile the default for Cygwin?

> I'm inclined to go ahead with the revised patch he submitted in the 
> above URL.  I'll commit it (with Robert as author and with the change 
> marked as a "tiny change") if that's OK with you, Eli.

If that's what Cygwin users want, fine with me.

> 1. Shouldn't the name in the ChangeLog be "Robert Pluim" instead of 
> "rpluim" to match all the other entries?

I don't think it matters when there's no assignment on file.

> 2. I tested making a git commit with
> 
>    --author="Robert Pluim <rpluim@gmail.com>  (tiny change)"
> 
> but git stripped away the "(tiny change)" part.  What should I do to get 
> "tiny change" into the git history?  Maybe it's not so important now, 
> because I can put it into the ChangeLog, but I'm thinking about the 
> future, when ChangeLogs are automatically generated.

The projects I know of use

  Copyright-paperwork-exempt: yes

on a separate line in the log message.  You will see that I already
used that in Emacs, at least once.

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Ken Brown]

On 2/28/2015 2:14 PM, Eli Zaretskii wrote:
>> Date: Sat, 28 Feb 2015 13:44:38 -0500
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: bug-gnu-emacs@gnu.org
>>
>>> No, I meant your reasons for specifying --with-file-notification=w32.
>>> You could only bump into this issue if you specified it, AFAIU.
>>
>> Based on http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19909#36, I think
>> he specified --with-file-notification=w32 because he wanted file
>> notification and didn't know he could use gfile.
>
> Isn't gfile the default for Cygwin?

Yes.  (Of course, the builder has to have the appropriate glib 
development package installed, or else file notification won't actually 
be used.)

>> I'm inclined to go ahead with the revised patch he submitted in the
>> above URL.  I'll commit it (with Robert as author and with the change
>> marked as a "tiny change") if that's OK with you, Eli.
>
> If that's what Cygwin users want, fine with me.

I personally don't care, but Robert requested it, and I don't see any 
harm in it.  So I'll go ahead as soon as I hear back from him as to how 
he wants his name shown.

>> 1. Shouldn't the name in the ChangeLog be "Robert Pluim" instead of
>> "rpluim" to match all the other entries?
>
> I don't think it matters when there's no assignment on file.
>
>> 2. I tested making a git commit with
>>
>>     --author="Robert Pluim <rpluim@gmail.com>  (tiny change)"
>>
>> but git stripped away the "(tiny change)" part.  What should I do to get
>> "tiny change" into the git history?  Maybe it's not so important now,
>> because I can put it into the ChangeLog, but I'm thinking about the
>> future, when ChangeLogs are automatically generated.
>
> The projects I know of use
>
>    Copyright-paperwork-exempt: yes
>
> on a separate line in the log message.  You will see that I already
> used that in Emacs, at least once.

Thanks.

Ken

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Robert Pluim]

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

On Sat, Feb 28, 2015 at 9:14 PM, Ken Brown <kbrown@cornell.edu> wrote:

> On 2/28/2015 2:14 PM, Eli Zaretskii wrote:
>
>> Date: Sat, 28 Feb 2015 13:44:38 -0500
>>> From: Ken Brown <kbrown@cornell.edu>
>>> CC: bug-gnu-emacs@gnu.org
>>>
>>>  No, I meant your reasons for specifying --with-file-notification=w32.
>>>> You could only bump into this issue if you specified it, AFAIU.
>>>>
>>>
>>> Based on http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19909#36, I think
>>> he specified --with-file-notification=w32 because he wanted file
>>> notification and didn't know he could use gfile.
>>>
>>
>>
Exactly


> Isn't gfile the default for Cygwin?
>>
>
> Yes.  (Of course, the builder has to have the appropriate glib development
> package installed, or else file notification won't actually be used.)
>
>  I'm inclined to go ahead with the revised patch he submitted in the
>>> above URL.  I'll commit it (with Robert as author and with the change
>>> marked as a "tiny change") if that's OK with you, Eli.
>>>
>>
>> If that's what Cygwin users want, fine with me.
>>
>
> I personally don't care, but Robert requested it, and I don't see any harm
> in it.  So I'll go ahead as soon as I hear back from him as to how he wants
> his name shown.
>
>  1. Shouldn't the name in the ChangeLog be "Robert Pluim" instead of
>>> "rpluim" to match all the other entries?
>>>
>>
>> I don't think it matters when there's no assignment on file.
>>
>
I'd prefer "Robert Pluim" but it's not a big deal. It does show that
there's a variable somewhere that I haven't set correctly though, since
that ChangeLog entry should never have said "rpluim"


>
>>  2. I tested making a git commit with
>>>
>>>     --author="Robert Pluim <rpluim@gmail.com>  (tiny change)"
>>>
>>> but git stripped away the "(tiny change)" part.  What should I do to get
>>> "tiny change" into the git history?  Maybe it's not so important now,
>>> because I can put it into the ChangeLog, but I'm thinking about the
>>> future, when ChangeLogs are automatically generated.
>>>
>>
>> The projects I know of use
>>
>>    Copyright-paperwork-exempt: yes
>>
>> on a separate line in the log message.  You will see that I already
>> used that in Emacs, at least once.
>>
>
I'll test the waters with my future employers as to copyright assignment,
and this discussion will hopefully not have to happen again :)

Regards

Robert

[-- Attachment #2: Type: text/html, Size: 4898 bytes --]

bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

[Ken Brown]

On 3/2/2015 4:35 AM, Robert Pluim wrote:
> I'd prefer "Robert Pluim" but it's not a big deal.

OK, I've pushed the change to the master branch and am closing the bug.

> It does show that
> there's a variable somewhere that I haven't set correctly though, since
> that ChangeLog entry should never have said "rpluim"

I think user-full-name is what you're looking for.

Ken

bug#19909: closed (Re: bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin)

[GNU bug Tracking System]

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

Your bug report

#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to 19909@debbugs.gnu.org.

-- 
19909: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19909
GNU Bug Tracking System
Contact help-debbugs@gnu.org with problems

[-- Attachment #2: Type: message/rfc822, Size: 3500 bytes --]

From: Ken Brown <kbrown@cornell.edu>
To: Robert Pluim <rpluim@gmail.com>
Cc: Eli Zaretskii <eliz@gnu.org>, 19909-done@debbugs.gnu.org
Subject: Re: bug#19909: [PATCH] Error out if with-file-notification=w32 is specified on cygwin
Date: Mon, 02 Mar 2015 09:05:40 -0500
Message-ID: <54F46E34.1090502@cornell.edu>

On 3/2/2015 4:35 AM, Robert Pluim wrote:
> I'd prefer "Robert Pluim" but it's not a big deal.

OK, I've pushed the change to the master branch and am closing the bug.

> It does show that
> there's a variable somewhere that I haven't set correctly though, since
> that ChangeLog entry should never have said "rpluim"

I think user-full-name is what you're looking for.

Ken



[-- Attachment #3: Type: message/rfc822, Size: 5130 bytes --]

From: Robert Pluim <rpluim@gmail.com>
To: bug-gnu-emacs@gnu.org
Subject: [PATCH] Error out if with-file-notification=w32 is specified on cygwin
Date: Fri, 20 Feb 2015 10:47:36 +0100
Message-ID: <821tlkhq2v.fsf@gmail.com>

It's a combination that won't compile, so it's probably best to error
out during configure.

diff --git a/ChangeLog b/ChangeLog
index 4365668..dec9ce0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-20  rpluim  <rpluim@gmail.com>
+
+	* configure.ac: Error out if with-file-notification=w32 is
+	specified on cygwin.
+
 2015-02-13  Jan Djärv  <jan.h.d@swipnet.se>
 
 	* configure.ac: Set locallisppath to empty for NS self contained,
diff --git a/configure.ac b/configure.ac
index 0bcc55c..048168a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2659,6 +2659,10 @@ fi
 
 dnl MS Windows native file monitor is available for mingw32 only.
 case $with_file_notification,$opsys in
+  w32,cygwin)
+    AC_MSG_ERROR([`--with-file-notification=w32' was specified, but
+    this is only supported on MS-Windows native and MinGW32 builds])
+    ;;
   w32,* | yes,mingw32)
     AC_CHECK_HEADER(windows.h)
     if test "$ac_cv_header_windows_h" = yes ; then