Re: [shell32tests/shelllink.c] Use aliases for ordinals (resend/rediff)
Paul Vriens wrote:
Hi,
to ease Alexandre's work I re-diffed the patch. The consensus on wine-devel was that this patch is fine.
This patch makes the crosstest compile work again for shell32. Result tested on win98/winxp/Wine/W2KProf.
Changelog: Use aliases for calls to ordinals.
I don't think that's what the consensus says, if a consensus there is in the first place. If I understand Filip Navara's patch correctly, it shows that the fix belongs in MinGW's shell32.def file. And if that's the case then I'm arguing this test should remain as is. As I understand it, once MinGW's file is fixed, the compiler sees that there is an ordinal associated to SHSimpleIDListFromPath and then generates an executable which imports SHSimpleIDListFromPath by ordinal. This seems to be exactly what Visual C++ does on Windows. It's perfectly acceptable to import SHSimpleIDListFromPath by name in programs compiled using Visual C++, and the resulting executable works even if shell32.dll exports this API by ordinal only. Our conformance tests should reflect that. That's because they are meant to reflect the Windows behavior (not the MinGW one), including where compilation and linking are concerned. -- Francois Gouget fgouget(a)codeweavers.com
On Wed, 2005-02-16 at 19:53, Francois Gouget wrote:
Paul Vriens wrote:
Changelog: Use aliases for calls to ordinals.
I don't think that's what the consensus says, if a consensus there is in the first place.
If I understand Filip Navara's patch correctly, it shows that the fix belongs in MinGW's shell32.def file. And if that's the case then I'm arguing this test should remain as is.
As I understand it, once MinGW's file is fixed, the compiler sees that there is an ordinal associated to SHSimpleIDListFromPath and then generates an executable which imports SHSimpleIDListFromPath by ordinal. This seems to be exactly what Visual C++ does on Windows.
It's perfectly acceptable to import SHSimpleIDListFromPath by name in programs compiled using Visual C++, and the resulting executable works even if shell32.dll exports this API by ordinal only. Our conformance tests should reflect that. That's because they are meant to reflect the Windows behavior (not the MinGW one), including where compilation and linking are concerned. That will leave us (for a unknown period) with a non-working winetest. And as Paul Millar stated that will be the case numerous times. So maybe we should use the patch and as soon as MingW is fixed put the calls to the names back? For now calling ordinals doesn't seem that bad as the compiled versions (as you've stated) will do the same.
I thought that the main purpose of conformance tests was to check the behavior on windows. If a windows program calls (somehow directly) SHSimpleIDListFromPath and friends, it will fail. The fact that the compiler 'translates' the call into a ordinal shouldn't matter then for conformance testing. Cheers, Paul.
Paul Vriens wrote: [...]
That will leave us (for a unknown period) with a non-working winetest.
The patch is available already. I attached it to this email so it's easy to find, credits go to Filip Navara. So winetest surely cannot remain broken for a long time.
And as Paul Millar stated that will be the case numerous times. So maybe we should use the patch and as soon as MingW is fixed put the calls to the names back? For now calling ordinals doesn't seem that bad as the compiled versions (as you've stated) will do the same.
I thought that the main purpose of conformance tests was to check the behavior on windows. If a windows program calls (somehow directly) SHSimpleIDListFromPath and friends, it will fail.
This just won't happen.
The fact that the compiler 'translates' the call into a ordinal shouldn't matter then for conformance testing.
It does. If a Windows application contains the following code: pidl=SHILCreateFromPath(pathW, &pidl, NULL); That program with compile, link and run just fine with Visual C++ on Windows. If you take those sources and try to compile them with WineLib then they should compile just fine too. In particular, if it fails du to the above line then it means there is a bug in Winelib and we want to know about that. As it stands the shelllink conformance test makes sure that we handle this correctly. If we modify it to use GetProcAddress() we lose this check. Modifying the test one way and back would be a lot of work especially when it is much simpler to just use the attached patch. Also it supposes that someone actually restores the test to its current state... who? when? Finally it's just not the Wine way. -- Francois Gouget fgouget(a)codeweavers.com Index: lib/shell32.def =================================================================== RCS file: /cvs/src/src/winsup/w32api/lib/shell32.def,v retrieving revision 1.7 diff -u -r1.7 shell32.def --- lib/shell32.def 1 Jan 2004 11:00:43 -0000 1.7 +++ lib/shell32.def 16 Feb 2005 12:30:28 -0000 @@ -32,6 +32,7 @@ SHUpdateRecycleBinIcon(a)0 SHSetUnreadMailCountW(a)12 SHSetLocalizedName(a)12 +SHSimpleIDListFromPath(a)4 @162 SHQueryRecycleBinW(a)8 SHQueryRecycleBinA(a)8 SHPathPrepareForWriteW(a)16 @@ -43,6 +44,7 @@ SHIsFileAvailableOffline(a)8 SHInvokePrinterCommandW(a)20 SHInvokePrinterCommandA(a)20 +SHILCreateFromPath(a)12 @28 SHHelpShortcuts_RunDLL(a)16 SHGetUnreadMailCountW(a)24 SHGetSpecialFolderPathW(a)16 @@ -172,11 +174,11 @@ ILCloneFirst(a)4 ILCombine(a)8 ILFindChild(a)8 -ILFree(a)4 +ILFree(a)4 @155 ILGetNext(a)4 ILGetSize(a)4 -ILIsEqual(a)8 +ILIsEqual(a)8 @21 ILIsParent(a)12 ILLoadFromStream(a)8 ILRemoveLastID(a)4
Ultimately, its about choice. Both methods have some merit. Currently the time-to-patch (ttp 2-3 days?) is less than the mean-time-between-breakage (mtbb ~14 days). This gives us the luxury of fixing MinGW as we find new holes in it. If these two time-scales become comparable, then we're in a realm where things are breaking as fast as we can patch them and we would be constantly battling to get a version of MinGW that can produce a winetest.exe. My *guess* is that on average, the mtbb will decrease, whilst ttp remains constant. That's what it feels like. However, we can wait and see what happens :^) On Wednesday 16 February 2005 12:35, Filip Navara wrote:
BTW, are there any (other) unofficial Wine patches for the MinGW W32API package?
Yes. Both Hans Leidekker and myself have a suite of patches; mine are unashamedly taken from Hans (many thanks!) and I don't think there is any difference between our patch-set. On Wednesday 16 February 2005 22:25, Francois Gouget wrote:
So winetest surely cannot remain broken for a long time.
No, in fact its now working again, now. Just doing an end-to-end test right now.
Finally it's just not the Wine way.
OK, fair point. But, there's two aspects: testing the MinGW code and the winetest.exe compilation service. Whilst ttp < mtbb - \delta, the service works and we can do it the Wine way (the \delta is so people retain their sanity ;) On Wednesday 16 February 2005 18:39, Alexandre Julliard wrote:
Actually no, fixing MinGW is a very desirable side-effect of cross-compiling our tests. If we find bugs in MinGW they should be fixed, not worked around.
OK, sure. I think it might be worth trying to push some of these patches up-stream again. That way, we are actually fixing MinGW. Paul. (as usual, just my 2c worth)
On Thursday 17 February 2005 02:06, Paul Millar wrote:
I think it might be worth trying to push some of these patches up-stream again. That way, we are actually fixing MinGW.
Been there. The issue is that MinGW has a patch acceptance policy that says that material should be documented by MS to be acceptable. The practice of reverse engineering would jeopardise the interests of some MinGW users I was told. Clearly, functions exported by ordinal only are often (mostly?) undocumented. This is the very reason I am maintaining these patches; to add just enough of the undocumented stuff to MinGW to make it usable for compiling Wine tests. Stefan Leichter already has been helping out recently, but we could use more helping hands of course. -Hans
Hi Hans, On Thursday 17 February 2005 09:59, Hans Leidekker wrote:
On Thursday 17 February 2005 02:06, Paul Millar wrote:
[...] push some of these patches up-stream
Been there. The issue is that MinGW has a patch acceptance policy that says that material should be documented by MS to be acceptable.
Naively, I was wondering if what Rolf said: On Wednesday 16 February 2005 13:31, Rolf Kalbermatter wrote:
Later after the court case they documented most of those formely undocumented APIs and also included them back into the next SDK release.
... would mean that the MinGW team would now accept at least some of the extra w32lib patches? In either case, I think it would be useful if a simple .tar.gz (and/or .zip) file containing all the patches were available from your website. The MinGW people can then link to this page with whatever disclaimer they feel necessary. Cheers, Paul.
On Thu, 2005-02-17 at 02:06, Paul Millar wrote:
No, in fact its now working again, now. Just doing an end-to-end test right now. Although we have a new winetest executable, it doesn't work on my win98 box. Starting the (automatically) downloaded winetest-200502171000-paul-mingw.exe gives errors that subtests of shell32 cannot be parsed. Executing the extracted shell32_test.exe starts with the (now well known) message:
The SHELL32_TEST.EXE file is linked to missing export SHELL32.DLL::ILFree I expect that win2k has the same problem (when looking at test.winehq.org). Any ideas? (besides using 'my' patch :-)). Cheers, Paul.
On Thursday 17 February 2005 16:30, Paul Vriens wrote:
I expect that win2k has the same problem (when looking at test.winehq.org).
Any ideas? (besides using 'my' patch :-)).
Filip's patch was not quite right, these functions need to be exported ordinal *only*, otherwise the resulting test binary won't link at runtime on systems older than xp. With the patch below I am able to produce binaries that run on xp as well as 2k and 98. -Hans
Hans Leidekker <hans(a)it.vu.nl> writes:
Filip's patch was not quite right, these functions need to be exported ordinal *only*, otherwise the resulting test binary won't link at runtime on systems older than xp. With the patch below I am able to produce binaries that run on xp as well as 2k and 98.
And actually the same thing should be done in the Wine spec file (in fact most of the shell32 functions should probably be marked noname). -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard wrote:
Hans Leidekker <hans(a)it.vu.nl> writes:
Filip's patch was not quite right, these functions need to be exported ordinal *only*, otherwise the resulting test binary won't link at runtime on systems older than xp. With the patch below I am able to produce binaries that run on xp as well as 2k and 98.
And actually the same thing should be done in the Wine spec file (in fact most of the shell32 functions should probably be marked noname).
Do I smell a janitorial project? -- Tony Lambregts
participants (6)
-
Alexandre Julliard -
Francois Gouget -
Hans Leidekker -
Paul Millar -
Paul Vriens -
Tony Lambregts