October 2014 - Wine-Devel - list.winehq.org

Re: [PATCH] jscript: Ignore BOM mark in next_token. (try 5)

by Nikolay Sivov

On 10/2/2014 14:48, Jacek Caban wrote: > On 10/02/14 08:29, Nikolay Sivov wrote: >>> static BOOL skip_spaces(parser_ctx_t *ctx) >>> { >>> - while(ctx->ptr < ctx->end && isspaceW(*ctx->ptr)) { >>> + while(ctx->ptr < ctx->end && (isspaceW(*ctx->ptr) || *ctx->ptr >>> == 0xFEFF /* UTF16 BOM */)) { >>> if(is_endline(*ctx->ptr++)) >>> ctx->nl = TRUE; >>> } >> This looks correct according to ECMA-252 section 7.2 - all of the >> following is a whitespace: >> >> - tab and vertical tab, 0x9 and 0xb; >> - form feed 0xc >> - space 0x20 >> - NBSP 0xa0 >> - UTF-16 BOM 0xfeff >> - any other Unicode "space separator" >> >> Hopefully isspaceW() covers everything but the BOM. What worries me is >> that isspaceW() itself is used in numerous places in code on its own. >> So probably we need more tests to cover more cases where space >> separators could be used, and later have our own is_space() call that >> will conform to the standard. > FWIW, ECMA-262 (which I usually use for jscript development) doesn't > mention UTF-16 as white space. Sorry, 252 was a typo of course. It does mention it here: http://www.ecma-international.org/ecma-262/5.1/#sec-7.2 > Anyway, I agree that it would be > interesting to see if it's considered white space in other places as > well. (I'm also fine with the patch in current form, but an extended > version would be obviously better). Sure, I'm not saying it's wrong either, just pointing out a potential direction for improvement. > > Jacek > >

Oct. 2, 2014

Re: Wine staging tree

by Sebastian Lackner

Hi Roderick, On 02.10.2014 06:59, Roderick Colenbrander wrote: > Hi Michael, > > Thanks for your long email about some sort of wine 'staging' project. I > understand what you are you trying to accomplish, but I have various > concerns, which I will share below. > > The initial motivation for this project seems to be to allow users to test > patches at an earlier stage. It may allow users to run their app sooner. > For developers it can give their code more testing, which I agree can be > useful. > > My main concern is that the quality of a lot of the patches is > unfortunately so-so, with various lose ends and side effects various > developers can't oversee. AJ is right to reject such patches. This can be a > hinder for a new developers, it is not always as easy to get feedback and > yes some mentoring for new developers can really help. As Michael already said in his previous mail, rejecting patches if they still contain flaws, is the right and the only way to do it. And of course having a more active mailing list would speed up the process of upstreaming stuff. Nevertheless, this has nothing to do with "new developers", we know _several_ examples of long-contributing developers (don't want to point at anyone...) who have the same impression, that just sending patches and getting it accepted _or_ useful feedback for improvements just doesn't work in most cases on wine-devel. At my opinion it mainly has to do with the interest of developers to get specific issues fixed. If there is not enough interest, then a lot of useful work might just not get enough attention, and is lost, even if it was for example actually done by a _payed_ GSoC student. Even if the implementation is _not used at all_ for the final version, it is still useful to see the ideas of other developers, or for example included testcases, to confirm tha t the "better" implementation is also correct. Developing is a process of improving a code over and over again, and there is no way to do that in an official repository. I personally like the current approach of shared repo, and it is not really that different from all the private developer repositories, which are also included in various Wine builds (wine-multimedia, wine-csmt, ...). > > Over the years there have been various projects in different forms like > PlayOnLinux to allow users to run Wine with non-upstreamed patches. As you > know the main concern of PlayOnLinux is that due to all the patching it is > not vanilla Wine anymore, but a 'tainted version'. This makes us typically > reject bug reports, because we don't know what code went in it, which can > waste a developers time. We are aware of that, and thats perfectly fine (more details below). > > No matter how good the intentions are to do a better job than PlayOnLinux, > I'm skeptical. I actually went over several of the patches in the repo and > various of them look incorrect. I understand that they may make app Z work > and while they may not always be as evil as PlayOnLinux-style hacks, I > still don't want them to be out in the public. In my opinion any tainted > Wine is bad for Wine developers. Yes, user may test your new patch, but I > expect to get a lot of issues back due to other incomplete patches with > side effects. "various look incorrect" <- Isn't that exactly what we were talking about? ;) If you spotted an error immediately while taking a short look, why don't you report it to us? If you mean one of the few patches, where we're not using an optimal solution yet, feel free to suggest a better one. Not sure which patches you were looking at, but most of them are correct, and might only need minor cleanup. We wouldn't add any patches where we are aware of, that it will have negative side affects for other applications. For most patches we also spend a lot of time discussing the possible effects before we come to a conclusion, if it should be added or not. > > In an ideal world there would be no need for your project, because all > patches make it upstream quickly. I think the discussion should be more on > how to get closer and closer to this situation. I think it should more be > found in mentoring and providing more feedback. Well, feel free to provide concrete suggestions if you have a better idea how to solve that. ;) > > Since Wine is an open source project, nobody forbids you from working on > your customized Wine. Providing builds to users to try it is fine with me, > but clearly don't mark it as the official Wine, but as you already doing by > calling it 'wine-compholio'. > > What does really scare me are the Fedora Wine builds. After you said that > they incorporate your changes, I checked the rpm and indeed it uses 150+ > custom patches from your repo. In my opinion, distro packages need to be as > vanilla as possible to prevent the PlayOnLinux-like bug rejection issues. > Sometimes build system change or some minor change is needed, but such an > amount of patches is not justified. Myself I don't have much time to work > on Wine anymore these days, but if I got bugs from Fedora users, I would > reject their bugs. I hope other Wine developers would really do the same, > because the packages clearly can't be trusted. Please note that it was never our idea to get these patches into regular distribution packages. In case of Fedora the packager contacted us, and even proceeded after both Michael and I told him about the possible consequences, and that we personally don't recommend to do that. I will not cite anything here, as this was discussed in private mail contact, but I think the Fedora guys are totally aware that each bug report by Fedora users can get rejected without a comment. Recently I also opened a bug report, since I would really prefer to have a clear statement somewhere, that people shouldn't report bugs to winehq, see: https://bugzilla.redhat.com/show_bug.cgi?id=1147271 > > Overall I know the intentions are good to allow users to test new code > sooner and allow developers to get their stuff test sooner. I'm really > skeptical about the benefit for developers due to the tainted Wine builds. > I think overall it may actually be more negative than positive for Wine. I don't think so. The question is just what is better: Having users reporting the same well-known issues over and over again (have to be identified as duplicates, also wastes time!), or probably a follow-up/regression issue which cannot be reproduced with vanilla Wine (bug reports can be easily closed, when error description doesn't match on developer machine). As you might know, the second problem already applies to a _huge_ amount of bugs, since users already use various (often unnecessary) winetricks recipes without knowing what they do, or report bugs when using PlayOnLinux or at least wine-multimedia (= _every_ Ubuntu user). Unless Wine reached a state, where it works out of the box with almost every app, there will always be a specific part of users with patched/modified versions - our approach is just a bit more combined and organized than some of the previous attempts, by reducing this to work-in-progress solutions. ;) If you want a solution to find out if a version contains our patches: Just ask the users on the bug tracker to run "wine --patches". > > Thanks, > Roderick > Regards, Sebastian

Oct. 2, 2014

Re: [1/2] kernel32/tests: Add tests for FindFirstFileExA with FIND_FIRST_EX_LARGE_FETCH flag.

by Sebastian Lackner

On 02.10.2014 12:29, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=9199 > > Your paranoid android. > > > === wvistau64 (32 bit file) === > file.c:367: Test failed: attributes of file "testfile.xxx" are 0x0820 > > === wvistau64 (64 bit file) === > file.c:367: Test failed: attributes of file "testfile.xxx" are 0x0820 > Not caused by my patch, also present when running the testbot with an empty dummy patch: https://newtestbot.winehq.org/JobDetails.pl?Key=9202

Oct. 2, 2014

Re: [PATCH] jscript: Ignore BOM mark in next_token. (try 5)

by Jacek Caban

On 10/02/14 08:29, Nikolay Sivov wrote: >> static BOOL skip_spaces(parser_ctx_t *ctx) >> { >> - while(ctx->ptr < ctx->end && isspaceW(*ctx->ptr)) { >> + while(ctx->ptr < ctx->end && (isspaceW(*ctx->ptr) || *ctx->ptr >> == 0xFEFF /* UTF16 BOM */)) { >> if(is_endline(*ctx->ptr++)) >> ctx->nl = TRUE; >> } > This looks correct according to ECMA-252 section 7.2 - all of the > following is a whitespace: > > - tab and vertical tab, 0x9 and 0xb; > - form feed 0xc > - space 0x20 > - NBSP 0xa0 > - UTF-16 BOM 0xfeff > - any other Unicode "space separator" > > Hopefully isspaceW() covers everything but the BOM. What worries me is > that isspaceW() itself is used in numerous places in code on its own. > So probably we need more tests to cover more cases where space > separators could be used, and later have our own is_space() call that > will conform to the standard. FWIW, ECMA-262 (which I usually use for jscript development) doesn't mention UTF-16 as white space. Anyway, I agree that it would be interesting to see if it's considered white space in other places as well. (I'm also fine with the patch in current form, but an extended version would be obviously better). Jacek

Oct. 2, 2014

Re: [1/2] kernel32/tests: Add tests for FindFirstFileExA with FIND_FIRST_EX_LARGE_FETCH flag.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=9199 Your paranoid android. === wvistau64 (32 bit file) === file.c:367: Test failed: attributes of file "testfile.xxx" are 0x0820 === wvistau64 (64 bit file) === file.c:367: Test failed: attributes of file "testfile.xxx" are 0x0820

Oct. 2, 2014

Re: Assorted spelling/grammar fixes

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=9197 Your paranoid android. === w8 (32 bit visual) === visual.c:2635: Test failed: unexpected colortable on offscreen surface

Oct. 2, 2014

Re: msvcrt: Make the first call to fwrite bufferred (try2)

by Andreas Fuchs

Hi Piotr, thanks for clearing this up for me... I was caught so off guard by the change in behavior that it did not occur to me that this was actually a correction... Sorry for the trouble... Cheers, Andreas Am Donnerstag, den 02.10.2014, 11:01 +0200 schrieb Piotr Caban: > Hi, > > On 10/02/14 10:28, Andreas Fuchs wrote: > > Hi Piotr, Hi all, > > > > I traced down this commit as the origin of the regression, where I cannot parse stdout of wine correctly anymore. > > See also https://bugs.winehq.org/show_bug.cgi?id=37345 > > > > I hope, sombody knowledgable (Piotr maybe you as the author) could help me figure this out or even provide a fix... > > > > Description: > > When calling wine from another process after redirecting stdout to a pipe, wine does not flush the stdout-buffer correctly anymore (Wine 1.6 did correctly). > > > > Attached is a usecase. To test do: > > i686-w64-mingw32-gcc helloworld.c > > gcc callwine.c > > ./a.out > > > > What will happen is: > > - on a buggy wine it will take 10 seconds and then all of the stdout-buffer is flushed on the exit of the child. (imagine the child would run longer than the 10 seconds) > > - on a good wine we will see some stdout-content then 10 second sleep and then the rest of the content. > this is not a bug. The pipes are supposed to be buffered on windows. > > The simple way of reproducing it is following: > i686-w64-mingw32-gcc helloworld.c > wine a.exe |more (linux) > a.exe |more (windows) > In both cases you will see all of the output after 10 seconds. > > Cheers, > Piotr > >

Oct. 2, 2014

Re: [PATCH 1/3] comctl32: Move TaskDialogIndirect function to separate file

by Nikolay Sivov

On 10/2/2014 13:10, Joachim Priesner wrote: >> On 10/1/2014 16:35, Joachim Priesner wrote: >>> +#include <stdarg.h> >>> +#include <string.h> >>> + >>> +#include "comctl32.h" >>> +#include "shlwapi.h" >>> + >>> +#include "windef.h" >>> +#include "winbase.h" >>> +#include "wingdi.h" >>> +#include "winternl.h" >>> +#include "dlgs.h" >>> +#include "wine/debug.h" >>> +#include "wine/unicode.h" >> It's unlikely you need all of those. > I'll need them for patch 2 of 3 though, and wanted to balance the size of the patches. Shall I move them to the later patch? Yes, I think it's better to add those step by step. > > That said, if I resend the patches: Do patches in a series have to retain all functionality when applied on their own? If not, I could make the patch smaller by not moving the old TaskDialogIndirect implementation to the new file, but creating a stub instead. Patch series should apply from first to last patch incrementally, meaning to apply patch n you have to apply patches 1..n-1 first (for n > 1 of course). You did it right - first patch just moves to a separate file, then next adds more and so on. I looked at patches 2 and 3, and got some more: > +HICON TASKDIALOG_GetIcon(HINSTANCE hInstance, PCWSTR pszIcon) Please avoid prefixed names like that, instead call it 'icon'. > + /* If hInstance is NULL and pszMainIcon is not one if the TD_*_ICON constants, the icon > + * is taken from imageres.dll. Until this is implemented, hard-code some of the offsets > + * for standard icons in imageres.dll. */ Is this documented somewhere? > + windowTitleBuffer = HeapAlloc(GetProcessHeap(), 0, (len + 1) * sizeof(WCHAR)); comctl32 code traditionally uses Alloc()/Free(). > + unsigned char hasButton = 0; This should be BOOL. > + static const WCHAR task_dialog_resource_nameW[] = { 'T','A','S','K','D','I','A','L','O','G',0 }; It's easier to define a resource id for that. > + if (contentTextBuffer) > + HeapFree(GetProcessHeap(), 0, contentTextBuffer); > + if (mainInstructionBuffer) > + HeapFree(GetProcessHeap(), 0, mainInstructionBuffer); You don't have to check for nulls. > >>> +#define TD_WARNING_ICON MAKEINTRESOURCEW(-1) >>> +#define TD_ERROR_ICON MAKEINTRESOURCEW(-2) >>> +#define TD_INFORMATION_ICON MAKEINTRESOURCEW(-3) >>> +#define TD_SHIELD_ICON MAKEINTRESOURCEW(-4) >> Where this comes from? I mean particular id values. > Those are the definitions from Windows' CommCtrl.h. Is it okay to use these directly or do I have to write a test program that prints the value of these constants (and get the same results)? Oh, sorry. I missed that you add those to a public header. It's fine to use them of course. > > P.S. On a general note I forgot to add: This patch is heavily based on dlls/user32/msgbox.c. That's not a problem.

Oct. 2, 2014

Re: [PATCH 1/3] comctl32: Move TaskDialogIndirect function to separate file

by Joachim Priesner

> On 10/1/2014 16:35, Joachim Priesner wrote: > > +#include <stdarg.h> > > +#include <string.h> > > + > > +#include "comctl32.h" > > +#include "shlwapi.h" > > + > > +#include "windef.h" > > +#include "winbase.h" > > +#include "wingdi.h" > > +#include "winternl.h" > > +#include "dlgs.h" > > +#include "wine/debug.h" > > +#include "wine/unicode.h" > It's unlikely you need all of those. I'll need them for patch 2 of 3 though, and wanted to balance the size of the patches. Shall I move them to the later patch? That said, if I resend the patches: Do patches in a series have to retain all functionality when applied on their own? If not, I could make the patch smaller by not moving the old TaskDialogIndirect implementation to the new file, but creating a stub instead. > > +#define TD_WARNING_ICON MAKEINTRESOURCEW(-1) > > +#define TD_ERROR_ICON MAKEINTRESOURCEW(-2) > > +#define TD_INFORMATION_ICON MAKEINTRESOURCEW(-3) > > +#define TD_SHIELD_ICON MAKEINTRESOURCEW(-4) > Where this comes from? I mean particular id values. Those are the definitions from Windows' CommCtrl.h. Is it okay to use these directly or do I have to write a test program that prints the value of these constants (and get the same results)? P.S. On a general note I forgot to add: This patch is heavily based on dlls/user32/msgbox.c.

Oct. 2, 2014

Re: msvcrt: Make the first call to fwrite bufferred (try2)

by Piotr Caban

Hi, On 10/02/14 10:28, Andreas Fuchs wrote: > Hi Piotr, Hi all, > > I traced down this commit as the origin of the regression, where I cannot parse stdout of wine correctly anymore. > See also https://bugs.winehq.org/show_bug.cgi?id=37345 > > I hope, sombody knowledgable (Piotr maybe you as the author) could help me figure this out or even provide a fix... > > Description: > When calling wine from another process after redirecting stdout to a pipe, wine does not flush the stdout-buffer correctly anymore (Wine 1.6 did correctly). > > Attached is a usecase. To test do: > i686-w64-mingw32-gcc helloworld.c > gcc callwine.c > ./a.out > > What will happen is: > - on a buggy wine it will take 10 seconds and then all of the stdout-buffer is flushed on the exit of the child. (imagine the child would run longer than the 10 seconds) > - on a good wine we will see some stdout-content then 10 second sleep and then the rest of the content. this is not a bug. The pipes are supposed to be buffered on windows. The simple way of reproducing it is following: i686-w64-mingw32-gcc helloworld.c wine a.exe |more (linux) a.exe |more (windows) In both cases you will see all of the output after 10 seconds. Cheers, Piotr

Oct. 2, 2014