MingW build of Transmission

[rb07]

I'm sending the patch used to build libtransmission and QTr using mingw (in a cross build environment, but its the same installing the native Mingw and all required dependencies).

The patch has a binary, the qtr.ico, which I don't include in the Wiki patch since I instruct people to copy or download the image. It would be nice to have it on the sources and not make everybody download from svn.

It also has changes I just like, i.e. not necessary (like the number of decimal digits displayed).

[charles]

I've forwarded the miniupnpc diff upstream:

​http://miniupnp.tuxfamily.org/forum/viewtopic.php?p=1802#1802

[charles]

passing-third-party-patches-upstream, part two: Juliusz is the author of the upstream DHT package, so I've CCed him to this ticket because of your win32 patches to DHT.

[charles]

passing-third-party-patches-upstream, part three: I've passed the JSON_parser.h diff upstream to Jean at ​http://fara.cs.uni-potsdam.de/~jsg/json_parser/

[rb07]

Some of the things missing in the patch:

• The change to configure should really go in configure.am;
• The Qt README should be changed to say something like "use qmake qtr.pro; make release" because using just make means "make debug" which results in an application that doesn't work in many platforms (Linux included, I guess all that use the free version).

In Qt the debug version has all assertions enabled, some of those are bugs, they are corrected with later versions of Qt but as of 4.7beta there is at least one that makes debugging impossible, the app exits when you open a panel. I also patched Qt to be able to debug the app.

The miniupnp change was not important, it only affects cross-compilation (could also affect Windows if someone goes into the trouble of making it case sensitive).

I also wonder if instead of "#ifdef WIN32" I should be using "#ifdef __MINGW__" or something like it; that's for not messing with anyone that might try to use Microsoft's VC.

[charles]

rb07, I have some thoughts on the libtransmission portions of this diff:

• the diffs to configure and Makefile.in can't be accepted as-is because those files are autogenerated by autoconf/automake. Instead, you want to make those changes to configure.ac and Makefile.am, respectively.

• Likewise, the additional -liphlpapi link should probably be added to LIBS in the *mingw32* test in configure.ac, rather than in the Makefiles. You might want to test that out and see if it works -- if so, that would be friendlier to other OSes using our autoconf system. :)

• If you define DISABLE_GETTEXT in utils.h, the _() macro will be defined to tr_strip_positional_args(), which will get rid of the positional argument noise in "%1$s (%2$s)" so that we don't have to change each of those lines on their own. Possibly adding an #ifdef (win32? mingw?) #define DISABLE_GETTEXT #endif would do the trick.

• windows doesn't understand %zu? really? :(

• what we've done with other platform-specific functions is to add a libtransmission wrapper -- maybe that's what we should do here with realpath. Adding a tr_realpath() to utils.[ch], and adding the Mark Junker's implementation inside an #ifdef in tr_realpath()'s implementation, would be good.

• I don't understand ifdeffing out the st.st_blksize test. Does win32 not support that field?

[rb07]

Replying to charles:

rb07, I have some thoughts on the libtransmission portions of this diff:

• the diffs to configure and Makefile.in can't be accepted as-is because those files are autogenerated by autoconf/automake. Instead, you want to make those changes to configure.ac and Makefile.am, respectively.

I agree.

• Likewise, the additional -liphlpapi link should probably be added to LIBS in the *mingw32* test in configure.ac, rather than in the Makefiles. You might want to test that out and see if it works -- if so, that would be friendlier to other OSes using our autoconf system. :)

Ditto.

• If you define DISABLE_GETTEXT in utils.h, the _() macro will be defined to tr_strip_positional_args(), which will get rid of the positional argument noise in "%1$s (%2$s)" so that we don't have to change each of those lines on their own. Possibly adding an #ifdef (win32? mingw?) #define DISABLE_GETTEXT #endif would do the trick.

Either that or I add gettext. I'll have to see it that fixes those pesky critters... nope, I already have gettext installed on my mingw environment.

• windows doesn't understand %zu? really? :(

I might have overdone those, the part Mingw doesn't have is the localization, as most of those really where %'zu they where (still are) only causing compile warnings and my debug output didn't show the values, so I changed them to see the values.

• what we've done with other platform-specific functions is to add a libtransmission wrapper -- maybe that's what we should do here with realpath. Adding a tr_realpath() to utils.[ch], and adding the Mark Junker's implementation inside an #ifdef in tr_realpath()'s implementation, would be good.

• I don't understand ifdeffing out the st.st_blksize test. Does win32 not support that field?

Not on my development environment, the stat.h doesn't have it (Cygwin's for instance does).

[charles]

Ah, and I saw your comment on the blocklist in the forums too. I'm looking forward to your next diff.

Thanks for digging into all these pieces. This is a great step along Transmission's path to world domination. :)

[jch]

Yes, unfortunately the mingw port insists on using a badly obsolete version of Microsoft's C runtime, so a lot of libc things we're used to is not available.

I think that this port should be rewritten by writing a set of compatibility functions rather than peppering the code with #ifdefs. As an example -- there should be a function set_nonblocking that does the fcntl under Unix and the ioctl under Windows, and the main dht.c code should just call (the right version of) this function.

Similarly, you shouldn't be defining inet_ntop, realpath etc. in multiple places -- just dump everything in a single compatibility file.

The fix_errno thing is a hack -- I can see at least two ways of doing that cleanly. Please see

​http://www.pps.jussieu.fr/~jch/software/repos/polipo/mingw.h

​http://www.pps.jussieu.fr/~jch/software/repos/polipo/mingw.c

Finally, whenever you touch the files in third-party/dht, please make sure you mark the sources as being modified.

--jch

[rb07]

Updated patch.

[rb07]

The updated patch addresses all items commented by Charles, its a full cleanup of my code changes which cover also most of those items commented by JCH, I already had taken out inet_ntop() from everywhere and placed it in utils.c; in DHT I replaced it with an external definition which I commented as being resolved by libtransmission.a (in our case, some other library otherwise).

The patch adds the fix to inet_pton() which makes the block list download work, and probably affects other parts since my old function wasn't working at all.

I read the mingw files pointed out by JCH and my thoughts is that some of it is already in transmission, it follows a very similar line as gnulib, but I would have to patch it (since its using an obsolete and incompatible header). The fix_error mess was just a way to simplify the bigger mess that I found, socketerror replacing errno in some places, and not in others... the library still doesn't handle all the errors that Windows trows at it (I've seen some 10051 "socket not connected" for instance).

Also to JCH: I'm not distributing a version of your project so I don't understand your last comment, i.e. I'm making available a patch to build Transmission Qt on Windows, its obvious the patch modifies the sources and the result is not the original. I don't intend to distribute it (didn't even put my name on the changes), and am very reluctant to send patches to third party projects which I don't intend to fully test (since I don't use DHT outside of Transmission). Actually I'm not even sure it works on the application I build, in the debugger I've seen many "dht_periodic failed with error 0" (but that's probably because my test had one private tracker, no work for DHT).

[charles]

Parts of the diff have been committed in r10814.

some thoughts on the rest of the diff:

• what does Windows not like about the select() call in web.c?

• maybe there should be a tr_mkstemp() wrapper in utils.[ch].

• the two net functions should probably stay in net.[ch] instead of moving to utils.[ch].

[rb07]

Replying to charles:

Parts of the diff have been committed in r10814.

some thoughts on the rest of the diff:

• what does Windows not like about the select() call in web.c?

Literally what I wrote in the comment, the function returns a "bad parameters" error... and I don't see anything wrong with the parameters. I didn't dig deeper, just went around, but I agree it should work (the Microsoft documentation says it should).

• maybe there should be a tr_mkstemp() wrapper in utils.[ch].

Could be, but my fix is just a quick hack. There where 2 problems with those temporary files, the first is that Windows didn't like files with more than one dot (I'm not sure if I just read that one somewhere or I did really see it not working since after changing the dot to an underscore the file name still has more than one dot, or it could be "more than one extension" where "extension" is any "dot followed by 1 to 3 chars").

The second problem is the obvious, there is no mkstemp() in Mingw, that's why I used a combination of mktemp() with open()... not the same thing, I know.

• the two net functions should probably stay in net.[ch] instead of moving to utils.[ch].

Either way is fine.

[charles]

I've committed a second part of the difffs in r10816. The rest of the diffs are either upstream or I have questions about... :)

• I'm not sure why peer-mgr.c #includes errno.h. We don't use errno in that file.

• that web.c diff just seems wrong. It would be good to know why select() is failing there on win32.

• the peer-io diff seems like overkill just to get the right errno for a dbgmsg. I'm not sure it's worth it, unless you think this could be a debugging hot area.

• in peer-io.c, %zu is appropriate for a size_t :)

• I think that adding Prefs::ENCRYPTION in session.c at line 142 is an error. Prefs::ENCRYPTION is a special case handled down on line 172.

[rb07]

Replying to charles:

I've committed a second part of the difffs in r10816. The rest of the diffs are either upstream or I have questions about... :)

• I'm not sure why peer-mgr.c #includes errno.h. We don't use errno in that file.

You use the error codes, commenting out my change:

  CC     peer-mgr.o
peer-mgr.c: In function 'peerCallbackFunc':
peer-mgr.c:1519: error: 'ERANGE' undeclared (first use in this function)

• that web.c diff just seems wrong. It would be good to know why select() is failing there on win32.

Do you want the gory detail?

Perhaps this will save somebody some wasted time: I was investigating the problem reported on the forum about the QTr builds I made available, one user said it made his PC overheat.

That part is easy to understand, the select() was just returning an error, the program never "slept" so it looped at top speed (millions of times per second on modern hardware).

I ran the app under gdb, first suspicious part is the call to curl_multi_fdset() changed max_fd to -1, but that was not the problem, Microsoft's documentation says their select() ignores the first parameter, that its there just for compatibility. I even tried with an added "max_fd= 0;", yeah, I don't trust anything.

So that leaves the select() itself as the problem. Microsoft's select() is different from the Unix one in 2 ways: it doesn't allow to use all NULLs for the sets, and the time spec has to be less than 30 days or so (from memory). No problem I can see there again.

So, as I said, the parameters are correct, the call should have worked but it doesn't. Perhaps I(we) hit a bug on Microsoft's libc (or whatever is called msvcrt.dll).

I could go back to Microsoft's documentation to see if I find something... but debugging their libc is really not in my plans. Remember I changed to gnulib? that was one reason, the errno mess was another... and I had to debug their select(); but I've left that version on the freezer for now.

• the peer-io diff seems like overkill just to get the right errno for a dbgmsg. I'm not sure it's worth it, unless you think this could be a debugging hot area.

I agree it would be overkill for making debug messages pretty.

But I suspected that mishandling of the error responses was causing the other problem I was trying to fix: the application was not working stand alone.

I am not completely sure it was the error handling, but after I hacked at those, one by one, the application started working stand alone. I wrote in the forum thread that the problem could be tracked with TcpView: the app opened all the connections to peers (obviously had previously received those peers from trackers) and never did anything with those connections, in detail I saw that it was waiting for the handshake. Could have been one mishandled code, or more, I don't know.

• in peer-io.c, %zu is appropriate for a size_t :)

The Mingw compiler doesn't handle those "z", I get dozens of warnings saying so.

As a fix I tried adding macro libraries to autoconf, so it would recognize that I have gettext and libiconv (I'm guessing needed for localization) but only the autogen.sh running improved. BTW I had to add zlib.m4 from your repository, my Gentoo autoconf libraries don't have it, that was weird.

But its not important for the Qt app, if I need complete debug output I'll change them for me. That is unless people start trying to use transmission-remote, then they'll get a lot of garbled output (the z's come out literally, instead of the value).

• I think that adding Prefs::ENCRYPTION in session.cc at line 142 is an error. Prefs::ENCRYPTION is a special case handled down on line 172.

OK, that was another quick hack, let me explain: I you use the drop-down to change encryption preferences the only thing that used to happen was a message to the console (something like "option 43 not handled" -- I don't really remember the number), so I did that change for the message to disappear and AFAIK the drop-down doesn't work, or it didn't use to, because with version 2.00 at least it started to report the correct value on the remote server.

[rb07]

About select() on Windows, its not a bug, but a Microsoft "feature".

Ref.​http://msdn.microsoft.com/en-us/library/ms740141%28VS.85%29.aspx

Any two of the parameters, readfds, writefds, or exceptfds, can be given as null. At least one must be non-null, and any non-null descriptor set must contain at least one handle to a socket.

So that's a 3rd difference with Unix select(). My workaround stands, except that the select() call should be in an #else section, I thought it would do something even with the error return, it appears it doesn't.

[charles]

Ahhh, okay.

So when there are events pending, our call to select() will succeed... but when the thread is idling, that's when we get the error.

Good http performance depends on that select() call, so maybe what we should do is test to see if any of the descriptor sets are empty before calling select(). Alternately, we could do it even simpler than that:

if( select( ... ) == -1 )
    tr_wait( msec );

What do you think of that approach?

[rb07]

Yes that looks better, but it probably doesn't work.

Just to clarify: Windows' select() returns error if any of the sets are empty.

I don't know how the curl library works, but is it likely that it will return all sets populated? I don't think so, and the workaround is a really ugly piece of code, something like:

#ifdef WIN32 && WINDOWS_REALLY_SUCKS
    if (r_fd_set.fd_count == 0 && w_fd.set.fd_count == 0 && c_fd_set.fd_count == 0)
        Sleep(msec);
    else
        select(0,
            (r_fd_set.fd_count == 0)? NULL : &r_fd_set,
            (w_fd.set.fd_count == 0)? NULL : &w_fd_set,
            (c_fd_set.fd_count == 0)? NULL : &c_fd_set,
            &t);
#endif

For reference, a little test program with 3 test cases, first is what libtransmission wanted to do, second what Windows likes, and third just to make sure any empty sets produce the error:

$ cat test_select.c
/*
 * gcc -g -o test_select test_select.c -lws2_32
 */
#include <stdio.h>
#include <ws2tcpip.h>   /* select(), struct timeval, etc. */

void
tr_netInit( void )
{
    static int initialized = FALSE;

    if( !initialized )
    {
        WSADATA wsaData;
        WSAStartup( MAKEWORD( 2, 2 ), &wsaData );
        initialized = TRUE;
    }
}

int main () {

  long msec = 2500L;    /* 2.5 sec */
  struct timeval t;
  fd_set r_fd_set, w_fd_set, e_fd_set;
  int res;
  SOCKET s;

  FD_ZERO(&r_fd_set);
  FD_ZERO(&w_fd_set);
  FD_ZERO(&e_fd_set);
  t.tv_sec= msec / 1000;
  t.tv_usec= 1000 * (msec % 1000);

  tr_netInit();

  res= select(1, &r_fd_set, &w_fd_set, &e_fd_set, &t);

  if (res == 0) {
    printf("OK, time expired\n");
    return 0;
  } else {
    char buffer[1024];
    printf("ERROR, select() returned %d\n", res);
    res= WSAGetLastError();
    FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, res, 0, buffer, 1024, NULL);
    printf("Error (%d), which means: %s", res, buffer);
//    return 1;
  }

  /* Let's do it their way ... */

  s= socket(AF_INET, SOCK_STREAM, 0);
  FD_SET(s, &r_fd_set);
  res= select(1, &r_fd_set, NULL, NULL, &t);
  if (res == 0) {
    printf("OK, time expired\n");
//    return 0;
  } else {
    char buffer[1024];
    printf("ERROR, select() returned %d\n", res);
    res= WSAGetLastError();
    FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, res, 0, buffer, 1024, NULL);
    printf("Error (%d), which means: %s", res, buffer);
//    return 1;
  }

  /* One last test: does it accept a combination of empty and non-empty sets? */

  res= select(1, &r_fd_set, &w_fd_set, NULL, &t);
  if (res == 0) {
    printf("OK, time expired\n");
    return 0;
  } else {
    char buffer[1024];
    printf("ERROR, select() returned %d\n", res);
    res= WSAGetLastError();
    FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, res, 0, buffer, 1024, NULL);
    printf("Error (%d), which means: %s", res, buffer);
    return 1;
  }
}

$ gcc -g -o test_select test_select.c -lws2_32

$ time ./test_select
ERROR, select() returned -1
Error (10022), which means: An invalid argument was supplied.
OK, time expired
ERROR, select() returned -1
Error (10022), which means: An invalid argument was supplied.

real    0m2.547s
user    0m0.015s
sys     0m0.000s

Only the second test "select(1, &non-empty-set, NULL, NULL, &t)" worked.

[charles]

r10844: strip out the ' token from printf messages in tr_strip_positional_args().

[charles]

r10845: possible win32 fix for the curl thread's select() call. needs actual real-world win32 testing.

[charles]

rb07: could you update your diff?

[rb07]

Replying to charles:

rb07: could you update your diff?

Yes, of course. I'll check out revision 10845 and see how building goes.

I'll also put out a new build to allow people, other than me, to test the changes.

[rb07]

Patch to r10847

[rb07]

Diff with r10847 uploaded, a few notes about other than the obvious changes:

• mmap() on Windows is another case like select(), most people say Mingw doesn't have mmap() and I've found a couple of implementations. But the function is there, somewhere, my guess is that binutils is providing it... but a very restricted mmap(). If I'm right the options used by libtransmission are exactly the fixed options in the restricted mmap().

• In net.[ch] I changed the inet_ntop() and inet_pton() functions from static to non. Reason: at least thirdparty/dht's lib wants one of the functions, so they should be exported.

[charles]

Okay, I've applied more of your diff to r10858 now :)

I think all of the remaining diffs are "upstream" in the sense that I'd like to get direction from those component's authors instead of stepping on their code.

• dht -- jch weighed in @ comment 10, but the dht.c code still has #ifdefs instead of wrapper functions

• tr-lpd.c -- ezset apparently hasn't seen this ticket yet. :)

• miniupnpc -- the upstream author has a question about this at ​http://miniupnp.tuxfamily.org/forum/viewtopic.php?p=1802#1802 ... rb07 could post an answer for him there?

[rb07]

Replying to charles: ...

• miniupnpc -- the upstream author has a question about this at ​http://miniupnp.tuxfamily.org/forum/viewtopic.php?p=1802#1802 ... rb07 could post an answer for him there?

Done.

[rb07]

Problems running r10858:

The app connects to peers but only for a very limited time, about 1 or 2 seconds, downloads a little, disconnects... keeps doing that forever.

Version 2.00 with only my select() change (above -- minus the syntax errors) has no such problem.

I haven't investigated the cause, could be the calls to libcurl changing the timeout? some other changes in r10858?

This is for the app working stand-alone (local session), I also have it working in a different PC with a remote session and there has been no problem there.

[rb07]

The problem is not the select().

I still don't know what causes it, but monitoring that select() doesn't show a single failure.

I was testing with one torrent that had 20 seeders and only myself as leacher, strange thing is that the app only connects to one peer at a time, downloads at very low speed for about 5 seconds, then disconnects. A while later does the same with a different peer.

Next I'll check differences between my version 2.00 and r10858, I think that's easier than debugging the app.

[rb07]

The problem with r10847 (sorry for my mistake in the above messages, I was not using 10858) was in peer-io.c, using the wrong error return codes is not an option.

I'm uploading a new patch on that revision, only peer-io.c was changed, and I kept the changes to a minimal set plus a little of debugging in case I needed it.

Instead of changes like those I made we could go with the "sockerrno" type fix that web.[ch] has, to keep coding similar.

[rb07]

Updated patch. Good test results.

[rb07]

Looking at debug output... peer-io.c is receiving a few ENOTCONN error codes, I'm guessing that is one that should be handled as EPIPE (I mapped several similar error codes to EPIPE, which doesn't exist for Windows' sockets, but seems I missed this one).

And ECONNABORTED I'm guessing its one that really is not handled explicitly, perhaps a time-out which just tags the peer as being in error.

[jch]

Excellent, that's slowly starting to look like something usable.

Charles, are you okay with "#ifdef WIN32", or do you prefer "#ifndef HAVE_INET_PTON" and friends?

inet_pton looks wrong to me (no failure case), but I haven't read it carefully.

Code duplication for handling the idosyncratic error returns from Winsock; I suggest putting all of that in a single function (or a single macro).

In peer-mgr.c, just include errno.h unconditionally, it doesn't do any harm on non-Windows platforms.

In lpd.c, what's the deal with the inline prototype for fcntl?

In both lpd.c and dht.c, please write a system depenedent "set_nonblocking(int fd)" function rather than having #ifdefs inline.

(In short -- there should be no #ifdefs at all in the main libtransmission code, just in a small set of system-dependent files.)

--jch

[charles]

This patch doesn't apply cleanly to trunk because several of these cleanups (such as including errno.h unconditionally in peer-mgr.c) were checked in a few days ago post-r10847.

[rb07]

Replying to jch:

Excellent, that's slowly starting to look like something usable.

Charles, are you okay with "#ifdef WIN32", or do you prefer "#ifndef HAVE_INET_PTON" and friends?

inet_pton looks wrong to me (no failure case), but I haven't read it carefully.

Actually my inet_pton() is fully posix compliant, it does have the required error returns and errno code. I can't say the same about inet_ntop() since I haven't gone over that one in as much detail, the former was the one causing problems.

Code duplication for handling the idosyncratic error returns from Winsock; I suggest putting all of that in a single function (or a single macro).

In peer-mgr.c, just include errno.h unconditionally, it doesn't do any harm on non-Windows platforms.

In lpd.c, what's the deal with the inline prototype for fcntl?

tr-lpd.c ... like the comment in the line above says, "all missing" (from MingW headers). Perhaps its not clean that "all" means all the 3 changes from here.

I know its strange that MingW has a fcntl.h and that file doesn't have the prototype, just macros.

In both lpd.c and dht.c, please write a system depenedent "set_nonblocking(int fd)" function rather than having #ifdefs inline.

(In short -- there should be no #ifdefs at all in the main libtransmission code, just in a small set of system-dependent files.)

Which are?

There where "#ifdefs WIN32" before I started making changes, and there are in new code added recently (the tr-lpd.[ch] is a good example). I had to clean mistakes in all places, tried to keep the coding "style" close to what I saw... but if you want to make rules as the game progresses...

[rb07]

Replying to charles:

This patch doesn't apply cleanly to trunk because several of these cleanups (such as including errno.h unconditionally in peer-mgr.c) were checked in a few days ago post-r10847.

I don't understand.

Does this mean you want a new patch against trunk?

[jch]

if you want to make rules as the game progresses

Just to be clear: I am not one of the Transmission developers, just a friendly onlooker who's trying to be helpful. I'm trying to influence your code so that it's the best possible, with no particular regard to any existing rules.

The only people who are entitled to make any "rules" regarding the Transmission code are Charles and Livings.

--jch

[charles]

Ah, good: the diffs are getting smaller... :)

jch, the reason I wanted your input was to make sure the diffs to dht.c would be in a form that you approve of. Since you're that module's author, you definitely *do* make the rules there.

[rb07]

Patch to r10904

[charles]

rb07: I've adapted a lot of your diff into trunk in r10913. As before, I've tried to edit out as many of the #ifdefs as possible, so you might want to give it a spin to see if I've broken anything and maybe update the diff so that we can repeat the cycle again ;)

Also, I have a question -- does Windows have a policy on units to use for displaying disk capacity, vs memory size, vs bandwidth speeds, as Ubuntu does in <​https://wiki.ubuntu.com/UnitsPolicy>? For example, does Windows have a consistent use of KiB, or KB, or kB?

[charles]

jch: as you requested, I've applied rb07's patch to third-party/dht/dht.c

[jch]

Charles, please import dht-0.15, which fixes rb07's inline ifdefs.

​http://www.pps.jussieu.fr/~jch/software/files/dht-0.15.tar.gz

​http://www.pps.jussieu.fr/~jch/software/files/dht-0.15.tar.gz.asc

--jch

[rb07]

Replying to charles:

rb07: I've adapted a lot of your diff into trunk in r10913. As before, I've tried to edit out as many of the #ifdefs as possible, so you might want to give it a spin to see if I've broken anything and maybe update the diff so that we can repeat the cycle again ;)

OK. I doesn't compile so I'll take a look:

  CC     net.o
net.c:63: warning: missing braces around initializer
net.c:63: warning: (near initialization for 'tr_in6addr_any.addr.addr6._S6_un')
net.c: In function 'tr_net_strerror':
net.c:155: error: 'e' undeclared (first use in this function)

That e should be err.

  CC     peer-io.o
peer-io.c:42:1: warning: "EAGAIN" redefined
In file included from peer-io.c:14:
.../i686-pc-mingw32/include/errno.h:36:1: warning: this is the location of the previous definition
peer-io.c:43:1: warning: "EINTR" redefined
...
peer-io.c:45:1: warning: "EPIPE" redefined

Lines 349 to 355 should be deleted.

The redefinitions don't seem to be complete... EINPROGRESS missing, EPIPE from empirical evidence should be either WSAENOTCONN or (WSAENOTCONN | WSAECONNRESET) maybe more, of course they can't be combined like that.

This one should be the show stopper, Windows does close sockets and blocks new connections per design (the infamous tcpip.sys 'feature' which limits connections to as low as 10 in XP Home, a little more on XP Pro, and I don't know if it was changed on Vista and the 7 flavors of 7). The limit can be changed of course, but the point is the behavior, we are certain to see those WSANOTCONN, the app won't work if not handled right.

  CC     web.o
...
web.c: In function 'tr_select':
web.c:259: error: 'msec' undeclared (first use in this function)

Add the extra parameter? or just make it THREADFUNC_MAX_SLEEP_MSEC.

Also, I have a question -- does Windows have a policy on units to use for displaying disk capacity, vs memory size, vs bandwidth speeds, as Ubuntu does in <​https://wiki.ubuntu.com/UnitsPolicy>? For example, does Windows have a consistent use of KiB, or KB, or kB?

Windows doesn't use SI units at all.

I don't know if there is a published 'standard' by Microsoft, but GB in Windows are real (i.e. Computing Science, not hard-disk manufacturers crap) Gigabytes, which is the same as GiB.

On network stuff you also see Gbps and Mbps, I haven't seen any option to change those bits to bytes, but most applications use bytes KB/s or MB/s (I don't have GB speeds), so those also don't use SI units.

[Longinus00]

You can pretty much assume everyone besides ubuntu(10.10?) and osx(10.6) use the 1024 byte standard and they stick with SI prefixes.

[charles]

Replying to rb07:

I don't know if there is a published 'standard' by Microsoft, but GB in Windows are real (i.e. Computing Science, not hard-disk manufacturers crap) Gigabytes, which is the same as GiB.

That's what I'm talkin' about. Yay Microsoft!

Whoa, what am I saying?

shudder

anyway.

r10914: merge into trunk, jch's upstream win32 portability changes to third-party/dht/dht.c

[rb07]

Patch to r10915. Not tested, just compiled.

[charles]

r10917 /trunk/ (3 files in 2 dirs): (trunk libT) #3311 "MingW build of Transmission" -- added rb07's revisions to my revisions to his diff.

[charles]

I think we're getting really close here to having a first draft done. If nothing else, the diffs keep getting smaller :)

A couple of other random points:

• for consistency with the other Transmission apps, qtr has been renamed transmission-qt. However on Windows users probably have no idea what the Q stands for. Maybe it would be better to call it transmission-win?

• You might try out upx 3.05 with the commands "--lzma --best --ultra-brute" to see how far down the .exe file can be shrunk before you put it in the installer ;)

• Is the installer necessary? What files do we bundle along other than the .exe itself?

[rb07]

I was just looking at the change I made in web.c:259, and it was pretty silly, the actual change should be something like:

        tr_wait_msec( 1000 * t->tv_sec + t->tv_usec / 1000 );

Replying to charles:

I think we're getting really close here to having a first draft done. If nothing else, the diffs keep getting smaller :)

A couple of other random points:

• for consistency with the other Transmission apps, qtr has been renamed transmission-qt. However on Windows users probably have no idea what the Q stands for. Maybe it would be better to call it transmission-win?

I think it should be named the same in all platforms, so -win wouldn't be my choice.

• You might try out upx 3.05 with the commands "--lzma --best --ultra-brute" to see how far down the .exe file can be shrunk before you put it in the installer ;)

Been there, done that. You get an installer weighting a little less, the actual files (exe and dll) are about 40% lighter... I think, because the installer maker uses zip compression anyway, and I could change that option to use other compression method; not sure if its one that windows doesn't support natively you probably end up shipping gunzip.

• Is the installer necessary? What files do we bundle along other than the .exe itself?

That depends how I built it, currently the list of files include all the shared libraries, the copyright, and the web files:

$ ll
total 27M
drwx------+ 1 rberber None    0 Jun 25 14:09 ./
drwxr-xr-x  1 rberber None    0 Jun 30 19:08 ../
-rwx------+ 1 rberber None  18K Jun 26 20:29 COPYING*
-rwx------+ 1 rberber None 991K Jun  1 10:31 dbus-1.dll*
-rwx------+ 1 rberber None  24K Mar 28 16:18 mingwm10.dll*
-rwx------+ 1 rberber None 2.7M May 27 22:55 QtCore4.dll*
-rwx------+ 1 rberber None 561K May 27 22:55 QtDBus4.dll*
-rwx------+ 1 rberber None  11M May 27 22:57 QtGui4.dll*
-rwx------+ 1 rberber None 2.6M May 27 22:57 QtNetwork4.dll*
-rwx------+ 1 rberber None 429K May 27 23:01 QtXml4.dll*
-rwx------+ 1 rberber None 1.6K Jun 26 20:29 README*
-rwx------+ 1 rberber None 4.1M Jun 28 18:13 transmission-qt.exe*
drwx------+ 1 rberber None    0 Jun 17 19:24 web/

Before DBUS was added I had started building a static linked version, in that case only transmission-qt.exe, mingwm10.dll, the copyright files, and the web files were included in the installer. DBUS and Qt under Windows didn't allow me to keep linking everything static, its a mess from the Qt folks, I just didn't want to clean that up.

I'm not really happy with the installer, but it does simplify things for users. My gripe is that it doesn't clean unused old files, so at one point I ended with old and new libraries which caused problems... another Windows 'feature'. But that probably means I have to learn to use the installer maker better.

[charles]

That's a lot of good information. Thanks for the summary.

I wonder, would it be possible to build Qt statically as described at <​http://doc.trolltech.com/4.6/deployment-windows.html#building-qt-statically>?

The upside to statically linking transmission-qt against static Qt libraries:

1. The problem of cleaning old libraries goes away

2. The compiler/linker can strip out all the parts of Qt that Transmission doesn't use, making the final result much smaller. (I hope.)

3. Having a single static executable will make it easier for upx to do its magic. (Though, I don't know how much this matters after the installer's compression.)

[KyleK]

The patch for r10917 adds the compiler option "-Wvla" to configure.ac, which is only available in gcc 4.3.x (my version, 4.1.3, complained about an unknown option, and I checked the documentation on the gcc website, the parameter appears first in version 4.3.5).

[rb07]

Replying to KyleK:

The patch for r10917 adds the compiler option "-Wvla" to configure.ac, which is only available in gcc 4.3.x (my version, 4.1.3, complained about an unknown option, and I checked the documentation on the gcc website, the parameter appears first in version 4.3.5).

There is no patch for r10917 here, and none of the patches here add that option...

[rb07]

Replying to charles:

I wonder, would it be possible to build Qt statically as described at <​http://doc.trolltech.com/4.6/deployment-windows.html#building-qt-statically>?

Yes, and I do have Qt 4.6 built with only static libs, and that was used at least in one of the builds I made available. I also used upx on that one.

But as I said, with Transmission 2.00 and above DBUS appeared as a dependency of the Qt application, Qt for Windows doesn't include DBUS by default, I had to build it and it wasn't possible to build it with static libraries, the Qt build script is not expecting DBUS on Windows or is expecting one of the old 3rd party ports to Windows which now have been consolidated into the main DBUS code (well, only for the developer version, no 'stable' release).

I tried two ways: get rid of DBUS on the Qt application since DBUS is not really needed (lucky for me, it only complains that the daemon is not running), and the second way was building Qt with DBUS support.

[KyleK]

Replying to rb07:

Replying to KyleK:

The patch for r10917 adds the compiler option "-Wvla" to configure.ac, which is only available in gcc 4.3.x (my version, 4.1.3, complained about an unknown option, and I checked the documentation on the gcc website, the parameter appears first in version 4.3.5).

There is no patch for r10917 here, and none of the patches here add that option...

Sorry, it's r10913.

[rb07]

Replying to KyleK:

Sorry, it's r10913.

The second argument still stands: there is no change in the patches that adds the option you point out.

I can see that configure.ac has that option in line 88 (looking at r10915), but this ticket didn't add it.

[rb07]

I'm currently looking for the cause of some errors that are still appearing on peer-io.c :

• Code 122, EMSGSIZE but that's the stange part, Windows should set 10040 WSAEMSGSIZE, and the only part in Transmission that sets that, peer-msgs.c pulls a macro to replace it with the Windows' code. They appear at tr_peerIoTryWrite(). I'll have to dig into libevent's code.

• Same as above in tr_peerIoTryRead(). This ones seem to appear in pairs, one for write and then one for read, but with lower frequency.

• WSAENOTCONN in tr_peerIoTryWrite(). This probably is what I said about the EPIPE equivalents, perhaps there is not one equivalent, and this error code should be added.

• WSAECONNRESET, this could be the result of me stopping the torrent.

The first case is the more frequent (a quick count gives 37/2/28/2 for the above 4 errors), I'll try to find what is going on there.

The r10915 application seems to run about normal, but then it crashed when I was opening the properties panel on the torrent. Don't know what the crash was about, I was running a non-debug version, but those still log debug messages when I run them in gdb.

There were also other strange messages when I ran the same application on a remote session, I haven't seen these:

warning: QHttpNetworkConnectionChannel::_q_receiveReply() called without QHttpNetworkReply, 0 bytes on socket. 

and there was one about the cache, I can't find it in my log, maybe I saw it in an error panel.

[rb07]

A bug was reported to me (through PM on the forum):

My Transmission-Qt build for Windows can't handle non-English characters on path names.

Its not a bug on Transmission-Qt but on the build done with MingW.

Windows itself uses UTF8, I tried enabling UNICODE support on MingW just like Qt does: "CFLAGS += -DUNICODE" but the result was a mess, the application dies and all the debug messages appear unreadable, could be UTF8 (I can't see anything), it can't open files (like settings.json), looks like Mingw is trying to use all text strings as Unicode.

One more detail to investigate... or I could change compilers to Mingw-w64 which says it really supports Unicode.

[KyleK]

Replying to rb07:

Replying to KyleK:

Sorry, it's r10913.

The second argument still stands: there is no change in the patches that adds the option you point out.

I can see that configure.ac has that option in line 88 (looking at r10915), but this ticket didn't add it.

The revision clearly refers to this ticket in the commit message. The revision causes a regression, so I filed a regression report at the ticket the changes are attached to.

[rb07]

Quick notes on my comments #51 and #52 :

The error code 122 comes from Windows, according to the documentation ​http://msdn.microsoft.com/en-us/library/ms681382%28v=VS.85%29.aspx looks to be the same as WSAEMSGSIZE:

ERROR_INSUFFICIENT_BUFFER The data area passed to a system call is too small.

Haven't found what is causing it.

On the "path with foreign characters problem", I was completely wrong. Transmission-Qt is converting those paths to UTF8, which is what libtransmission gets as parameter.

The problem is Windows has 2 APIs: one for ASCII, one for UTF16 (what Microsoft calls 'wide characters'), so currently passing UTF8 is wrong but it works as long as the path has all ASCII characters.

The solution, taking into account only paths not full internationalization, is to change all relevant calls to the UTF16 variety and convert the parameters using either Transmission's ConvertUTF8toUTF16() or Windows' MultiByteToWideChar()... fixed parameters could use fixed strings (i.e. "wb+" to L"wb+" which is supported by the compiler).

I don't know if changing all those UTF8 conversion on the Qt application is a good option, it would save us from doing UTF8 to UTF16, but it could break things for Linux/MacOSX.

[charles]

rb07, is there still anything left to do on this ticket? It looks like your mingw builds that you post in the forums are going great.

[rb07]

Replying to charles:

rb07, is there still anything left to do on this ticket? It looks like your mingw builds that you post in the forums are going great.

No, sorry I should have closed it.

Yes, Transmission builds "out of the box" since the 2.04+ versions, the 2.1x applications are very stable judging from about 2 months use. I have been documenting this on the Wiki, one liners saying "no patch required" for the last versions.

There are reported issues on the forum, but those merit their own bug report and follow up (i.e. no easy or quick fix). Two come to mind: local verifying and foreign languages.

For the record, about local verifying I did check the code to see if we had missed a file opening not in binary mode, no such thing, so running under the debugger will be necessary.

The foreign language porting to Windows, since windows only has a UTF16 API, and we are not using it, is a bigger change. I started it, the low level changes are easy (I have them as a mix of macros and very simple code), the high level part seems to be already in place (the Qt code converts from whatever local to UTF8), the middle level is still missing, and its the most messy (I have some incomplete changes for directory iterators).