[PATCH] msvcrt: Fix fscanf return when EOF is immediately after an end of line.

[Erich E. Hoover]

Spotted while compiling VTK for Python under Wine.

[Piotr Caban]

Hi Erich,

The implementation part of the patch doesn't look right. I think that it 
should be fixed by %s format reporting error when there's no characters 
to read.

I'm attaching a diff (generated on top of your patch) that shows the 
problems.

Also please use only tests on strings (sscanf) or handle file creation 
failure.

Thanks,
Piotr

[Marvin]

Hi,

While running your changed tests, 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=57904

Your paranoid android.

=== build (build log) ===

error: patch failed: dlls/msvcrt/tests/scanf.c:27
Task: Patch failed to apply

=== debian10 (build log) ===

error: patch failed: dlls/msvcrt/tests/scanf.c:27
Task: Patch failed to apply

=== debian10 (build log) ===

error: patch failed: dlls/msvcrt/tests/scanf.c:27
Task: Patch failed to apply

[Erich E. Hoover]

Thanks Piotr!

I'm happy to change that, I was just using the fprintf tests as a
template.  Does the attached look better to you?

Best,
Erich

On Thu, Oct 17, 2019 at 5:23 AM Piotr Caban  wrote:
> Hi Erich,
>
> The implementation part of the patch doesn't look right. I think that it
> should be fixed by %s format reporting error when there's no characters
> to read.
>
> I'm attaching a diff (generated on top of your patch) that shows the
> problems.
>
> Also please use only tests on strings (sscanf) or handle file creation
> failure.
>
> Thanks,
> Piotr

[Piotr Caban]

Hi Erich,

On 10/17/19 5:41 PM, Erich E. Hoover wrote:
> -
>                /* if we have reached the EOF and output nothing then report EOF */
> -
>                if (nch==_EOF_  && rd==0 && st==0) {

_UNLOCK_FILE_(file);
> -
>                    return _EOF_RET;
> -
>                }

Please also update the widecharstring case.
> - fp = fopen(file_name, "wb");
> - ok(fp, "fp = %p\n", fp);

This cause compilation warning.

Thanks,
Piotr

[Erich E. Hoover]

Hi Piotr,

I just ran the widecharstring case on the testbot (
https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to
return 0 instead of WEOF...  Are you alright with just adding this
test to demonstrate that it doesn't work the same way?  Failing test:
===
    buffer[0] = ' ';
    buffer[1] = '\t';
    buffer[2] = '\n';
    buffer[3] = '\n';
    buffer[4] = 0;
    ret = swscanf(buffer, formats, results);
    ok( ret == (short)WEOF, "ret = %d\n", ret );
===

Best,
Erich

On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban  wrote:
> Hi Erich,
>
> On 10/17/19 5:41 PM, Erich E. Hoover wrote:
> > -
> >                /* if we have reached the EOF and output nothing then report EOF */
> > -
> >                if (nch==_EOF_  && rd==0 && st==0) {
>
> _UNLOCK_FILE_(file);
> > -
> >                    return _EOF_RET;
> > -
> >                }
>
> Please also update the widecharstring case.
> > - fp = fopen(file_name, "wb");
> > - ok(fp, "fp = %p\n", fp);
>
> This cause compilation warning.
>
> Thanks,
> Piotr

[Piotr Caban]

Hi Erich,

I've done some testing and here's what I found:
  - swscanf in msvcrt returns 0 as you have described
  - swscanf in msvcr90 returns WEOF
  - __stdio_common_vswscanf in ucrtbase returns WEOF

It looks like there's a bug/backward compatibility code in native 
msvcrt.dll. It would be good to do some more testing on different 
*wscanf functions in msvcrt.

I guess that the way to go is to:
  - add swscanf tests in msvcrt (you can use L"" there)
  - add swscanf tests in one of the newer dlls
  - make the implementation change specific to newer versions of C-runtime

Hope it helps,
Piotr

On 10/18/19 5:40 PM, Erich E. Hoover wrote:
> Hi Piotr,
>
> I just ran the widecharstring case on the testbot (
> https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to
> return 0 instead of WEOF...  Are you alright with just adding this
> test to demonstrate that it doesn't work the same way?  Failing test:
> ===
>      buffer[0] = ' ';
>      buffer[1] = '\t';
>      buffer[2] = '\n';
>      buffer[3] = '\n';
>      buffer[4] = 0;
>      ret = swscanf(buffer, formats, results);
>      ok( ret == (short)WEOF, "ret = %d\n", ret );
> ===
>
> Best,
> Erich
>
> On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban  wrote:
> > Hi Erich,
> >
> > On 10/17/19 5:41 PM, Erich E. Hoover wrote:
> > > -
> > >                /* if we have reached the EOF and output nothing then report EOF */
> > > -
> > >                if (nch==_EOF_  && rd==0 && st==0) {
> >
> > _UNLOCK_FILE_(file);
> > > -
> > >                    return _EOF_RET;
> > > -
> > >                }
> >
> > Please also update the widecharstring case.
> > > - fp = fopen(file_name, "wb");
> > > - ok(fp, "fp = %p\n", fp);
> >
> > This cause compilation warning.
> >
> > Thanks,
> > Piotr

[Erich E. Hoover]

Wow, thanks Piotr!

Are you sure you were testing against msvcr90?  I just ran a couple
tries against the testbot and I see it doing what we expect on
msvcr120, but msvcr90 it's giving 0 (
https://testbot.winehq.org/JobDetails.pl?Key=57997 ).  I'll go through
and establish exactly what version they fixed this in, but I want to
make sure that there isn't something weird going on with msvcr90.

Best,
Erich

On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban  wrote:
> Hi Erich,
>
> I've done some testing and here's what I found:
> - swscanf in msvcrt returns 0 as you have described
> - swscanf in msvcr90 returns WEOF
> - __stdio_common_vswscanf in ucrtbase returns WEOF
>
> It looks like there's a bug/backward compatibility code in native
> msvcrt.dll. It would be good to do some more testing on different
> *wscanf functions in msvcrt.
>
> I guess that the way to go is to:
> - add swscanf tests in msvcrt (you can use L"" there)
> - add swscanf tests in one of the newer dlls
> - make the implementation change specific to newer versions of C-runtime
>
> Hope it helps,
> Piotr
>
> On 10/18/19 5:40 PM, Erich E. Hoover wrote:
> > Hi Piotr,
> >
> > I just ran the widecharstring case on the testbot (
> > https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to
> > return 0 instead of WEOF...  Are you alright with just adding this
> > test to demonstrate that it doesn't work the same way?  Failing test:
> > ===
> >      buffer[0] = ' ';
> >      buffer[1] = '\t';
> >      buffer[2] = '\n';
> >      buffer[3] = '\n';
> >      buffer[4] = 0;
> >      ret = swscanf(buffer, formats, results);
> >      ok( ret == (short)WEOF, "ret = %d\n", ret );
> > ===
> >
> > Best,
> > Erich
> >
> > On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban  wrote:
> > > Hi Erich,
> > >
> > > On 10/17/19 5:41 PM, Erich E. Hoover wrote:
> > > > -
> > > >                /* if we have reached the EOF and output nothing then report EOF */
> > > > -
> > > >                if (nch==_EOF_  && rd==0 && st==0) {
> > >
> > > _UNLOCK_FILE_(file);
> > > > -
> > > >                    return _EOF_RET;
> > > > -
> > > >                }
> > >
> > > Please also update the widecharstring case.
> > > > - fp = fopen(file_name, "wb");
> > > > - ok(fp, "fp = %p\n", fp);
> > >
> > > This cause compilation warning.
> > >
> > > Thanks,
> > > Piotr

[Piotr Caban]

Hi,

The test is not failing on msvcr120 only because it’s skipped on all machines.

The tests are linked agains msvcrt.dll, you need to load the function dynamically.

Thanks,
Piotr
> On 18 Oct 2019, at 21:15, Erich E. Hoover  wrote:
>
> Wow, thanks Piotr!
>
> Are you sure you were testing against msvcr90?  I just ran a couple
> tries against the testbot and I see it doing what we expect on
> msvcr120, but msvcr90 it's giving 0 (
> https://testbot.winehq.org/JobDetails.pl?Key=57997 ).  I'll go through
> and establish exactly what version they fixed this in, but I want to
> make sure that there isn't something weird going on with msvcr90.
>
> Best,
> Erich
>
> On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban  wrote:
> > Hi Erich,
> >
> > I've done some testing and here's what I found:
> > - swscanf in msvcrt returns 0 as you have described
> > - swscanf in msvcr90 returns WEOF
> > - __stdio_common_vswscanf in ucrtbase returns WEOF
> >
> > It looks like there's a bug/backward compatibility code in native
> > msvcrt.dll. It would be good to do some more testing on different
> > *wscanf functions in msvcrt.
> >
> > I guess that the way to go is to:
> > - add swscanf tests in msvcrt (you can use L"" there)
> > - add swscanf tests in one of the newer dlls
> > - make the implementation change specific to newer versions of C-runtime
> >
> > Hope it helps,
> > Piotr
> >
> > On 10/18/19 5:40 PM, Erich E. Hoover wrote:
> > > Hi Piotr,
> > >
> > > I just ran the widecharstring case on the testbot (
> > > https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to
> > > return 0 instead of WEOF...  Are you alright with just adding this
> > > test to demonstrate that it doesn't work the same way?  Failing test:
> > > ===
> > >     buffer[0] = ' ';
> > >     buffer[1] = '\t';
> > >     buffer[2] = '\n';
> > >     buffer[3] = '\n';
> > >     buffer[4] = 0;
> > >     ret = swscanf(buffer, formats, results);
> > >     ok( ret == (short)WEOF, "ret = %d\n", ret );
> > > ===
> > >
> > > Best,
> > > Erich
> > >
> > > On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban  wrote:
> > > > Hi Erich,
> > > >
> > > > On 10/17/19 5:41 PM, Erich E. Hoover wrote:
> > > > > -
> > > > >                /* if we have reached the EOF and output nothing then report EOF */
> > > > > -
> > > > >                if (nch==_EOF_  && rd==0 && st==0) {
> > > >
> > > > _UNLOCK_FILE_(file);
> > > > > -
> > > > >                    return _EOF_RET;
> > > > > -
> > > > >                }
> > > >
> > > > Please also update the widecharstring case.
> > > > > - fp = fopen(file_name, "wb");
> > > > > - ok(fp, "fp = %p\n", fp);
> > > >
> > > > This cause compilation warning.
> > > >
> > > > Thanks,
> > > > Piotr

[Erich E. Hoover]

Ah, well - glad I asked :)  How much specificity do you want in
figuring out where this got fixed? (since there are no tests before
msvcr90)

Best,
Erich

On Fri, Oct 18, 2019 at 1:22 PM Piotr Caban  wrote:
> Hi,
>
> The test is not failing on msvcr120 only because it’s skipped on all machines.
>
> The tests are linked agains msvcrt.dll, you need to load the function dynamically.
>
> Thanks,
> Piotr
> > On 18 Oct 2019, at 21:15, Erich E. Hoover  wrote:
> >
> > Wow, thanks Piotr!
> >
> > Are you sure you were testing against msvcr90?  I just ran a couple
> > tries against the testbot and I see it doing what we expect on
> > msvcr120, but msvcr90 it's giving 0 (
> > https://testbot.winehq.org/JobDetails.pl?Key=57997 ).  I'll go through
> > and establish exactly what version they fixed this in, but I want to
> > make sure that there isn't something weird going on with msvcr90.
> >
> > Best,
> > Erich
> >
> > On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban  wrote:
> > > Hi Erich,
> > >
> > > I've done some testing and here's what I found:
> > > - swscanf in msvcrt returns 0 as you have described
> > > - swscanf in msvcr90 returns WEOF
> > > - __stdio_common_vswscanf in ucrtbase returns WEOF
> > >
> > > It looks like there's a bug/backward compatibility code in native
> > > msvcrt.dll. It would be good to do some more testing on different
> > > *wscanf functions in msvcrt.
> > >
> > > I guess that the way to go is to:
> > > - add swscanf tests in msvcrt (you can use L"" there)
> > > - add swscanf tests in one of the newer dlls
> > > - make the implementation change specific to newer versions of C-runtime
> > >
> > > Hope it helps,
> > > Piotr
> > >
> > > On 10/18/19 5:40 PM, Erich E. Hoover wrote:
> > > > Hi Piotr,
> > > >
> > > > I just ran the widecharstring case on the testbot (
> > > > https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to
> > > > return 0 instead of WEOF...  Are you alright with just adding this
> > > > test to demonstrate that it doesn't work the same way?  Failing test:
> > > > ===
> > > >     buffer[0] = ' ';
> > > >     buffer[1] = '\t';
> > > >     buffer[2] = '\n';
> > > >     buffer[3] = '\n';
> > > >     buffer[4] = 0;
> > > >     ret = swscanf(buffer, formats, results);
> > > >     ok( ret == (short)WEOF, "ret = %d\n", ret );
> > > > ===
> > > >
> > > > Best,
> > > > Erich
> > > >
> > > > On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban  wrote:
> > > > > Hi Erich,
> > > > >
> > > > > On 10/17/19 5:41 PM, Erich E. Hoover wrote:
> > > > > > -
> > > > > >                /* if we have reached the EOF and output nothing then report EOF */
> > > > > > -
> > > > > >                if (nch==_EOF_  && rd==0 && st==0) {
> > > > >
> > > > > _UNLOCK_FILE_(file);
> > > > > > -
> > > > > >                    return _EOF_RET;
> > > > > > -
> > > > > >                }
> > > > >
> > > > > Please also update the widecharstring case.
> > > > > > - fp = fopen(file_name, "wb");
> > > > > > - ok(fp, "fp = %p\n", fp);
> > > > >
> > > > > This cause compilation warning.
> > > > >
> > > > > Thanks,
> > > > > Piotr

[Piotr Caban]

Hi Erich,

msvcr80 is the first dll that was changed. Attaching a hacky test that 
shows it (it also shows difference is %S format handling but it's not 
something that has to be fixed as part of your patch).

Thanks,
Piotr

On 10/18/19 9:30 PM, Erich E. Hoover wrote:
> Ah, well - glad I asked :)  How much specificity do you want in
> figuring out where this got fixed? (since there are no tests before
> msvcr90)
>
> Best,
> Erich
>
> On Fri, Oct 18, 2019 at 1:22 PM Piotr Caban  wrote:
> > Hi,
> >
> > The test is not failing on msvcr120 only because it’s skipped on all machines.
> >
> > The tests are linked agains msvcrt.dll, you need to load the function dynamically.
> >
> > Thanks,
> > Piotr
> > > On 18 Oct 2019, at 21:15, Erich E. Hoover  wrote:
> > >
> > > Wow, thanks Piotr!
> > >
> > > Are you sure you were testing against msvcr90?  I just ran a couple
> > > tries against the testbot and I see it doing what we expect on
> > > msvcr120, but msvcr90 it's giving 0 (
> > > https://testbot.winehq.org/JobDetails.pl?Key=57997 ).  I'll go through
> > > and establish exactly what version they fixed this in, but I want to
> > > make sure that there isn't something weird going on with msvcr90.
> > >
> > > Best,
> > > Erich
> > >
> > > On Fri, Oct 18, 2019 at 12:45 PM Piotr Caban  wrote:
> > > > Hi Erich,
> > > >
> > > > I've done some testing and here's what I found:
> > > > - swscanf in msvcrt returns 0 as you have described
> > > > - swscanf in msvcr90 returns WEOF
> > > > - __stdio_common_vswscanf in ucrtbase returns WEOF
> > > >
> > > > It looks like there's a bug/backward compatibility code in native
> > > > msvcrt.dll. It would be good to do some more testing on different
> > > > *wscanf functions in msvcrt.
> > > >
> > > > I guess that the way to go is to:
> > > > - add swscanf tests in msvcrt (you can use L"" there)
> > > > - add swscanf tests in one of the newer dlls
> > > > - make the implementation change specific to newer versions of C-runtime
> > > >
> > > > Hope it helps,
> > > > Piotr
> > > >
> > > > On 10/18/19 5:40 PM, Erich E. Hoover wrote:
> > > > > Hi Piotr,
> > > > >
> > > > > I just ran the widecharstring case on the testbot (
> > > > > https://testbot.winehq.org/JobDetails.pl?Key=57989 ) and that seems to
> > > > > return 0 instead of WEOF...  Are you alright with just adding this
> > > > > test to demonstrate that it doesn't work the same way?  Failing test:
> > > > > ===
> > > > >      buffer[0] = ' ';
> > > > >      buffer[1] = '\t';
> > > > >      buffer[2] = '\n';
> > > > >      buffer[3] = '\n';
> > > > >      buffer[4] = 0;
> > > > >      ret = swscanf(buffer, formats, results);
> > > > >      ok( ret == (short)WEOF, "ret = %d\n", ret );
> > > > > ===
> > > > >
> > > > > Best,
> > > > > Erich
> > > > >
> > > > > On Fri, Oct 18, 2019 at 3:49 AM Piotr Caban  wrote:
> > > > > > Hi Erich,
> > > > > >
> > > > > > On 10/17/19 5:41 PM, Erich E. Hoover wrote:
> > > > > > > +                    /* if we have reached the EOF and output nothing then report EOF */
> > > > > > > +                    if (nch==_EOF_  && rd==0 && st==0) {
> > > > > > _UNLOCK_FILE_(file);
> > > > > > > +                        return _EOF_RET;
> > > > > > > +                    }
> > > > > >
> > > > > > Please also update the widecharstring case.
> > > > > >
> > > > > > > +    fp = fopen(file_name, "wb");
> > > > > > > +    ok(fp, "fp = %p\n", fp);
> > > > > > This cause compilation warning.
> > > > > >
> > > > > > Thanks,
> > > > > > Piotr

[Marvin]

Hi,

While running your changed tests, 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=58117

Your paranoid android.

=== w2008s64 (32 bit report) ===

Report errors:
The report seems to have been truncated

=== w2008s64 (64 bit report) ===

Report errors:
The report seems to have been truncated

[Francois Gouget]

On Mon, 21 Oct 2019, Marvin wrote:
> Hi,
>
> While running your changed tests, 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=58117
>
> Your paranoid android.
>
> === w2008s64 (32 bit report) ===
>
> Report errors:
> The report seems to have been truncated
>
> === w2008s64 (64 bit report) ===
>
> Report errors:
> The report seems to have been truncated

The reason for these errors is that Windows fails to start the 
executable because it cannot find microsoft.vc80.crt.

When starting the patched executable by hand I get the following error 
message (on stderr which is unfortunately not captured):

The application has failed to start because its side-by-side 
   configuration is incorrect. Please see the application event log for 
   more detail.

Looking in the event log I see:

Activation context generation failed for "C:\Users\Administrator\Documents\msvcr90_test.exe". Dependent Assembly 
   microsoft.vc80.crt,language="*",processorArchitecture="*",publicKeyToken="1fc8b3b9a1e18e3b",type="win32",version="8.0.50727.4053" 
   could not be found. Please use sxstrace.exe for detailed diagnosis.

[Erich E. Hoover]

On Mon, Oct 21, 2019 at 10:29 AM Francois Gouget
 wrote:
> On Mon, 21 Oct 2019, Marvin wrote:
> > ...
> > https://testbot.winehq.org/JobDetails.pl?Key=58117
>
> The reason for these errors is that Windows fails to start the
> executable because it cannot find microsoft.vc80.crt.
> ...

So, what you're saying is that the reason we don't have tests before
msvcr90 is that not all the testbots can run them.  That makes sense,
thanks!

Best,
Erich

[Piotr Caban]

On 10/21/19 7:17 PM, Erich E. Hoover wrote:
> On Mon, Oct 21, 2019 at 10:29 AM Francois Gouget
>  wrote:
> > On Mon, 21 Oct 2019, Marvin wrote:
> > > ...
> > > https://testbot.winehq.org/JobDetails.pl?Key=58117
> >
> > The reason for these errors is that Windows fails to start the
> > executable because it cannot find microsoft.vc80.crt.
> > ...
>
> So, what you're saying is that the reason we don't have tests before
> msvcr90 is that not all the testbots can run them.  That makes sense,
> thanks!

It was only an explanation why the tests didn't run. If it's added 
properly the tests will be skipped (based on tested dll name).

There's no reason to not add tests for msvcr80. On the other hand it's 
probably only useful to document differences between versions.

Thanks,
Piotr

[Francois Gouget]

On Mon, 21 Oct 2019, Erich E. Hoover wrote:
> On Mon, Oct 21, 2019 at 10:29 AM Francois Gouget
>  wrote:
> > On Mon, 21 Oct 2019, Marvin wrote:
> > > ...
> > > https://testbot.winehq.org/JobDetails.pl?Key=58117
> >
> > The reason for these errors is that Windows fails to start the
> > executable because it cannot find microsoft.vc80.crt.
> > ...
>
> So, what you're saying is that the reason we don't have tests before
> msvcr90 is that not all the testbots can run them.  That makes sense,
> thanks!

Yes, it's possible some VMs are missing some dlls.

Question:
  Do we want some test configurations where these optional components 
  are not installed?

In this case the description for w2008s64 says:

Upgraded with Internet Explorer 9, .Net Framework 3.5 SP1 & 4.5.1 and 
  all updates up to 2013/04/15. Added Visual C++ 2012 & 2013 runtimes.

Whereas the one for w2003std says:

Installed Internet Explorer 8, SP2 and all updates up to 2019/07/23. 
  Added .Net 2.0 SP1, 3.0 SP2, 3.5 SP2 & 4, msxml4 SP3, Visual C++ 2005 
  SP1, 2008 SP1 & 2010 SP1 runtimes.

msvcr80 is part of Visual C++ 2005 so it should be present on w2003std.

For w2008s64 I think I determined that the Visual C++ 2005 runtime was 
already installed and thus did not manually install it. In 
c:\windows\winsxs I do see the following files:

01/19/2008  05:11 AM    <DIR>          amd64_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.1434_none_88de292b2fb06019
04/08/2013  06:58 AM    <DIR>          amd64_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.3053_none_88e044e32fae7230
04/08/2013  07:50 AM    <DIR>          amd64_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.4016_none_88dc01492fb256de
04/08/2013  07:03 AM    <DIR>          amd64_microsoft.vc90.crt_1fc8b3b9a1e18e3b_9.0.21022.8_none_750b37ff97f4f68b
01/19/2008  05:11 AM    <DIR>          x86_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.1434_none_d08b6002442c891f
04/08/2013  06:58 AM    <DIR>          x86_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.3053_none_d08d7bba442a9b36
04/08/2013  07:50 AM    <DIR>          x86_microsoft.vc80.crt_1fc8b3b9a1e18e3b_8.0.50727.4016_none_d0893820442e7fe4
04/08/2013  07:02 AM    <DIR>          x86_microsoft.vc90.crt_1fc8b3b9a1e18e3b_9.0.21022.8_none_bcb86ed6ac711f91

But they are all slightly older than the version required by the patch 
(8.0.50727.4053).

[Piotr Caban]

On 10/21/19 8:04 PM, Francois Gouget wrote:
> Question:
>    Do we want some test configurations where these optional components
>    are not installed?

I don't think there's much value in having machines without these dlls. 
If it's not too much work it's probably worth adding to more machines.

Thanks,
Piotr

[Francois Gouget]

On Mon, 21 Oct 2019, Francois Gouget wrote:
[...]
> In this case the description for w2008s64 says:
>
> Upgraded with Internet Explorer 9, .Net Framework 3.5 SP1 & 4.5.1 and 
>  all updates up to 2013/04/15. Added Visual C++ 2012 & 2013 runtimes.

So running vcredist-2005sp1_x86.exe does not change anything. However 
running Windows Update a few times does install the expected msvcr80 dll 
in the end.

So I have added w2008s64 to the list of VMs to update.

[Erich E. Hoover]

On Mon, Oct 21, 2019 at 4:33 AM Piotr Caban  wrote:
> ...
> msvcr80 is the first dll that was changed. Attaching a hacky test that
> shows it (it also shows difference is %S format handling but it's not
> something that has to be fixed as part of your patch).

Thanks Piotr, I was trying to do this on Friday and didn't realize
that the manifest needed to be changed for it to work.  At some point
is it worth adding tests for these older msvcr* dlls so that it's
easier to test against them?

Anyway, I believe that this version now has all of the changes that
you have requested - so please let me know if I missed something.

Best,
Erich

[Piotr Caban]

Hi Erich,

The patch looks mostly good now but it causes compilation warnings. The 
prototype of swscanf function is incorrect:
+static int (__cdecl *p_swscanf)(const char *str, const wchar_t* format, 
...);
Also result variable is not used.

Thanks,
Piotr

On 10/21/19 6:00 PM, Erich E. Hoover wrote:
> On Mon, Oct 21, 2019 at 4:33 AM Piotr Caban  wrote:
> > ...
> > msvcr80 is the first dll that was changed. Attaching a hacky test that
> > shows it (it also shows difference is %S format handling but it's not
> > something that has to be fixed as part of your patch).
>
> Thanks Piotr, I was trying to do this on Friday and didn't realize
> that the manifest needed to be changed for it to work.  At some point
> is it worth adding tests for these older msvcr* dlls so that it's
> easier to test against them?
>
> Anyway, I believe that this version now has all of the changes that
> you have requested - so please let me know if I missed something.
>
> Best,
> Erich

[Erich E. Hoover]

On Mon, Oct 21, 2019 at 11:51 AM Piotr Caban  wrote:
> ...
> The patch looks mostly good now but it causes compilation warnings. The
> prototype of swscanf function is incorrect:
> +static int (__cdecl *p_swscanf)(const char *str, const wchar_t* format,
> ...);
> Also result variable is not used.

Whoops, sorry about that. I thought I had "-Werror" turned on to catch
such things and didn't realize that it now needs to be added to
CROSSCFLAGS as well.

Best,
Erich

[Piotr Caban]

Signed-off-by: Piotr Caban