Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#1797 closed defect (fixed)

Restore support for building Tor in non-Unicode Windows installations

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy, windows
Cc: mingw-san Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When we merged WinCE support in 8d31141ccbdbeee9589d04ea99819af7aa35193b, we reportedly broke Win98 (!?) by requiring the Unicode versions of functions that previously had used the ascii variants.

The real fix here is to use the generic version of each WinAPI function, plus appropriate macros to make it build either with the UNICODE preprocessor macro defined or undefined.

In other words, where previously we said in our Windows code

    void func(const char *filename) {
        HANDLE library = LoadLibrary("filename.dll");
        CreateFile(filename, ...)
    }

and now since 8d31141 we say

    void func(const char *filename) {
        HANDLE library = LoadLibraryW(L"filename.dll");
        WCHAR wfilename[MAX_PATH]= {0};
        mbstowcs(wfilename,filename,MAX_PATH);
        CreateFileW(wfilename, ...)
    }

we should instead say

    void func(const char *filename) {
        HANDLE library = LoadLibrary(TEXT("filename.dll"));
#ifdef UNICODE
        WCHAR tfilename[MAX_PATH]= {0};
        mbstowcs(tfilename,filename,MAX_PATH);
#else
        const char *tfilename = filename;
#endif
        CreateFile(tfilename, ...)
    }

Thanks to mingw-san for pointing this out on bug #1715.

Child Tickets

Attachments (1)

0001-Make-the-windows-build-succeed-with-or-without-DUNIC.patch (21.6 KB) - added by nickm 9 years ago.
Make the windows build succeed with or without -DUNICODE enabled

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final
Status: newneeds_review

See branch win_unicode_fixes in my public repository.

comment:2 Changed 9 years ago by Sebastian

I looked at it carefully and it appears to be ok. I don't have old windows or wince to test, though, so we should definitely ask someone with those platforms to test for us.

comment:3 Changed 9 years ago by Sebastian

eh, just to make that clear. I did try on windows 7 in mingw and it worked without a problem.

comment:4 Changed 9 years ago by arma

Triage: seems like it'd be good to get into 0.2.2.x, since it's a bug that was introduced during 0.2.2.x.

Who is the right person to review it?

comment:5 Changed 9 years ago by nickm

Cc: mingw-san added

Adding mingw-san to the Cc list, since it was mingw-san that reported this problem in the first place.

I'll email Valerio about it and see if he can take a look to see if it breaks WinCE, and if so, how.

Absent review from either of them, I say we just merge it into the next alpha.

Changed 9 years ago by nickm

Make the windows build succeed with or without -DUNICODE enabled

comment:6 Changed 9 years ago by mingw-san

Tor (c0c7868) builds with mingw and works for Win98 just fine as before WinCE-patch.

Not sure if WinCE could like non-unicode funcs as CreateFile() etc however. Need to wait WinCE's people.

comment:7 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay, Valerio just confirmed that this looks okay to him. Merging. Thanks, all!

comment:8 Changed 9 years ago by Sebastian

Priority: normalmajor
Resolution: fixed
Status: closedreopened

So this turned into build breakage on Windows XP somehow. I wonder if we got mixed up with branches or something, because I'm not sure how this could have worked before. I have a branch more_win_fixes that attempts to fix the issues for helix' winXP build at least, someone should review that carefully for wince/win9x though.

We should probably merge this soon and tag 0.2.2.17-alpha, so that helix doesn't have to make new packages for non-windows before the rc packages.

comment:9 Changed 9 years ago by nickm

Resolution: fixed
Status: reopenedclosed

The patch looks good; this does indeed look like a branches mixup.

Merging it, re-closing.

We should get hudson-on-windows up soon.

comment:10 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.