Cygwin patches

Cygwin patches

[Ken Brown]

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

Now that you're preparing for emacs 23.2, could you please apply the 
cygwin patches that I submitted last June?

   http://lists.gnu.org/archive/html/emacs-devel/2009-06/msg00041.html
   http://lists.gnu.org/archive/html/emacs-devel/2009-06/msg00330.html

For your convenience, I've remade the patches against the current CVS 
source and attached them to this message.  I don't want to repeat the 
rationale for the patches that was already made in the threads cited 
above, but let me just add two comments:

1. The patches to browse-url.el and cygwin.h fix cygwin problems and 
should have no impact on other systems.

2. I built the official cygwin emacs-23.1 package with these patches, 
and there have been no reported problems after several months of use.

Thanks for your consideration.

Ken

[-- Attachment #2: browse-url.el.patch --]
[-- Type: text/plain, Size: 1389 bytes --]

--- origsrc/lisp/net/browse-url.el	2009-11-20 11:34:25.000000000 -0500
+++ src/lisp/net/browse-url.el	2009-11-20 11:35:28.000000000 -0500
@@ -693,7 +693,9 @@
 	  (cond ((not (buffer-modified-p)))
 		(browse-url-save-file (save-buffer))
 		(t (message "%s modified since last save" file))))))
-  (browse-url (browse-url-file-url file))
+  (if (eq system-type 'cygwin)
+      (shell-command (concat "cygstart " (shell-quote-argument file)))
+    (browse-url (browse-url-file-url file)))
   (run-hooks 'browse-url-of-file-hook))
 
 (defun browse-url-file-url (file)
@@ -830,11 +832,13 @@
 
 (defun browse-url-default-windows-browser (url &optional new-window)
   (interactive (browse-url-interactive-arg "URL: "))
-  (if (eq system-type 'ms-dos)
-      (if dos-windows-version
-	  (shell-command (concat "start " (shell-quote-argument url)))
-	(error "Browsing URLs is not supported on this system"))
-    (w32-shell-execute "open" url)))
+  (cond ((eq system-type 'ms-dos)
+	 (if dos-windows-version
+	     (shell-command (concat "start " (shell-quote-argument url)))
+	   (error "Browsing URLs is not supported on this system")))
+	((eq system-type 'cygwin)
+	 (shell-command (concat "cygstart " (shell-quote-argument url))))
+	(t (w32-shell-execute "open" url))))
 
 (defun browse-url-default-macosx-browser (url &optional new-window)
   (interactive (browse-url-interactive-arg "URL: "))

[-- Attachment #3: PROBLEMS.patch --]
[-- Type: text/plain, Size: 2993 bytes --]

--- origsrc/etc/PROBLEMS	2009-11-20 11:34:12.000000000 -0500
+++ src/etc/PROBLEMS	2009-11-20 11:34:46.000000000 -0500
@@ -255,6 +255,18 @@
 Cygwin malloc, the Cygwin memalign always returns ENOSYS.  A fix for this
 problem would be welcome.
 
+A workaround is to set G_SLICE=always-malloc before starting emacs.
+For example, in bash,
+
+  G_SLICE=always-malloc emacs
+
+or put
+
+  export G_SLICE=always-malloc
+
+in one of the bash startup files.  This  also has to be done before
+building emacs on Cygwin with Gtk+.
+
 * General runtime problems
 
 ** Lisp problems
@@ -2207,34 +2219,6 @@
 changes the keyboard layout depends on your Windows version; for XP,
 in the Languages tab, click "Details" and then "Key Settings".)
 
-** Cygwin build of Emacs hangs after rebasing Cygwin DLLs
-
-Usually, on Cygwin, one needs to rebase the DLLs if an application
-aborts with a message like this:
-
-  C:\cygwin\bin\python.exe: *** unable to remap C:\cygwin\bin\cygssl.dll to
-  same address as parent(0xDF0000) != 0xE00000
-
-However, since Cygwin DLL 1.5.17 was released, after such rebasing,
-Emacs hangs.
-
-This was reported to happen for Emacs 21.2 and also for the pretest of
-Emacs 22.1 on Cygwin.
-
-To work around this, build Emacs like this:
-
-  LDFLAGS='-Wl,--enable-auto-import -Wl,--enable-auto-image-base' ./configure
-  make LD='$(CC)'
-  make LD='$(CC)' install
-
-This produces an Emacs binary that is independent of rebasing.
-
-Note that you _must_ use LD='$(CC)' in the last two commands above, to
-prevent GCC from passing the "--image-base 0x20000000" option to the
-linker, which is what it does by default.  That option produces an
-Emacs binary with the base address 0x20000000, which will cause Emacs
-to hang after Cygwin DLLs are rebased.
-
 ** Interrupting Cygwin port of Bash from Emacs doesn't work.
 
 Cygwin 1.x builds of the ported Bash cannot be interrupted from the
@@ -2481,17 +2465,12 @@
 
 (using the location of the 32-bit X libraries on your system).
 
-*** Building the Cygwin port for MS-Windows can fail with some GCC versions
+*** Building the Cygwin port of emacs requires GCC 4
 
-Building Emacs 22 with Cygwin builds of GCC 3.4.4-1 and 3.4.4-2 is
-reported to either fail or cause Emacs to segfault at run time.  In
-addition, the Cygwin GCC 3.4.4-2 has problems with generating debug
-info.  Cygwin users are advised not to use these versions of GCC for
-compiling Emacs.  GCC versions 4.0.3, 4.0.4, 4.1.1, and 4.1.2
-reportedly build a working Cygwin binary of Emacs, so we recommend
-these GCC versions.  Note that these versions of GCC, 4.0.3, 4.0.4,
-4.1.1, and 4.1.2, are currently the _only_ versions known to succeed
-in building Emacs (as of v22.1).
+As of Emacs 22.1, there have been problems with Cygwin builds of Emacs
+using GCC 3.  Cygwin users are advised to use GCC 4.  As far as we
+know, there have been no problems with Cygwin builds of Emacs using
+GCC 4.
 
 *** Building the native MS-Windows port with Cygwin GCC can fail.
 

[-- Attachment #4: cygwin.h.patch --]
[-- Type: text/plain, Size: 606 bytes --]

--- origsrc/src/s/cygwin.h	2009-06-21 00:38:20.000000000 -0400
+++ src/src/s/cygwin.h	2009-11-20 11:41:27.281250000 -0500
@@ -105,11 +105,7 @@
 #define SYSV_SYSTEM_DIR 1
 #define UNEXEC unexcw.o
 #define POSIX_SIGNALS 1
-/* force the emacs image to start high in memory, so dll relocation
-   can put things in low memory without causing all sorts of grief for
-   emacs lisp pointers */
-#define DATA_SEG_BITS 0x20000000
-#define LINKER $(CC) -Wl,--image-base,DATA_SEG_BITS
+#define LINKER $(CC)
 
 /* Use terminfo instead of termcap.  Fewer environment variables to
    go wrong, more terminal types. */

Re: Cygwin patches

[Chong Yidong]

Ken Brown <kbrown@cornell.edu> writes:

> Now that you're preparing for emacs 23.2, could you please apply the
> cygwin patches that I submitted last June?

Thanks, I've checked in your changes, except for the following:

> --- origsrc/lisp/net/browse-url.el	2009-11-20 11:34:25.000000000 -0500
> +++ src/lisp/net/browse-url.el	2009-11-20 11:35:28.000000000 -0500
> @@ -693,7 +693,9 @@
>  	  (cond ((not (buffer-modified-p)))
>  		(browse-url-save-file (save-buffer))
>  		(t (message "%s modified since last save" file))))))
> -  (browse-url (browse-url-file-url file))
> +  (if (eq system-type 'cygwin)
> +      (shell-command (concat "cygstart " (shell-quote-argument file)))
> +    (browse-url (browse-url-file-url file)))
>    (run-hooks 'browse-url-of-file-hook))

I still don't understand why cygwin needs special handling when, e.g.,
the other Windows ports don't.  Could you explain?

Re: Cygwin patches

[Lennart Borgman]

On Fri, Nov 20, 2009 at 10:50 PM, Chong Yidong <cyd@stupidchicken.com> wrote:
> Ken Brown <kbrown@cornell.edu> writes:
>
>> Now that you're preparing for emacs 23.2, could you please apply the
>> cygwin patches that I submitted last June?
>
> Thanks, I've checked in your changes, except for the following:
>
>> --- origsrc/lisp/net/browse-url.el    2009-11-20 11:34:25.000000000 -0500
>> +++ src/lisp/net/browse-url.el        2009-11-20 11:35:28.000000000 -0500
>> @@ -693,7 +693,9 @@
>>         (cond ((not (buffer-modified-p)))
>>               (browse-url-save-file (save-buffer))
>>               (t (message "%s modified since last save" file))))))
>> -  (browse-url (browse-url-file-url file))
>> +  (if (eq system-type 'cygwin)
>> +      (shell-command (concat "cygstart " (shell-quote-argument file)))
>> +    (browse-url (browse-url-file-url file)))
>>    (run-hooks 'browse-url-of-file-hook))
>
> I still don't understand why cygwin needs special handling when, e.g.,
> the other Windows ports don't.  Could you explain?


Move the special part into browse-url. It calls the function in
browse-url-browser-function. That is where the OS specific things
should go.

Re: Cygwin patches

[Ken Brown]

On 11/20/2009 4:50 PM, Chong Yidong wrote:
> Ken Brown <kbrown@cornell.edu> writes:
> 
>> Now that you're preparing for emacs 23.2, could you please apply the
>> cygwin patches that I submitted last June?
> 
> Thanks, I've checked in your changes, except for the following:
> 
>> --- origsrc/lisp/net/browse-url.el	2009-11-20 11:34:25.000000000 -0500
>> +++ src/lisp/net/browse-url.el	2009-11-20 11:35:28.000000000 -0500
>> @@ -693,7 +693,9 @@
>>  	  (cond ((not (buffer-modified-p)))
>>  		(browse-url-save-file (save-buffer))
>>  		(t (message "%s modified since last save" file))))))
>> -  (browse-url (browse-url-file-url file))
>> +  (if (eq system-type 'cygwin)
>> +      (shell-command (concat "cygstart " (shell-quote-argument file)))
>> +    (browse-url (browse-url-file-url file)))
>>    (run-hooks 'browse-url-of-file-hook))
> 
> I still don't understand why cygwin needs special handling when, e.g.,
> the other Windows ports don't.  Could you explain?

Cygwin provides a linux-like environment for Windows, but it doesn't 
provide its own web browser.  So it's natural for a cygwin user to just 
want to use the default Windows browser.  And cygwin provides the 
"cygstart" command precisely to make this sort of thing easy.  Thus

   cygstart /unix/style/path/to/file.html

will open file.html in the default Windows browser.  Cygstart takes care 
of converting the path to a form that Windows understands.  Without my 
patch, (browse-url-file-url file) returns a URL that doesn't get 
correctly converted.

Ken

Without my patch,

Re: Cygwin patches

[Angelo Graziosi]

Ken Brown wrote:
> could you please apply the cygwin patches

I have applied those patches for a long time without problems, but now, 
as I suspected, they have been applied in wrong manner. Indeed now 
building Emacs on Cygwin fails:

[...]
gcc -Wl,--image-base,DATA_SEG_BITS   -Wl,--enable-auto-import 
-Wl,--enable-runtime-pseudo-reloc   -o temacs ecrt0.o dispnew.o frame.o 
scroll.o xdisp.o menu.o xmenu.o window.o charset.o coding.o category.o 
ccl.o character.o chartab.o cm.o term.o terminal.o xfaces.o xterm.o 
xfns.o xselect.o xrdb.o fontset.o xsmfns.o fringe.o image.o xsettings.o 
gtkutil.o dbusbind.o emacs.o keyboard.o macros.o keymap.o sysdep.o 
buffer.o filelock.o insdel.o marker.o minibuf.o fileio.o dired.o 
filemode.o cmds.o casetab.o casefiddle.o indent.o search.o regex.o 
undo.o alloc.o data.o doc.o editfns.o callint.o eval.o floatfns.o fns.o 
font.o print.o lread.o syntax.o unexcw.o bytecode.o process.o callproc.o 
region-cache.o sound.o atimer.o doprnt.o strftime.o intervals.o 
textprop.o composite.o md5.o   sheap.o xfont.o ftfont.o xftfont.o 
ftxfont.o terminfo.o gmalloc.o ralloc.o lastfile.o vm-limit.o 
getloadavg.o     -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lgdk_pixbuf-2.0 
-lpangocairo-1.0 -lgio-2.0 -lXinerama -lXi -lXrandr -lXcursor 
-lXcomposite -lXdamage -lpangoft2-1.0 -lXext -lXfixes -lcairo -lpixman-1 
-lglitz -lpng12 -lxcb-render-util -lXrender -lxcb-render -lX11 -lxcb 
-lXau -lXdmcp -lpango-1.0 -lm -lfontconfig -lexpat -lfreetype -lz 
-lgobject-2.0 -lgmodule-2.0 -lglib-2.0 -lintl -liconv -lpthread -lSM 
-lICE -ltiff -ljpeg -lpng -lz -lm -lgif -lXpm -lX11 -lXft -lXrender 
-lfontconfig -lexpat -liconv -lfreetype -lz -lX11 -lxcb -lXau -lXdmcp 
-ldbus-1 -lpthread -lcurses -lg  -lgconf-2 -lORBit-2 -lm -ldbus-1 
-lpthread -lgmodule-2.0 -lgthread-2.0 -lgobject-2.0 -lglib-2.0 -lintl 
-liconv `gcc -print-libgcc-file-name` -lm -lc `gcc -print-libgcc-file-name`
/usr/lib/gcc/i686-pc-cygwin/4.3.4/../../../../i686-pc-cygwin/bin/ld: 
invalid hex number for PE parameter 'DATA_SEG_BITS'
collect2: ld returned 1 exit status
make[2]: *** [temacs.exe] Error 1
make[2]: Leaving directory `/tmp/emacs/build/src'
make[1]: *** [src] Error 2
make[1]: Leaving directory `/tmp/emacs/build'
make: *** [bootstrap] Error 2

The applied patches for browse-url, PROBLEMS and cygwin.h are different 
from those that Ken have proposed and that work fine since many months.


Ciao,
Angelo.

Re: Cygwin patches

[Lennart Borgman]

On Fri, Nov 20, 2009 at 11:26 PM, Ken Brown <kbrown@cornell.edu> wrote:
> On 11/20/2009 4:50 PM, Chong Yidong wrote:
>>
>> Ken Brown <kbrown@cornell.edu> writes:
>>
>>> Now that you're preparing for emacs 23.2, could you please apply the
>>> cygwin patches that I submitted last June?
>>
>> Thanks, I've checked in your changes, except for the following:
>>
>>> --- origsrc/lisp/net/browse-url.el      2009-11-20 11:34:25.000000000
>>> -0500
>>> +++ src/lisp/net/browse-url.el  2009-11-20 11:35:28.000000000 -0500
>>> @@ -693,7 +693,9 @@
>>>          (cond ((not (buffer-modified-p)))
>>>                (browse-url-save-file (save-buffer))
>>>                (t (message "%s modified since last save" file))))))
>>> -  (browse-url (browse-url-file-url file))
>>> +  (if (eq system-type 'cygwin)
>>> +      (shell-command (concat "cygstart " (shell-quote-argument file)))
>>> +    (browse-url (browse-url-file-url file)))
>>>   (run-hooks 'browse-url-of-file-hook))
>>
>> I still don't understand why cygwin needs special handling when, e.g.,
>> the other Windows ports don't.  Could you explain?
>
> Cygwin provides a linux-like environment for Windows, but it doesn't provide
> its own web browser.  So it's natural for a cygwin user to just want to use
> the default Windows browser.  And cygwin provides the "cygstart" command
> precisely to make this sort of thing easy.  Thus
>
>  cygstart /unix/style/path/to/file.html
>
> will open file.html in the default Windows browser.  Cygstart takes care of
> converting the path to a form that Windows understands.  Without my patch,
> (browse-url-file-url file) returns a URL that doesn't get correctly
> converted.

That is ok, but the patch puts this enhancement in the wrong place.
Please look at how this is organized (see my prev post).

Re: Cygwin patches

[Chong Yidong]

Angelo Graziosi <angelo.graziosi@alice.it> writes:

> The applied patches for browse-url, PROBLEMS and cygwin.h are
> different from those that Ken have proposed and that work fine since
> many months.

Should be fixed now, thanks for noticing.

Re: Cygwin patches

[Eli Zaretskii]

> Date: Fri, 20 Nov 2009 17:26:12 -0500
> From: Ken Brown <kbrown@cornell.edu>
> Cc: Emacs <emacs-devel@gnu.org>
> 
> Cygwin provides a linux-like environment for Windows, but it doesn't 
> provide its own web browser.  So it's natural for a cygwin user to just 
> want to use the default Windows browser.  And cygwin provides the 
> "cygstart" command precisely to make this sort of thing easy.  Thus
> 
>    cygstart /unix/style/path/to/file.html
> 
> will open file.html in the default Windows browser.  Cygstart takes care 
> of converting the path to a form that Windows understands.  Without my 
> patch, (browse-url-file-url file) returns a URL that doesn't get 
> correctly converted.

I think that the right fix would be in browse-url-file-url, so that it
does return a correctly converted URL.  Using cygstart directly in
browse-url-of-file deviates too much from what other platforms do --
they all invoke the browser in browse-url.  Such a deviation could
mean maintenance headaches in the future.  For example, browse-url
takes care of setting the environment for the process being invoked,
while your patch short-circuits that for Cygwin.

Re: Cygwin patches

[Ken Brown]

On 11/21/2009 3:12 AM, Eli Zaretskii wrote:
>> Date: Fri, 20 Nov 2009 17:26:12 -0500
>> From: Ken Brown <kbrown@cornell.edu>
>> Cc: Emacs <emacs-devel@gnu.org>
>>
>> Cygwin provides a linux-like environment for Windows, but it doesn't 
>> provide its own web browser.  So it's natural for a cygwin user to just 
>> want to use the default Windows browser.  And cygwin provides the 
>> "cygstart" command precisely to make this sort of thing easy.  Thus
>>
>>    cygstart /unix/style/path/to/file.html
>>
>> will open file.html in the default Windows browser.  Cygstart takes care 
>> of converting the path to a form that Windows understands.  Without my 
>> patch, (browse-url-file-url file) returns a URL that doesn't get 
>> correctly converted.
> 
> I think that the right fix would be in browse-url-file-url, so that it
> does return a correctly converted URL.  Using cygstart directly in
> browse-url-of-file deviates too much from what other platforms do --
> they all invoke the browser in browse-url.  Such a deviation could
> mean maintenance headaches in the future.  For example, browse-url
> takes care of setting the environment for the process being invoked,
> while your patch short-circuits that for Cygwin.

OK, that makes sense.  I'll figure out how to patch browse-url-file-url 
instead.  But it will still mean cygwin-specific code in that function. 
  Is that acceptable?

[BTW, Lennart's suggestion that the cygwin-specific code should be moved 
to browse-url doesn't work.  In fact, browse-url works fine on cygwin as 
is; it just needs to be given a good URL.]

Ken

Re: Cygwin patches

[Lennart Borgman]

On Sat, Nov 21, 2009 at 1:12 PM, Ken Brown <kbrown@cornell.edu> wrote:
>>
>> I think that the right fix would be in browse-url-file-url, so that it
>> does return a correctly converted URL.  Using cygstart directly in
>> browse-url-of-file deviates too much from what other platforms do --
>> they all invoke the browser in browse-url.  Such a deviation could
>> mean maintenance headaches in the future.  For example, browse-url
>> takes care of setting the environment for the process being invoked,
>> while your patch short-circuits that for Cygwin.
>
> OK, that makes sense.  I'll figure out how to patch browse-url-file-url
> instead.  But it will still mean cygwin-specific code in that function.  Is
> that acceptable?
>
> [BTW, Lennart's suggestion that the cygwin-specific code should be moved to
> browse-url doesn't work.  In fact, browse-url works fine on cygwin as is; it
> just needs to be given a good URL.]


Sorry for the confusion. I meant the same as Eli here.

Re: Cygwin patches

[Eli Zaretskii]

> Date: Sat, 21 Nov 2009 07:12:04 -0500
> From: Ken Brown <kbrown@cornell.edu>
> CC: cyd@stupidchicken.com, emacs-devel@gnu.org
> 
> > I think that the right fix would be in browse-url-file-url, so that it
> > does return a correctly converted URL.  Using cygstart directly in
> > browse-url-of-file deviates too much from what other platforms do --
> > they all invoke the browser in browse-url.  Such a deviation could
> > mean maintenance headaches in the future.  For example, browse-url
> > takes care of setting the environment for the process being invoked,
> > while your patch short-circuits that for Cygwin.
> 
> OK, that makes sense.  I'll figure out how to patch browse-url-file-url 
> instead.

Thanks.

> But it will still mean cygwin-specific code in that function. 
> Is that acceptable?

Yes, of course.

> [BTW, Lennart's suggestion that the cygwin-specific code should be moved 
> to browse-url doesn't work.  In fact, browse-url works fine on cygwin as 
> is; it just needs to be given a good URL.]

Right, I figured that much.

Re: Cygwin patches

[Ken Brown]

On 11/21/2009 7:42 AM, Eli Zaretskii wrote:
>>> I think that the right fix would be in browse-url-file-url, so that it
>>> does return a correctly converted URL.  Using cygstart directly in
>>> browse-url-of-file deviates too much from what other platforms do --
>>> they all invoke the browser in browse-url.  Such a deviation could
>>> mean maintenance headaches in the future.  For example, browse-url
>>> takes care of setting the environment for the process being invoked,
>>> while your patch short-circuits that for Cygwin.
>> OK, that makes sense.  I'll figure out how to patch browse-url-file-url 
>> instead.

I have very little experience with emacs-lisp, so I'll need some help 
getting this right.  Here's my first attempt:

--- browse-url.el.orig	2009-11-21 12:33:40.000000000 -0500
+++ browse-url.el	2009-11-21 15:51:40.000000000 -0500
@@ -699,6 +699,10 @@
  (defun browse-url-file-url (file)
    "Return the URL corresponding to FILE.
  Use variable `browse-url-filename-alist' to map filenames to URLs."
+  (if (eq system-type 'cygwin)
+      (let ((winfile (shell-command-to-string (concat
+			  "cygpath -m " (shell-quote-argument file)))))
+	(setq file (concat "file://" (substring winfile 0 -1)))))
    (let ((coding (and (default-value 'enable-multibyte-characters)
  		     (or file-name-coding-system
  			 default-file-name-coding-system))))

Explanation: The cygpath command converts a unix-style file name (like 
/home/kbrown/index.html) to a Windows-style file name (like 
C:/cygwin/home/kbrown/index.html).  I have to add "file://" in front for 
reasons I don't understand.  (If I don't do this, "file:" gets added by 
the later code, with no slashes.)  And I delete the last character to 
get rid of a final newline.

When I try to test this in emacs 23.1, I get an error

   if: Wrong number of arguments: called-interactively-p, 1

The following additional patch, restoring something from the emacs 23.1 
version of browse-url, fixes it:

@@ -770,7 +774,7 @@
  Prompts for a URL, defaulting to the URL at or before point.  Variable
  `browse-url-browser-function' says which browser to use."
    (interactive (browse-url-interactive-arg "URL: "))
-  (unless (called-interactively-p 'interactive)
+  (unless (interactive-p)
      (setq args (or args (list browse-url-new-window-flag))))
    (let ((process-environment (copy-sequence process-environment)))
      ;; When connected to various displays, be careful to use the 
display of

I don't know what's going on.  Can you help?

Thanks.

Ken

Re: Cygwin patches

[Eli Zaretskii]

> Date: Sat, 21 Nov 2009 16:21:54 -0500
> From: Ken Brown <kbrown@cornell.edu>
> CC: cyd@stupidchicken.com, emacs-devel@gnu.org
> 
> When I try to test this in emacs 23.1, I get an error
> 
>    if: Wrong number of arguments: called-interactively-p, 1
> 
> The following additional patch, restoring something from the emacs 23.1 
> version of browse-url, fixes it:
> 
> @@ -770,7 +774,7 @@
>   Prompts for a URL, defaulting to the URL at or before point.  Variable
>   `browse-url-browser-function' says which browser to use."
>     (interactive (browse-url-interactive-arg "URL: "))
> -  (unless (called-interactively-p 'interactive)
> +  (unless (interactive-p)

Didn't we have a change in called-interactively-p on the trunk wrt
Emacs 23.1?

Re: Cygwin patches

[Davis Herring]

> +  (if (eq system-type 'cygwin)
> +      (let ((winfile (shell-command-to-string (concat
> +			  "cygpath -m " (shell-quote-argument file)))))
> +	(setq file (concat "file://" (substring winfile 0 -1)))))

I would instead bind winfile to

(with-output-to-string
  (call-process "cygpath" nil standard-output nil "-m" file))

(using, like `shell-command-to-string' does, the undocumented feature that
`with-output-to-string' causes `standard-output' to be a temporary
buffer).  Then you don't have to worry with `shell-quote-argument'.

> When I try to test this in emacs 23.1, I get an error
>
>    if: Wrong number of arguments: called-interactively-p, 1
> [...]
> I don't know what's going on.  Can you help?

That version of browse-url.el is incompatible with (newer than) 23.1.

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: Cygwin patches

[Ken Brown]

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

On 11/22/2009 12:25 AM, Davis Herring wrote:
>> +  (if (eq system-type 'cygwin)
>> +      (let ((winfile (shell-command-to-string (concat
>> +			  "cygpath -m " (shell-quote-argument file)))))
>> +	(setq file (concat "file://" (substring winfile 0 -1)))))
> 
> I would instead bind winfile to
> 
> (with-output-to-string
>   (call-process "cygpath" nil standard-output nil "-m" file))
> 
> (using, like `shell-command-to-string' does, the undocumented feature that
> `with-output-to-string' causes `standard-output' to be a temporary
> buffer).  Then you don't have to worry with `shell-quote-argument'.

Thanks for the suggestion.  My revised patch is attached.  If it looks 
OK, could someone please check it in?

Thanks.

Ken

[-- Attachment #2: browse-url.el.patch --]
[-- Type: text/plain, Size: 638 bytes --]

--- browse-url.el.orig	2009-11-21 12:33:40.000000000 -0500
+++ browse-url.el	2009-11-22 07:38:25.890761600 -0500
@@ -699,6 +699,10 @@
 (defun browse-url-file-url (file)
   "Return the URL corresponding to FILE.
 Use variable `browse-url-filename-alist' to map filenames to URLs."
+  (if (eq system-type 'cygwin)
+      (let ((winfile (with-output-to-string
+              (call-process "cygpath" nil standard-output nil "-m" file))))
+	(setq file (concat "file://" (substring winfile 0 -1)))))
   (let ((coding (and (default-value 'enable-multibyte-characters)
 		     (or file-name-coding-system
 			 default-file-name-coding-system))))

Re: Cygwin patches

[Chong Yidong]

Ken Brown <kbrown@cornell.edu> writes:

> Thanks for the suggestion.  My revised patch is attached.  If it looks
> OK, could someone please check it in?

Your patch looks incorrect.  It's not your job to add the "file://"
part; that's done in the rest of the function.  Does this corrected
patch do the right thing on cygwin?

--- browse-url.el.orig	2009-11-21 12:33:40.000000000 -0500
+++ browse-url.el	2009-11-22 07:38:25.890761600 -0500
@@ -699,6 +699,10 @@
 (defun browse-url-file-url (file)
   "Return the URL corresponding to FILE.
 Use variable `browse-url-filename-alist' to map filenames to URLs."
+  (if (eq system-type 'cygwin)
+      (let ((winfile (with-output-to-string
+              (call-process "cygpath" nil standard-output nil "-m" file))))
+	(setq file (substring winfile 0 -1))))
   (let ((coding (and (default-value 'enable-multibyte-characters)
 		     (or file-name-coding-system
 			 default-file-name-coding-system))))

Re: Cygwin patches

[Ken Brown]

On 11/22/2009 6:03 PM, Chong Yidong wrote:
> Ken Brown <kbrown@cornell.edu> writes:
> 
>> Thanks for the suggestion.  My revised patch is attached.  If it looks
>> OK, could someone please check it in?
> 
> Your patch looks incorrect.  It's not your job to add the "file://"
> part; that's done in the rest of the function.  Does this corrected
> patch do the right thing on cygwin?

Yes, it does do the right thing on cygwin.  As I said earlier in the 
thread, the rest of the function *doesn't* add "file://"; it adds 
"file:" without the slashes.  But if I add "file://", the rest of the 
function leaves this alone.  I guess browse-url-filename-alist would 
have to be changed to make it correctly add "file://" in this situation, 
but I haven't tried to dig into it to figure out how it works.  It 
seemed easier (and harmless) to just add it myself.

Ken

Re: Cygwin patches

[Lennart Borgman]

On Mon, Nov 23, 2009 at 12:23 AM, Ken Brown <kbrown@cornell.edu> wrote:
> On 11/22/2009 6:03 PM, Chong Yidong wrote:
>>
>> Ken Brown <kbrown@cornell.edu> writes:
>>
>>> Thanks for the suggestion.  My revised patch is attached.  If it looks
>>> OK, could someone please check it in?
>>
>> Your patch looks incorrect.  It's not your job to add the "file://"
>> part; that's done in the rest of the function.  Does this corrected
>> patch do the right thing on cygwin?
>
> Yes, it does do the right thing on cygwin.  As I said earlier in the thread,
> the rest of the function *doesn't* add "file://"; it adds "file:" without
> the slashes.  But if I add "file://", the rest of the function leaves this
> alone.  I guess browse-url-filename-alist would have to be changed to make
> it correctly add "file://" in this situation, but I haven't tried to dig
> into it to figure out how it works.  It seemed easier (and harmless) to just
> add it myself.


We were recently discussing if it should be file: or file:/// for w32.
I really do not know which is best but I suspect file:/// have a
slightly better chance to work. I just saw an instance
(css-validator.jar with Rhino) where it worked better.

Re: Cygwin patches

[Ken Brown]

On 11/22/2009 6:30 PM, Lennart Borgman wrote:
> On Mon, Nov 23, 2009 at 12:23 AM, Ken Brown <kbrown@cornell.edu> wrote:
>> On 11/22/2009 6:03 PM, Chong Yidong wrote:
>>> Ken Brown <kbrown@cornell.edu> writes:
>>>
>>>> Thanks for the suggestion.  My revised patch is attached.  If it looks
>>>> OK, could someone please check it in?
>>> Your patch looks incorrect.  It's not your job to add the "file://"
>>> part; that's done in the rest of the function.  Does this corrected
>>> patch do the right thing on cygwin?
>> Yes, it does do the right thing on cygwin.  As I said earlier in the thread,
>> the rest of the function *doesn't* add "file://"; it adds "file:" without
>> the slashes.  But if I add "file://", the rest of the function leaves this
>> alone.  I guess browse-url-filename-alist would have to be changed to make
>> it correctly add "file://" in this situation, but I haven't tried to dig
>> into it to figure out how it works.  It seemed easier (and harmless) to just
>> add it myself.
> 
> 
> We were recently discussing if it should be file: or file:/// for w32.
> I really do not know which is best but I suspect file:/// have a
> slightly better chance to work. I just saw an instance
> (css-validator.jar with Rhino) where it worked better.

So are you saying that "file:" (with no slashes) is added by design?  It 
doesn't work for me.  (After the call to cygpath, I have a path of the 
form C:/blah/blah/blah.html; I need to add "file://", since otherwise 
only "file:" gets added and I don't have a valid URL.)

Ken

Re: Cygwin patches

[Lennart Borgman]

On Mon, Nov 23, 2009 at 2:37 AM, Ken Brown <kbrown@cornell.edu> wrote:
> On 11/22/2009 6:30 PM, Lennart Borgman wrote:
>>
>> On Mon, Nov 23, 2009 at 12:23 AM, Ken Brown <kbrown@cornell.edu> wrote:
>>>
>>> On 11/22/2009 6:03 PM, Chong Yidong wrote:
>>>>
>>>> Ken Brown <kbrown@cornell.edu> writes:
>>>>
>>>>> Thanks for the suggestion.  My revised patch is attached.  If it looks
>>>>> OK, could someone please check it in?
>>>>
>>>> Your patch looks incorrect.  It's not your job to add the "file://"
>>>> part; that's done in the rest of the function.  Does this corrected
>>>> patch do the right thing on cygwin?
>>>
>>> Yes, it does do the right thing on cygwin.  As I said earlier in the
>>> thread,
>>> the rest of the function *doesn't* add "file://"; it adds "file:" without
>>> the slashes.  But if I add "file://", the rest of the function leaves
>>> this
>>> alone.  I guess browse-url-filename-alist would have to be changed to
>>> make
>>> it correctly add "file://" in this situation, but I haven't tried to dig
>>> into it to figure out how it works.  It seemed easier (and harmless) to
>>> just
>>> add it myself.
>>
>>
>> We were recently discussing if it should be file: or file:/// for w32.
>> I really do not know which is best but I suspect file:/// have a
>> slightly better chance to work. I just saw an instance
>> (css-validator.jar with Rhino) where it worked better.
>
> So are you saying that "file:" (with no slashes) is added by design?  It
> doesn't work for me.  (After the call to cygpath, I have a path of the form
> C:/blah/blah/blah.html; I need to add "file://", since otherwise only
> "file:" gets added and I don't have a valid URL.)


Yes, it is by design. Maybe it is because know one knows how many / to
add? In my case, plain w32, it should be three
(file:///c:/some/where.html).

Re: Cygwin patches

[Chong Yidong]

Ken Brown <kbrown@cornell.edu> writes:

> So are you saying that "file:" (with no slashes) is added by design?
> It doesn't work for me.  (After the call to cygpath, I have a path of
> the form C:/blah/blah/blah.html; I need to add "file://", since
> otherwise only "file:" gets added and I don't have a valid URL.)

Does this change (instead of the other patches we've been considering)
do the right thing on Cygwin?

*** emacs/lisp/net/browse-url.el.~1.86.~	2009-11-22 18:00:33.000000000 -0500
--- emacs/lisp/net/browse-url.el	2009-11-22 20:49:42.000000000 -0500
***************
*** 443,449 ****
      ;; it in anonymous cases.  If it's not anonymous the next regexp
      ;; applies.
      ("^/\\([^:@]+@\\)?\\([^:]+\\):/*" . "ftp://\\1\\2/")
!     ,@(if (memq system-type '(windows-nt ms-dos cygwin))
            '(("^\\([a-zA-Z]:\\)[\\/]" . "file:\\1/")
              ("^[\\/][\\/]+" . "file://")))
      ("^/+" . "file:///"))
--- 443,449 ----
      ;; it in anonymous cases.  If it's not anonymous the next regexp
      ;; applies.
      ("^/\\([^:@]+@\\)?\\([^:]+\\):/*" . "ftp://\\1\\2/")
!     ,@(if (memq system-type '(windows-nt ms-dos))
            '(("^\\([a-zA-Z]:\\)[\\/]" . "file:\\1/")
              ("^[\\/][\\/]+" . "file://")))
      ("^/+" . "file:///"))

Re: Cygwin patches

[Ken Brown]

On 11/22/2009 8:50 PM, Chong Yidong wrote:
> Ken Brown <kbrown@cornell.edu> writes:
> 
>> So are you saying that "file:" (with no slashes) is added by design?
>> It doesn't work for me.  (After the call to cygpath, I have a path of
>> the form C:/blah/blah/blah.html; I need to add "file://", since
>> otherwise only "file:" gets added and I don't have a valid URL.)
> 
> Does this change (instead of the other patches we've been considering)
> do the right thing on Cygwin?

No.  It still just adds "file:" to the windows path I get after the call 
to cygpath.  And I can't omit the call to cygpath; if I do that, I'll 
have a URL with the right number of slashes, but it won't be one that 
the Windows browser will understand.

Ken

Re: Cygwin patches

[Ken Brown]

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

On 11/22/2009 9:09 PM, Ken Brown wrote:
> On 11/22/2009 8:50 PM, Chong Yidong wrote:
>> Ken Brown <kbrown@cornell.edu> writes:
>>
>>> So are you saying that "file:" (with no slashes) is added by design?
>>> It doesn't work for me.  (After the call to cygpath, I have a path of
>>> the form C:/blah/blah/blah.html; I need to add "file://", since
>>> otherwise only "file:" gets added and I don't have a valid URL.)
>>
>> Does this change (instead of the other patches we've been considering)
>> do the right thing on Cygwin?
> 
> No.  It still just adds "file:" to the windows path I get after the call 
> to cygpath.  And I can't omit the call to cygpath; if I do that, I'll 
> have a URL with the right number of slashes, but it won't be one that 
> the Windows browser will understand.

On the other hand, if we adopt part of the patch that Lennart proposed in

   http://lists.gnu.org/archive/html/bug-gnu-emacs/2009-11/msg00343.html

then it does work on cygwin.  And, I gather from Lennart's post, this 
also solves a problem on Windows.  The combined patch is attached. 
[I've also taken the opportunity to make a slight improvement in 
browse-url-default-windows-browser by using call-process instead of 
shell-command.]

Does this look OK?

Ken

[-- Attachment #2: browse-url.el.patch --]
[-- Type: text/plain, Size: 1425 bytes --]

--- browse-url.el.orig	2009-11-20 16:49:49.000000000 -0500
+++ browse-url.el	2009-11-23 14:33:01.843750000 -0500
@@ -444,7 +444,7 @@
     ;; applies.
     ("^/\\([^:@]+@\\)?\\([^:]+\\):/*" . "ftp://\\1\\2/")
     ,@(if (memq system-type '(windows-nt ms-dos cygwin))
-          '(("^\\([a-zA-Z]:\\)[\\/]" . "file:\\1/")
+          '(("^\\([a-zA-Z]:\\)[\\/]" . "file:///\\1/")
             ("^[\\/][\\/]+" . "file://")))
     ("^/+" . "file:///"))
   "An alist of (REGEXP . STRING) pairs used by `browse-url-of-file'.
@@ -699,6 +699,10 @@
 (defun browse-url-file-url (file)
   "Return the URL corresponding to FILE.
 Use variable `browse-url-filename-alist' to map filenames to URLs."
+  (if (eq system-type 'cygwin)
+      (let ((winfile (with-output-to-string
+              (call-process "cygpath" nil standard-output nil "-m" file))))
+	(setq file (substring winfile 0 -1))))
   (let ((coding (and (default-value 'enable-multibyte-characters)
 		     (or file-name-coding-system
 			 default-file-name-coding-system))))
@@ -835,7 +839,7 @@
 	     (shell-command (concat "start " (shell-quote-argument url)))
 	   (error "Browsing URLs is not supported on this system")))
 	((eq system-type 'cygwin)
-	 (shell-command (concat "cygstart " (shell-quote-argument url))))
+	 (call-process "cygstart" nil nil nil url))
 	(t (w32-shell-execute "open" url))))
 
 (defun browse-url-default-macosx-browser (url &optional new-window)

Re: Cygwin patches

[Chong Yidong]

Ken Brown <kbrown@cornell.edu> writes:

> On the other hand, if we adopt part of the patch that Lennart proposed in
>
>   http://lists.gnu.org/archive/html/bug-gnu-emacs/2009-11/msg00343.html
>
> then it does work on cygwin.  And, I gather from Lennart's post, this
> also solves a problem on Windows.  The combined patch is
> attached. [I've also taken the opportunity to make a slight
> improvement in browse-url-default-windows-browser by using
> call-process instead of shell-command.]
>
> Does this look OK?

Looks OK, and checked in.  Thanks.