(In reply to comment #0)
> We have internally ported libvte to Windows, and it works fine for our
> purposes. We would like to contribute back the port that we created, however
> some more time
> is needed to review/cleanup the patches. To decide whether we invest this
> further work, I'd like to know whether the VTE maintainers are interested.
> Would such a contribution be accepted to vte mainline?

I'm interested, sure.

> We basically left out all POSIX/UNIX specific pty handling code, as we are
> using  vte_terminal_feed() do the communication part ourselves, vte is only
> responsible to display what the application sends to it.

So, does that mean it doesn't work as a terminal emulation widget on win32?  That ain't good.

I don't really see what you mean on "it doesn't work as a terminal emulation widget". The terminal emulation part is left intact, but instead of forking a program on a pty and displaying its output, we are feeding the terminal using
vte_terminal_feed()

I'm not sure whether the notion of pseudo terminals exist on Windows at all. And even if there was pty support on Windows, what point is it in running unmodified Windows apps inside a terminal emulator?

In my opinion VTE for Windows could be useful for a graphical SSH client. This part is not finished in our code though, we are only using it to display one-way data (e.g. replay a recorded terminal session).

I see.

Anyway, at any point you can get the command line ./vte tool running on Windows I would be more than happy to commit it :).

* wonders how mingw32+rxvt+bash works.

Let me add a "me too!" as I suspect your port will have done some very useful work in tidying up the code to use the portable glib equivalents...

I am also interested in this, and I'd volunteer to take a look at the code.

Created attachment 92923 [details] [review]
Patch against vte 0.12.1

Hey, can you do a svn diff against that version?
The diff you attached has lots of noise, including a patch in a patch.

Created attachment 94279 [details] [review]
New/recreated/enhanced version of the patch

Both Cody and I got Balazs to send us his patch; the patch I'm attaching here is my work I've been doing to get vte working on win32, mostly doing it by hand, with Balazs patch as a point of reference.

Note that this patch differs from the original one in some key ways:
1) Uses the regular autotools/make toolchain for building. I have been cross compiling vte using the approach listed on http://live.gnome.org/Cross_compiling_GTK%2B_for_Win32 . I am using the following command line in combination with this patch:
./mingw-configure --enable-explicit-deps=no --disable-x11 --disable-gnome-pty-helper --enable-debug --prefix=/opt/gtk

2) Balazs had mentioned he had been using this for a "display only" use of vte. At work here we need to use vte as a real terminal emulator, hooked up to a telnet or ssh session, so more work has gone into making that aspect of things work. In particular, some of the changes to add the VTE_DATADIR stuff (with inspiration from the way gtk+ finds it's data files in an x-platform way) were added to let us find the needed termcap file so that key mapping works properly.

3) No testing/effort to see how well the python stuff works, if at all.

4) gtk+ on win32 does not currently fire the visibility_notify_event at all, so a hack was added to make the initial display/painting work. Ideally, gtk+/win32 would be fixed to actually do visibility notify properly, and this part of the patch would be unnecessary.

5) The default order of the vte backend was switched so that the pango backend would be used by default on win32. Before that, the FT one was picked, which had lots of rendering problems/artifacting.

If folks want, I can make an effort to split this patch into discreet fixes, with explanations, to try to make this easier to follow/apply/review. Unfortunately, the only pty-less vte example in C I have uses libssh2, which doesn't work on win32, so I can't provide an easy example. Here is a screenshot of a small gtk# application I wrote using SharpSSH + vte-sharp to demo vte on win32: http://bzr.medsphere.com/~peter/vte-win32-hot-shit.png

Wow, great. I'm still overwhelmed with other stuff, but it is very nice to hear
that our patches initiated such effort. I know it must have been a lot of work to make something functional of our original patch.

FYI: we used the Python bindings on Windows, so they did work in our patched version.

Having a fully functional libvte on Windows is cool.

Also note that for a regex.h, etc solution, I am using Tor's (tml) libgnurx based on his suggestions to that effect found at: http://ftp.gnome.org/pub/gnome/binaries/win32/dependencies/regex.README

That can be found at: ftp://ftp.gnome.org/pub/gnome/binaries/win32/dependencies

Created attachment 104608 [details] [review]
Latest version based on feedback from behdad

Ok, so based on some feedback from behdad on IRC, I enhanced this to not use nearly as many #ifndef G_OS_WIN32 checks. We really only use this now for some win32 specific path checking stuff for the termcap file, etc.

Thoughts?

+#ifdef SIGCHLD
 	info.signum = signum;
 	if (signum != SIGCHLD) {

Shouldn't these be 

#ifdef HAVE_SIGCHLD

? I don't think we should rely on SIGCHLD being a preprocessor definition.

#ifdef G_OS_WIN32
+
+G_WIN32_DLLMAIN_FOR_DLL_NAME(static, dll_name)
+
+const gchar *
+_vte_get_datadir ()

Why do we need _vte_get_datadir() at all?  I don't see it being used.  If we do need it though, we should change it to be defined always, but have a different implementation for win32.

Hm, didn't mean to NEEDINFO.

The _vte_get_datadir is the approach used by gtk+ on win32 to have it be relocatable. Basically, it lets you locate the data files relative to where the library itself is installed.


As for using SIGCHLD as a preprocessor directive, I did that based on the existing use of things like that in src/pty.c: _vte_pty_reset_signal_handlers. I can definitely change that if you'd like though.

Thanks for the feedback!

Ah, OK I see how VTE_DATADIR is defined to call it now.  It does seem cleaner though to just have a global function _vte_get_datadir() instead of having a preprocessor macro call a function.  So it would look like:

== header file ==
const gchar *_vte_get_datadir ();

== implementation file ==
const char *
_vte_get_datadir()
{
#ifdef G_OS_WIN32
    static gchar *_gtk_datadir = NULL;
    if (gtk_datadir == NULL)
        gtk_datadir = g_win32_get_package_installation_subdirectory(NULL, dll_name, "share");
    return _gtk_datadir;
#else
    return VTE_DATADIR;
#endif
}

As for SIGCHLD is used elsewhere in vte that way, that's probably fine then.

Created attachment 104733 [details] [review]
Update based on feedback from Colin

Here's a new version based on the feedback from Colin.

Thoughts?

I think assuming signals are macros is quite widespread and accepted.  Quick review:


- Why are the X11 tweaks to configure.in needed?


- I think you can do the kill and sigaction checks unconditionally here:

+AC_CHECK_HEADERS(signal.h)
+if test x$ac_cv_header_signal_h != xno ; then
+	AC_CHECK_FUNCS([sigaction kill])
+fi

That shouldn't make any problems.  That is, unless Win32 has a kill() function that is totally different.


- Why do we skip curses/termcap search on win32 here:

+if test "x$platform_win32" = "xno"; then
+	 Look for ncurses or curses or termcap.


- Guess this should be HAVE_SYS_IOCTL_H:

+#ifdef HAVE_IOCTL_H
 #include <sys/ioctl.h>
+#endif


- The Pango backend is really really slow.  Until we fix that, the ft2 should be preferred.  So, this change is not allowed:

--- src/vtedraw.c	(revision 2023)
+++ src/vtedraw.c	(working copy)
@@ -42,13 +42,13 @@
 	&_vte_draw_xft,
 #endif /* HAVE_XFT2 */
 #endif /* !X_DISPLAY_MISSING */
-	&_vte_draw_ft2,
 #ifndef X_DISPLAY_MISSING
 #ifdef HAVE_GL
 	&_vte_draw_gl,
 #endif /* HAVE_GL */
 #endif /* !X_DISPLAY_MISSING */
 	&_vte_draw_pango,
+	&_vte_draw_ft2,
 #ifndef X_DISPLAY_MISSING
 #ifdef HAVE_PANGOX
 	&_vte_draw_pango_x,

Why did you need that?  There seems to be some HAVE_* conditionals missing around the ft2 one btw.  Seems like we pretty much assume freetype and fontconfig are available.  Would be good to remove that assumption.


- Why not build the command-line tool on Win32 again?  Cause there's no sh?  Or the pty stuff fails?

+if PLATFORM_WIN32
+bin_PROGRAMS =
+else
 bin_PROGRAMS = vte
+endif


- Same about slowcat.  That doesn't seem to need anything Unixish.


That's all.  This actually looks really good now!

Regarding the tools, it was because they used VTE_DATADIR, but we'd defined _vte_get_datadir in vte.c, which they didn't link to.  We could either link those tools to vte.c, or split the function out into a new file (utils.c?).

Linking to vte.c is definitely wrong.  Maybe we should make vte_get_datadir() public then if there are legit uses for it.  IIRC Pango has such a function.

Yeah, from pango-utils.h:

#ifdef PANGO_ENABLE_BACKEND

/* On Unix, return the name of the "pango" subdirectory of SYSCONFDIR
 * (which is set at compile time). On Win32, return the Pango
 * installation directory (which is set at installation time, and
 * stored in the registry). The returned string should not be
 * g_free'd.
 */
G_CONST_RETURN char *   pango_get_sysconf_subdirectory (void) G_GNUC_PURE;

/* Ditto for LIBDIR/pango. On Win32, use the same Pango
 * installation directory. This returned string should not be
 * g_free'd either.
 */
G_CONST_RETURN char *   pango_get_lib_subdirectory (void) G_GNUC_PURE;

#endif /* PANGO_ENABLE_BACKEND */


Should we do something similar?

What would the use for it be from the application's point of view?

Created attachment 104751 [details] [review]
New version based on latest feedback/questions

Ok, here's a new version. To answer some questions:

* I added a comment before the X11 checks. Basically, AC_PATH_XTRA ends up screwing up our CFLAGS and including -I/usr/include even though it didn't find X11, making our compile fail.

* I've made slowcat and vte compile, and slowcat works. the vte app won't work right now because it has no actual underlying console to open, but it should be possible to later drop in some win32 specific replacement to demonstrate vte.

* Compared to when I first started this, the FT2 backend now actually works on win32, so I've reverted that change. FT2 works fine now.

One thing pending still is the VTE_DATADIR stuff. For now, interpreter.c is just using VTE_DATADIR and not _vte_get_datadir (), as we don't have a public function to get that. Should we add this as public API? Not worry about it right now?

Thoughts?

(In reply to comment #22)
> 
> * I've made slowcat and vte compile, and slowcat works. the vte app won't work
> right now because it has no actual underlying console to open, but it should be
> possible to later drop in some win32 specific replacement to demonstrate vte.

Humm, if functionality is lost, we need to make that explicit.  What are the functions missing here that make vte not work?  We should make configure fail if they are not available, unless some --disable-something is passed.  --disable-pty?  As I said, not sure what exactly is the crucially missing bits.


> * Compared to when I first started this, the FT2 backend now actually works on
> win32, so I've reverted that change. FT2 works fine now.

Good.

> One thing pending still is the VTE_DATADIR stuff. For now, interpreter.c is
> just using VTE_DATADIR and not _vte_get_datadir (), as we don't have a public
> function to get that. Should we add this as public API? Not worry about it
> right now?

As we talked on IRC, lets forget about interpret.c not working on win32.  It's just a test after all.  No API needed.

> Thoughts? 

Behdad: The code in question does a few things, if you pass "--console" to vte, it does (paraphrasing):

consolefd = open("/dev/console", O_RDONLY | O_NOCTTY);
if (consolefd != -1) {
  if (ioctl(consolefd, TIOCCONS, &yes) != -1) {
    channel = g_io_channel_unix_new(consolefd);
    blah;
    blah;
  }
}

This, of course, doesn't work because 1) we don't have /dev/console on Win32, and O_NOCTTY isn't even defined on Win32.

Otherwise, the default is to open a shell, which calls:
vte_terminal_fork_command(terminal,
                          command, NULL, env_add,
                          working_directory,
                          TRUE, TRUE, TRUE);

Which doesn't currently work because vte_terminal_fork_command ultimately tries to open a PTY.

If not openning a shell, vte does the following:
i = vte_terminal_forkpty(terminal, env_add, working_directory, TRUE, TRUE, TRUE);

Which also won't work on Win32, because of lack of PTY support.

So yes, ultimately, the lack of PTYs is the issue. The question really is, how do we specifically want to handle this? What do the vte_terminal_fork_command, vte_terminal_forkpty, etc commands do if we pass --disable-pty? If we pass --disable-pty, do we just not build the vte.exe binary?

(In reply to comment #24)
> Behdad: The code in question does a few things, if you pass "--console" to vte,
> it does (paraphrasing):
> 
> consolefd = open("/dev/console", O_RDONLY | O_NOCTTY);
> if (consolefd != -1) {
>   if (ioctl(consolefd, TIOCCONS, &yes) != -1) {
>     channel = g_io_channel_unix_new(consolefd);
>     blah;
>     blah;
>   }
> }
> 
> This, of course, doesn't work because 1) we don't have /dev/console on Win32,
> and O_NOCTTY isn't even defined on Win32.
> 
> Otherwise, the default is to open a shell, which calls:
> vte_terminal_fork_command(terminal,
>                           command, NULL, env_add,
>                           working_directory,
>                           TRUE, TRUE, TRUE);
> 
> Which doesn't currently work because vte_terminal_fork_command ultimately tries
> to open a PTY.
> 
> If not openning a shell, vte does the following:
> i = vte_terminal_forkpty(terminal, env_add, working_directory, TRUE, TRUE,
> TRUE);
> 
> Which also won't work on Win32, because of lack of PTY support.
> 
> So yes, ultimately, the lack of PTYs is the issue. The question really is, how
> do we specifically want to handle this? What do the vte_terminal_fork_command,
> vte_terminal_forkpty, etc commands do if we pass --disable-pty? If we pass
> --disable-pty, do we just not build the vte.exe binary?

The api should simply return -1 which is what it does on errors currently.  (this should be documented).  And vte.exe not built sounds fine.

Better would be to have an option to just pipe stdin to the vte widget, and pipe all keystrokes to stdout.  That would be quite useful in fact.

I wouldn't force people compiling on Windows to pass --disable-pty because Windows will never have PTYs.  

On the other hand, if we're on Unix we should require the PTY APIs and hard fail if they don't appear to exist unless you do pass --disable-pty.  

Cygwin counts as Unix here, not Windows.

As for what to do with _fork_command etc. - we could just have them return an error immediately on Windows via ifdefs, or somewhat trickier would be to not define them at all on Windows.

(In reply to comment #26)
> I wouldn't force people compiling on Windows to pass --disable-pty because
> Windows will never have PTYs.  

Main question then is to identify that.  Needs hand-written configure.in magic which I try to avoid.  Still has the problem that default configuration on different systems would provide quite differing level of functionalities.

> On the other hand, if we're on Unix we should require the PTY APIs and hard
> fail if they don't appear to exist unless you do pass --disable-pty.  

Yes, this is my main objection against the current patch.  And I totally agree with your blogpost about this issue.

> Cygwin counts as Unix here, not Windows.
> 
> As for what to do with _fork_command etc. - we could just have them return an
> error immediately on Windows via ifdefs, or somewhat trickier would be to not
> define them at all on Windows.

Not defining needs a new generated-distributed header file.  Not worth it.

Ok, so the _fork_command, etc right now *do* end up just returning -1, because ultimately they call into the pty.c code, which ends up return -1 because things like _vte_pty_getpt don't have getpt (), etc on win32. I think this is preferable to let it fail at this level, compared to coding PTY specific failures inside the vte.c code. Thoughts?

As for the "detecting what should be required/supported based on the system", it's not really clear to me the conclusion of the discussion between Behdad and Colin. What's the expected behaviour for both Win32 and true "POSIX-like" systems, when it comes to ./configure? And on what level should disabling vte.exe happen? Just based on the detected platform being Win32 in configure.ac?

Thanks in advance.

I still prefer forcing users into --disable-pty on windows.  Otherwise it gives them the feeling that "I successfully compiled vte on windows, but my gnome-terminal doesn't work"...

I don't quite understand the motivation there, since a standalone terminal (like gnome-terminal) isn't of any use on Windows.  

(Again, Cygwin counts as its own operating system here, and we should definitely hard require pty development headers for Cygwin).

But it's not really a big deal - very few people are going to be building VTE for win32.  Ideally actually GNOME would ship an official binary installer, along with GTK+ etc.

(In reply to comment #30)
> I don't quite understand the motivation there, since a standalone terminal
> (like gnome-terminal) isn't of any use on Windows.  

Why again?  I'd love to have cmd.exe running in g-t instead of the stupid Windows terminal.

I don't think that's possible, at least not in a sane way.  I'm not an expert on this, but from what I understand, cmd.exe is a Windows console application (http://en.wikipedia.org/wiki/Win32_console).

Windows consoles are kind of like pseudoterminals in that it's heavily tied into the OS.  On Windows, that component is called CSRSS (http://en.wikipedia.org/wiki/Client/Server_Runtime_Subsystem).

You know how cmd.exe and PowerShell have that bizarre resizing behavior?  That's due to CSRSS; essentially Windows is controlling all aspects of the display.  And it's not easy to run console applications outside of CSRSS; as I understand it, the difficulty could be compared to the effort Cygwin has to go to emulate pseudoterminals.  Trying to emulate CSRSS enough outside of Windows' control such that you could then translate things into ANSI terminal codes would be a nightmare.  

This post from PeterB on Ars Technica talks about it a bit: http://episteme.arstechnica.com/eve/forums/a/tpc/f/12009443/m/128001068831/p/3

Better to just skip the crappy OS-provided CSRSS and pseudoterminal subsystems and run Hotwire =)

(In reply to comment #32)
> I don't think that's possible, at least not in a sane way.  I'm not an expert
> on this, but from what I understand, cmd.exe is a Windows console application
> (http://en.wikipedia.org/wiki/Win32_console).
> 
> Windows consoles are kind of like pseudoterminals in that it's heavily tied
> into the OS.  On Windows, that component is called CSRSS
> (http://en.wikipedia.org/wiki/Client/Server_Runtime_Subsystem).
> 
> You know how cmd.exe and PowerShell have that bizarre resizing behavior? 
> That's due to CSRSS; essentially Windows is controlling all aspects of the
> display.  And it's not easy to run console applications outside of CSRSS; as I
> understand it, the difficulty could be compared to the effort Cygwin has to go
> to emulate pseudoterminals.  Trying to emulate CSRSS enough outside of Windows'
> control such that you could then translate things into ANSI terminal codes
> would be a nightmare.  
> 
> This post from PeterB on Ars Technica talks about it a bit:
> http://episteme.arstechnica.com/eve/forums/a/tpc/f/12009443/m/128001068831/p/3


Sure, if it's not possible, fine, I just want to make sure user is clearly informed about it.  That's all.


> Better to just skip the crappy OS-provided CSRSS and pseudoterminal subsystems
> and run Hotwire =)

I'm going to make an attempt to boil down this discussion into some quantifiable changes I need to make to this patch to make it acceptable:

1) In general, when --disable-pty is passed, the action taken is to not build vte/vte.exe.

2) On Win32, non-cygwin build, require --disable-pty explicitly, in order to inform the user about what's going on, and that vte.exe won't be built.

3) On all other platforms (cygwin, linux, other POSIX platforms, etc), fail the configure stage if the detection of fcntl, and related pty functions fails.

Is this right?

Even simpler:

1) Fail configure unless all the functions needed to build a working vte app (that runs a shell inside) are available, unless --disable-pty is passed.  When failing, the error message should suggest --disable-pty, but note that that will disable some functionalities, namely, the vte_terminal_fork*() APIs will simply return error status.

2) Don't build/install vte app if --disable-pty is passed to configure.

That's all.

Makes sense. I will post an updated patch when I have the above implemented.

Created attachment 106199 [details] [review]
Patch with latest suggestions about configure behaviour

Here's a new version based on the suggestions from Behdad for the ./configure behaviour. My autotools foo is fair at best, so there may be a better way to do what I'm doing here.

Thoughts?

Looks good.  Ping me after GNOME 2.22 is out, and I'll do a final review and commit.

Thanks!

Ping, GNOME 2.22 is out!

Was going to commit, but seems like the pty stuff is malfunctioning on Linux now.

Running vte with the patch, I get:

(vte:31773): Vte-WARNING **: Error setting PTY size: Success.

(vte:31773): Vte-WARNING **: Error reading PTY size, using defaults: Success.

(vte:31773): Vte-WARNING **: Error reading PTY size, using defaults: Resource temporarily unavailable.

(vte:31773): Vte-WARNING **: Error setting PTY size: Resource temporarily unavailable.

(vte:31773): Vte-WARNING **: Error reading PTY size, using defaults: Resource temporarily unavailable.

Argh. Ok, I'll look at this next week.

Created attachment 111580 [details] [review]
New version with fix for pty issue

This should fix the issue you were seeing Behdad. I had a check against HAVE_IOCTL_H instead of HAVE_SYS_IOCTL_H.

Humm, the warnings are gone, but the bash inside vte is still unusable...

Also, can you not change sleep to SLEEP and just redefine sleep?  Same about USLEEP.  In fact, configure checks for sleep and usleep is bogus as you are redefining them unconditionally for win32.

Umm, my bad.

Humm, it's actually worse now.  I get:

[behdad:0 vte]$ src/vte
Gtk-Message: Failed to load module "gnomebreakpad": libgnomebreakpad.so: cannot open shared object file: No such file or directory

(lt-vte:31057): Vte-WARNING **: Error setting PTY size: No such file or directory.

(lt-vte:31057): Vte-WARNING **: Error reading PTY size, using defaults: No such file or directory.

(lt-vte:31057): Vte-WARNING **: Error reading PTY size, using defaults: Resource temporarily unavailable.

(lt-vte:31057): Vte-WARNING **: Error setting PTY size: Success.

(lt-vte:31057): Vte-WARNING **: Error reading PTY size, using defaults: Success.

and no vteapp running...

Did you try the patch on Linux?

You USLEEP definition is also wrong.  Should be Sleep((n)/1000), not Sleep(n).

How is the status on this ?
We try to port pida to windows and use vte in the most important way :)

Someone needs to update the patch, fixing the brokenness (see comment 45 and 46).

Dear developers
To increase the noise level, I wanted to know if the plans to include a port for Windows have been dropped? 

Since there is (was) a patch, it would be great if it were included in trunk. This may help cross-platform apps using VTE provide the same functionality on all platforms.

If you want the feature included in vte, consider working on the patch. It needs to be updated to apply to vte from git master, and to fix the issues raised in the review.

Anyone still interested in this? Should be much easier to do with vte git master. I'd suggest to start from scratch, and instead of sprinkling ifdefs around pty.cc, to split pty into 'unix' and 'dummy' implementations.

I recently learned that there's someone using vte on windows, on cygwin (for the PTY support) but using the win32 gdk backend instead of X.

Since that apparently works fine, let's call this one OBSOLETE.