Re: [programs/winetest] Extend PATH if we have dll's that only come with .NET (Try2)

[Nicolas Le Cam]

Hi Paul, minor comments :
> programs/winetest/main.c |   60
> +++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 59 insertions(+), 1 deletions(-)
>
> diff --git a/programs/winetest/main.c b/programs/winetest/main.c
> index 74a4307..58e520b 100644
> --- a/programs/winetest/main.c
> +++ b/programs/winetest/main.c
> @@ -44,6 +44,7 @@ struct wine_test
>     int subtest_count;
>     char **subtests;
>     char *exename;
> - char *maindllpath;
>
> };
>
> char *tag = NULL;
> @@ -61,6 +62,9 @@ static unsigned int nb_filters = 0;
>  static HMODULE hmscoree;
>  static HRESULT (WINAPI *pLoadLibraryShim)(LPCWSTR, LPCWSTR, LPVOID, HMODULE
> *);
>
> +/* To store the current PATH setting (related to .NET only provided dlls)
> */
> +static char *curpath;
> -
>
> /* check if test is being filtered out */
>  static BOOL test_filtered_out( LPCSTR module, LPCSTR testname )
>  {
> @@ -341,6 +345,19 @@ static DWORD wait_process( HANDLE process, DWORD
> timeout )
>     return WAIT_TIMEOUT;
>  }
>
> +static void append_path( const char *path)
> +{
> - char *newpath;
> -
> - newpath = xmalloc(strlen(curpath) + 1 + strlen(path) + 1);
> - strcpy(newpath, curpath);
> - strcat(newpath, ";");
> - strcat(newpath, path);
> - SetEnvironmentVariableA("PATH", newpath);
> -
> - free(newpath);
>
> +}

Perhaps use newpath = strmake( NULL, "%s;%s", curpath, path); I don't
know. (I used it in a similar patch that I was making yesterday)
> -
>
> /* Run a command for MS milliseconds.  If OUT != NULL, also redirect
>    stdout to there.
>
> @@ -446,8 +463,16 @@ get_subtests (const char *tempdir, struct wine_test
> *test, LPTSTR res_name)
>
> extract_test (test, tempdir, res_name);
>     cmd = strmake (NULL, "%s --list", test->exename);
> - if (test->maindllpath) {
> - /* We need to add the path (to the main dll) to PATH */
> - append_path(test->maindllpath);
> - }
>
> status = run_ex (cmd, subfile, tempdir, 5000);
>     err = GetLastError();
> - if (test->maindllpath) {
> - /* Restore PATH again */
> - SetEnvironmentVariableA("PATH", curpath);
> - }
>
> free (cmd);
>
> if (status == -2)
> @@ -538,11 +563,27 @@ extract_test_proc (HMODULE hModule, LPCTSTR lpszType,
>     strcpy(dllname, lpszName);
>     *strstr(dllname, testexe) = 0;
> - wine_tests[nr_of_files].maindllpath = NULL;
>
> dll = LoadLibraryExA(dllname, NULL, LOAD_LIBRARY_AS_DATAFILE);
>     if (!dll && pLoadLibraryShim)
>     {
>         MultiByteToWideChar(CP_ACP, 0, dllname, -1, dllnameW, MAX_PATH);
> - if (FAILED( pLoadLibraryShim(dllnameW, NULL, NULL, &dll) )) dll =
>
> 0;
> - if (FAILED( pLoadLibraryShim(dllnameW, NULL, NULL, &dll) ))
> - dll = 0;
> - else
> - {
> - char dllpath[MAX_PATH];

dllpath should be declared at start of the function IMHO.
> -
> - /* We have a dll that cannot be found through LoadLibraryExA.
>
> This
> - * is the case for .NET provided dll's. We will add the
>
> directory
> - * where the dll resides to the PATH variable when dealing with
> - * the tests for this dll.
> - */
> - GetModuleFileNameA(dll, dllpath, MAX_PATH);
> - *strrchr(dllpath, '\') = '\0';
> - wine_tests[nr_of_files].maindllpath = xmalloc(strlen(dllpath) +
>
> 1);
> - strcpy(wine_tests[nr_of_files].maindllpath, dllpath);
> - }

This won't work if test needs more than one dll found through LoadLibraryShim
> }
>     if (!dll) {
>         xprintf ("    %s=dll is missing\n", dllname);
> @@ -578,6 +619,12 @@ run_tests (char *logname)
>     DWORD strsize;
>     SECURITY_ATTRIBUTES sa;
>     char tmppath[MAX_PATH], tempdir[MAX_PATH+4];
> - DWORD needed;
> -
> - /* Get the current PATH only once */
> - needed = GetEnvironmentVariableA("PATH", NULL, 0);
> - curpath = xmalloc(needed);
> - GetEnvironmentVariable("PATH", curpath, needed);

This should be GetEnvironmentVariableA.
> SetErrorMode (SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX);
>
> @@ -684,11 +731,21 @@ run_tests (char *logname)
>         struct wine_test *test = wine_tests + i;
>         int j;
> - if (test->maindllpath) {
> - /* We need to add the path (to the main dll) to PATH */
> - append_path(test->maindllpath);
> - }
> -
>
> for (j = 0; j < test->subtest_count; j++) {
>             report (R_STEP, "Running: %s:%s", test->name,
>                     test->subtests[j]);
>            run_test (test, test->subtests[j], logfile, tempdir);
>         }
> -
> - if (test->maindllpath) {
> - /* Restore PATH again */
> - SetEnvironmentVariableA("PATH", curpath);
> - }
>
> }
>     report (R_DELTA, 0, "Running: Done");
>
> @@ -697,6 +754,7 @@ run_tests (char *logname)
>     logfile = 0;
>     remove_dir (tempdir);
>     free (wine_tests);
> - free (curpath);
>
> return logname;
>  }

I did a similar patch yesterday but forget to set PATH around run_ex
in get_subtests so it didn't work when I tested on my W2K Pro test
platform.
Thanks for your work

[programs/winetest] Extend PATH if we have dll's that only come with .NET (Try2)

[Paul Vriens]

Nicolas Le Cam wrote:
> Hi Paul, minor comments :
> > +static void append_path( const char *path)
> > +{
> > - char *newpath;
> > -
> > - newpath = xmalloc(strlen(curpath) + 1 + strlen(path) + 1);
> > - strcpy(newpath, curpath);
> > - strcat(newpath, ";");
> > - strcat(newpath, path);
> > - SetEnvironmentVariableA("PATH", newpath);
> > -
> > - free(newpath);
> >
> > +}
>
> Perhaps use newpath = strmake( NULL, "%s;%s", curpath, path); I don't
> know. (I used it in a similar patch that I was making yesterday)

Just a matter of preference I guess.
> > if (!dll && pLoadLibraryShim)
> > {
> >     MultiByteToWideChar(CP_ACP, 0, dllname, -1, dllnameW, MAX_PATH);
> > -
> >    if (FAILED( pLoadLibraryShim(dllnameW, NULL, NULL, &dll) )) dll =
> >
> > 0;
> > -
> >    if (FAILED( pLoadLibraryShim(dllnameW, NULL, NULL, &dll) ))
> > -
> >        dll = 0;
> > -
> >    else
> > -
> >    {
> > -
> >        char dllpath[MAX_PATH];
>
> dllpath should be declared at start of the function IMHO.

It's only needed in the else branch.
> > -
> > -
> >        /* We have a dll that cannot be found through LoadLibraryExA.
> >
> > This
> > -
> >         * is the case for .NET provided dll's. We will add the
> >
> > directory
> > -
> >         * where the dll resides to the PATH variable when dealing with
> > -
> >         * the tests for this dll.
> > -
> >         */
> > -
> >        GetModuleFileNameA(dll, dllpath, MAX_PATH);
> > -
> >        *strrchr(dllpath, '\\') = '\0';
> > -
> >        wine_tests[nr_of_files].maindllpath = xmalloc(strlen(dllpath) +
> >
> > 1);
> > -
> >        strcpy(wine_tests[nr_of_files].maindllpath, dllpath);
> > -
> >    }
>
> This won't work if test needs more than one dll found through LoadLibraryShim

Not sure what you mean as LoadLibraryShim will only return 1 dll.
> > }
> > if (!dll) {
> >     xprintf ("    %s=dll is missing\n", dllname);
> >
> > @@ -578,6 +619,12 @@ run_tests (char *logname)
> >     DWORD strsize;
> >     SECURITY_ATTRIBUTES sa;
> >     char tmppath[MAX_PATH], tempdir[MAX_PATH+4];
> > - DWORD needed;
> > -
> > - /* Get the current PATH only once */
> > - needed = GetEnvironmentVariableA("PATH", NULL, 0);
> > - curpath = xmalloc(needed);
> > - GetEnvironmentVariable("PATH", curpath, needed);
>
> This should be GetEnvironmentVariableA.

Well it would work anyway but for consistency sake I will change that.

[programs/winetest] Extend PATH if we have dll's that only come with .NET (Try2)

[Nicolas Le Cam]

> > > -
> > > - /* We have a dll that cannot be found through
> > >
> > > LoadLibraryExA.
> > > This
> > > - * is the case for .NET provided dll's. We will add the
> > >
> > > directory
> > > - * where the dll resides to the PATH variable when dealing
> > >
> > > with
> > > - * the tests for this dll.
> > > - */
> > > - GetModuleFileNameA(dll, dllpath, MAX_PATH);
> > > - *strrchr(dllpath, '\') = '\0';
> > > - wine_tests[nr_of_files].maindllpath =
> > >
> > > xmalloc(strlen(dllpath) +
> > > 1);
> > > - strcpy(wine_tests[nr_of_files].maindllpath, dllpath);
> > > - }
> >
> > This won't work if test needs more than one dll found through
> > LoadLibraryShim
>
> Not sure what you mean as LoadLibraryShim will only return 1 dll.

I mean if the test imports more than one dll that can only be found by
LoadLibraryShim it will replace the first PATH you retrieved by the
second one, and a message will be displayed for the first d. In my
test I did something like that :

else
        {
            char dllpath[MAX_PATH];

/* We have a dll that cannot be found through LoadLibraryExA. This
             * is the case for .NET provided dll's. We will add the directory
             * where the dll resides to the PATH variable when dealing with
             * the tests for this dll.
             */
            GetModuleFileNameA(dll, dllpath, MAX_PATH);
            *strrchr(dllpath, '\') = '\0';
            if (!wine_tests[nr_of_files].maindllpath)
                wine_tests[nr_of_files].maindllpath = strmake ( NULL,
";%s", dllpath);
            else
            {
                char *newpath = wine_tests[nr_of_files].maindllpath;

wine_tests[nr_of_files].maindllpath = strmake ( NULL,
"%s;%s", newpath, dllpath);
                free(newpath);
            }
        }

and wine_tests[nr_of_files].maindllpath = NULL; wasn't set in extract_test_proc

[programs/winetest] Extend PATH if we have dll's that only come with .NET (Try2)

[Paul Vriens]

Nicolas Le Cam wrote:
> > > > -
> > > > -
> > > >        /* We have a dll that cannot be found through
> > > >
> > > > LoadLibraryExA.
> > > > This
> > > > -
> > > >         * is the case for .NET provided dll's. We will add the
> > > >
> > > > directory
> > > > -
> > > >         * where the dll resides to the PATH variable when dealing
> > > >
> > > > with
> > > > -
> > > >         * the tests for this dll.
> > > > -
> > > >         */
> > > > -
> > > >        GetModuleFileNameA(dll, dllpath, MAX_PATH);
> > > > -
> > > >        *strrchr(dllpath, '\\') = '\0';
> > > > -
> > > >        wine_tests[nr_of_files].maindllpath =
> > > >
> > > > xmalloc(strlen(dllpath) +
> > > > 1);
> > > > -
> > > >        strcpy(wine_tests[nr_of_files].maindllpath, dllpath);
> > > > -
> > > >    }
> > >
> > > This won't work if test needs more than one dll found through
> > > LoadLibraryShim
> >
> > Not sure what you mean as LoadLibraryShim will only return 1 dll.
>
> I mean if the test imports more than one dll that can only be found by
> LoadLibraryShim it will replace the first PATH you retrieved by the
> second one, and a message will be displayed for the first d. In my
> test I did something like that :
>     else
>     {
>         char dllpath[MAX_PATH];
>
>         /* We have a dll that cannot be found through LoadLibraryExA. This
>          * is the case for .NET provided dll's. We will add the directory
>          * where the dll resides to the PATH variable when dealing with
>          * the tests for this dll.
>          */
>         GetModuleFileNameA(dll, dllpath, MAX_PATH);
>         *strrchr(dllpath, '\\') = '\0';
>         if (!wine_tests[nr_of_files].maindllpath)
>             wine_tests[nr_of_files].maindllpath = strmake ( NULL,
>
> ";%s", dllpath);
>             else
>             {
>                 char *newpath = wine_tests[nr_of_files].maindllpath;
>             wine_tests[nr_of_files].maindllpath = strmake ( NULL,
>
> "%s;%s", newpath, dllpath);
>                 free(newpath);
>             }
>         }
>
> and wine_tests[nr_of_files].maindllpath = NULL; wasn't set in extract_test_proc

I still don't get it.

The only dll that we try to load is the main dll. We don't care about all the 
other imports.

[programs/winetest] Extend PATH if we have dll's that only come with .NET (Try2)

[Nicolas Le Cam]

2009/2/25 Paul Vriens :
> Nicolas Le Cam wrote:
> > > > > -
> > > > > - /* We have a dll that cannot be found through
> > > > >
> > > > > LoadLibraryExA.
> > > > > This
> > > > > - * is the case for .NET provided dll's. We will add the
> > > > >
> > > > > directory
> > > > > - * where the dll resides to the PATH variable when dealing
> > > > >
> > > > > with
> > > > > - * the tests for this dll.
> > > > > - */
> > > > > - GetModuleFileNameA(dll, dllpath, MAX_PATH);
> > > > > - *strrchr(dllpath, '\') = '\0';
> > > > > - wine_tests[nr_of_files].maindllpath =
> > > > >
> > > > > xmalloc(strlen(dllpath) +
> > > > > 1);
> > > > > - strcpy(wine_tests[nr_of_files].maindllpath, dllpath);
> > > > > - }
> > > >
> > > > This won't work if test needs more than one dll found through
> > > > LoadLibraryShim
> > >
> > > Not sure what you mean as LoadLibraryShim will only return 1 dll.
> >
> > I mean if the test imports more than one dll that can only be found by
> > LoadLibraryShim it will replace the first PATH you retrieved by the
> > second one, and a message will be displayed for the first d. In my
> > test I did something like that :
> >
> > else
> >        {
> >            char dllpath[MAX_PATH];
> >
> > /* We have a dll that cannot be found through LoadLibraryExA.
> > This
> >             * is the case for .NET provided dll's. We will add the
> > directory
> >             * where the dll resides to the PATH variable when dealing with
> >             * the tests for this dll.
> >             */
> >            GetModuleFileNameA(dll, dllpath, MAX_PATH);
> >            *strrchr(dllpath, '\') = '\0';
> >            if (!wine_tests[nr_of_files].maindllpath)
> >                wine_tests[nr_of_files].maindllpath = strmake ( NULL,
> > ";%s", dllpath);
> >            else
> >            {
> >                char *newpath = wine_tests[nr_of_files].maindllpath;
> >
> > wine_tests[nr_of_files].maindllpath = strmake ( NULL,
> > "%s;%s", newpath, dllpath);
> >                free(newpath);
> >            }
> >        }
> >
> > and wine_tests[nr_of_files].maindllpath = NULL; wasn't set in
> > extract_test_proc
>
> I still don't get it.
>
> The only dll that we try to load is the main dll. We don't care about all
> the other imports.
>
> --
> Cheers,
>
> Paul.

Now I get it.
I thought we were trying to resolve every imports. Seems that I didn't
understand the first part of extract_test_proc as I should.
Sorry, I was mistaken.