Обсуждение: Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)

Hi all,

Here is a continuation of the thread which covered $subject for MinGW
and Cygwin:
http://www.postgresql.org/message-id/51F19059.7050702@pgexperts.com
But this time it is for MSVC.

Just a bit of background... Since commit cb4a3b04 we are installing
shared libraries in bin/ and lib/ for Cygwin and MinGW on Windows, but
we are still missing MSVC, so as of now build method is inconsistent.
Attached is a patch aimed at fixing this inconsistency. What it does
is simple: when kicking Install.pm to install the deliverables of each
project, we check if the project Makefile contains SO_MAJOR_VERSION.
If yes, libraries of this project are installed in bin/ and lib/. The
path to the Makefile is found by scanning ResourceCompile in the
vcproj file, this method having the advantage to make the patch
independent of build process.

This also removes a wart present in Install.pm installing copying
manually libpq.dll:
-       lcopy($target . '/lib/libpq.dll', $target . '/bin/libpq.dll');

I am adding an entry in the upcoming CF for this patch, and let's use
this thread for this discussion.
Regards,
--
Michael

On Tue, Mar 3, 2015 at 8:36 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Thank you Michael. I have looked the patch.

Thanks for the review!

> Overall logic looks good to me,
> I have checked it with MSVC{2013,2008}. It works for MSVC 2013 but fail for
> MSVC 2008, I think the condition "if ($proj =~
> qr{ResourceCompile\s*Include="([^"]+)"})" is not going to work for MSVC2008
> and MSVC2005 i.e.
> [...]

Thanks for the details, my patch is definitely missing vcproj entries.
Note that I don't have yet an environment with MSVC 2008 or similar, I
just got 2010 on my box for now. So you will have to wait until I have
a fresh environment to get an updated patch...

> It seems that search criteria can be narrowed to skip reading related
> Makefile for SO_MAJOR_VERSION string when VS project file is related to
> StaticLibrary or Application.

Well, agreed, and the patch takes care of that for vcxproj files by
only installing shared libraries if they use DynamicLibrary. Perhaps
you have in mind a better logic?

> Although this patch is going to make MSVC
> build consistent with Cygwin and MinGW build, following files seems
> redundant now, is there any use for them other than backward compatibility ?
> i.e.
>> inst\lib\libpq.dll
>> inst\lib\libpgtypes.dll
>> inst\lib\libecpg_compat.dll
>> inst\lib\libecpg.dll

Indeed, I haven't noticed them. But we can simply remove them as they
are installed in bin/ as well with this patch, it seems better for
consistency with MinGW and Cygwin in any case.
-- 
Michael

On Wed, Mar 4, 2015 at 4:43 PM, Michael Paquier wrote:
> On Tue, Mar 3, 2015 at 8:36 PM, Asif Naeem wrote:
>> Thank you Michael. I have looked the patch.
>
> Thanks for the review!
>
>> Overall logic looks good to me,
>> I have checked it with MSVC{2013,2008}. It works for MSVC 2013 but fail for
>> MSVC 2008, I think the condition "if ($proj =~
>> qr{ResourceCompile\s*Include="([^"]+)"})" is not going to work for MSVC2008
>> and MSVC2005 i.e.
>> [...]
>
> Thanks for the details, my patch is definitely missing vcproj entries.
> Note that I don't have yet an environment with MSVC 2008 or similar, I
> just got 2010 on my box for now. So you will have to wait until I have
> a fresh environment to get an updated patch...

OK, so I have been able to figure out a regex expression scanning for
win32ver.rc in RelativePath for a vcproj file. For vcxproj files, I am
still using ResourceCompile. Attached is the promised patch.

>> Although this patch is going to make MSVC
>> build consistent with Cygwin and MinGW build, following files seems
>> redundant now, is there any use for them other than backward compatibility ?
>> i.e.
>>> inst\lib\libpq.dll
>>> inst\lib\libpgtypes.dll
>>> inst\lib\libecpg_compat.dll
>>> inst\lib\libecpg.dll
>
> Indeed, I haven't noticed them. But we can simply remove them as they
> are installed in bin/ as well with this patch, it seems better for
> consistency with MinGW and Cygwin in any case.

Those entries are removed as well in the patch.

Regards,
--
Michael

On Wed, Mar 4, 2015 at 4:13 AM, Michael Paquier wrote:
> Those entries are removed as well in the patch.

Please find attached a new version of the patch, fixing a failure for
plperl installation that contains GNUmakefile instead of Makefile.
--
Michael

On Wed, Mar 11, 2015 at 10:03 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Thank you Michael overall v3 patch looks good to me, There is one
> observation that it is not installing following lib files that are required
> for dev work i.e.

Thanks for your input.

>> inst\lib\libpq.lib
>> inst\lib\libpgtypes.lib
>> inst\lib\libecpg_compat.lib
>> inst\lib\libecpg.lib
>
> Please do let me know if I missed something but was not there a need to
> avoid installing related .dll files in lib (duplicate) along with bin
> directory e.g?

Yeas, right. Sorry for missing something like that. In HEAD, the
following things are installed regarding the shared libraries:
- In lib/, all .dll and .lib
- In bin/, only libpq.dll

So I have recoded the patch to use an hash of arrays (makes the code
more readable IMO) to be able to track more easily what to install
where, and process now does the following for shared libraries:
- In lib/, install all .dll and .lib
- In bin/, install all .dll

I hope that's fine to you.
Regards,
--
Michael

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       tested, failed
Spec compliant:           not tested
Documentation:            not tested

Thank you Michael. v4 patch looks good to me. 

The new status of this patch is: Ready for Committer

On Mon, Mar 16, 2015 at 4:56 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:       tested, failed
> Spec compliant:           not tested
> Documentation:            not tested
>
> Thank you Michael. v4 patch looks good to me.
>
> The new status of this patch is: Ready for Committer

Thanks!
-- 
Michael

Michael Paquier wrote:

> So I have recoded the patch to use an hash of arrays (makes the code
> more readable IMO) to be able to track more easily what to install
> where, and process now does the following for shared libraries:
> - In lib/, install all .dll and .lib
> - In bin/, install all .dll

I wonder why do we need this part:

> @@ -247,22 +250,47 @@ sub CopySolutionOutput
>  
>          my $proj = read_file("$pf.$vcproj")
>            || croak "Could not open $pf.$vcproj\n";
> +
> +        # Check if this project uses a shared library by looking if
> +        # SO_MAJOR_VERSION is defined in its Makefile, whose path
> +        # can be found using the resource file of this project.
> +        if (($vcproj eq 'vcxproj' &&
> +             $proj =~ qr{ResourceCompile\s*Include="([^"]+)"}) ||
> +            ($vcproj eq 'vcproj' &&
> +             $proj =~ qr{File\s*RelativePath="([^\"]+)\.rc"}))
> +        {
> +            my $projpath = dirname($1);
> +            my $mfname = -e "$projpath/GNUmakefile" ?
> +                "$projpath/GNUmakefile" : "$projpath/Makefile";
> +            my $mf = read_file($mfname) ||
> +                croak "Could not open $mfname\n";
> +
> +            if ($mf =~ /^SO_MAJOR_VERSION\s*=\s*(.*)$/mg)
> +            {
> +                $is_sharedlib = 1;
> +            }
> +        }

I mean, can't we just do the "push" unconditionally here?

>              elsif ($1 == 2)
>              {
> -                $dir = "lib";
> -                $ext = "dll";
> +                push( @{ $install_list { 'lib' } }, "dll");
> +                if ($is_sharedlib)
> +                {
> +                    push( @{ $install_list { 'bin' } }, "dll");
> +                    push( @{ $install_list { 'lib' } }, "lib");
> +                }
>              }

Surely if there are no "lib/dll" files in the subdirectory, nothing will
happen, right?  (I haven't actually tried.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Mar 18, 2015 at 3:13 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> So I have recoded the patch to use an hash of arrays (makes the code
>> more readable IMO) to be able to track more easily what to install
>> where, and process now does the following for shared libraries:
>> - In lib/, install all .dll and .lib
>> - In bin/, install all .dll
>
> I wonder why do we need this part:
>
>> @@ -247,22 +250,47 @@ sub CopySolutionOutput
>>
>>               my $proj = read_file("$pf.$vcproj")
>>                 || croak "Could not open $pf.$vcproj\n";
>> +
>> +             # Check if this project uses a shared library by looking if
>> +             # SO_MAJOR_VERSION is defined in its Makefile, whose path
>> +             # can be found using the resource file of this project.
>> +             if (($vcproj eq 'vcxproj' &&
>> +                      $proj =~ qr{ResourceCompile\s*Include="([^"]+)"}) ||
>> +                     ($vcproj eq 'vcproj' &&
>> +                      $proj =~ qr{File\s*RelativePath="([^\"]+)\.rc"}))
>> +             {
>> +                     my $projpath = dirname($1);
>> +                     my $mfname = -e "$projpath/GNUmakefile" ?
>> +                             "$projpath/GNUmakefile" : "$projpath/Makefile";
>> +                     my $mf = read_file($mfname) ||
>> +                             croak "Could not open $mfname\n";
>> +
>> +                     if ($mf =~ /^SO_MAJOR_VERSION\s*=\s*(.*)$/mg)
>> +                     {
>> +                             $is_sharedlib = 1;
>> +                     }
>> +             }
>
> I mean, can't we just do the "push" unconditionally here?

Why should we install unnecessary stuff? This complicates the
installation contents, the point being to have only shared libraries's
dll installed in bin/, and make things consistent with what MinGW
does.

>>                       elsif ($1 == 2)
>>                       {
>> -                             $dir = "lib";
>> -                             $ext = "dll";
>> +                             push( @{ $install_list { 'lib' } }, "dll");
>> +                             if ($is_sharedlib)
>> +                             {
>> +                                     push( @{ $install_list { 'bin' } }, "dll");
>> +                                     push( @{ $install_list { 'lib' } }, "lib");
>> +                             }
>>                       }
>
> Surely if there are no "lib/dll" files in the subdirectory, nothing will
> happen, right?  (I haven't actually tried.)

No, it fails. And we should actually have a bin/lib that has been
correctly generated. Do you think this is a problem? Normally we
*should* fail IMO, meaning that the build process has broken what it
should have done.
-- 
Michael

Michael Paquier wrote:
> On Wed, Mar 18, 2015 at 3:13 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > I mean, can't we just do the "push" unconditionally here?
> 
> Why should we install unnecessary stuff? This complicates the
> installation contents, the point being to have only shared libraries's
> dll installed in bin/, and make things consistent with what MinGW
> does.

I was expecting that if no .dll or .lib file is expected to be
installed, then the file would not be present in the first place.
I just wanted to avoid what seems fragile coding.

> > Surely if there are no "lib/dll" files in the subdirectory, nothing will
> > happen, right?  (I haven't actually tried.)
> 
> No, it fails. And we should actually have a bin/lib that has been
> correctly generated. Do you think this is a problem? Normally we
> *should* fail IMO, meaning that the build process has broken what it
> should have done.

Makes sense.

I have pushed your patch; we'll see what the buildfarm thinks of it.
(Sadly, the number of MSVC members is rather small and they don't run
often.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Mar 19, 2015 at 3:30 AM, Alvaro Herrera wrote:
> Makes sense.
>
> I have pushed your patch; we'll see what the buildfarm thinks of it.

Thanks.

> (Sadly, the number of MSVC members is rather small and they don't run
> often.)

Once Windows 10 is out, it could be installed on a Raspberry PI 2.
This would make a nice buildfarm member with MSVC 2015.
-- 
Michael