Bug 10472 - pidl: waf should have an option for the dir to install perl files and do not glob
pidl: waf should have an option for the dir to install perl files and do not ...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Build
unspecified
All All
: P5 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
: 10602 (view as bug list)
Depends on:
Blocks: 10077
  Show dependency treegraph
 
Reported: 2014-02-26 09:22 UTC by Andreas Schneider
Modified: 2014-11-24 20:09 UTC (History)
7 users (show)

See Also:


Attachments
v4-0-test and v4-1-test patch (5.11 KB, patch)
2014-02-27 11:29 UTC, Andreas Schneider
no flags Details
v4-0-test and v4-1-test patch (7.70 KB, patch)
2014-02-27 18:23 UTC, Andreas Schneider
metze: review-
Details
v4-1-test patch (14.33 KB, patch)
2014-03-07 07:18 UTC, Andreas Schneider
metze: review+
asn: review? (ddiss)
Details
v4-0-test patch (14.29 KB, patch)
2014-03-07 07:19 UTC, Andreas Schneider
metze: review+
asn: review? (ddiss)
Details
Additional patches for v4-1-test (8.76 KB, patch)
2014-05-10 08:31 UTC, Stefan Metzmacher
metze: review? (asn)
asn: review+
Details
Additional patches for v4-0-test (8.76 KB, patch)
2014-05-10 08:34 UTC, Stefan Metzmacher
asn: review+
Details
possible patchset for v4-1-test (54.14 KB, patch)
2014-11-10 15:23 UTC, Michael Adam
no flags Details
Final patchset for v4-1-test (55.09 KB, patch)
2014-11-12 10:54 UTC, Stefan Metzmacher
obnox: review+
Details
Final patchset for v4-0-test (55.03 KB, patch)
2014-11-12 10:55 UTC, Stefan Metzmacher
obnox: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schneider 2014-02-26 09:22:22 UTC
waf should have an option for the dir to install perl files and do not glob.
Comment 1 Andreas Schneider 2014-02-27 11:29:54 UTC
Created attachment 9730 [details]
v4-0-test and v4-1-test patch
Comment 2 Andreas Schneider 2014-02-27 14:54:48 UTC
Comment on attachment 9730 [details]
v4-0-test and v4-1-test patch

We need an additional patch for Yapp.
Comment 3 Andreas Schneider 2014-02-27 18:23:07 UTC
Created attachment 9731 [details]
v4-0-test and v4-1-test patch
Comment 4 Stefan Metzmacher 2014-02-27 19:22:11 UTC
Comment on attachment 9731 [details]
v4-0-test and v4-1-test patch

I think --with-perl-vendordir should be
--with-perl-vendorlib
and the default should be
read_out('print $Config{vendorlib}')[0]

The buildtools/wafadmin/ change should be in its own commit
Comment 5 Andreas Schneider 2014-02-27 20:21:09 UTC
read_out('print $Config{vendorlib}')[0] will not work in autobuild! A user can't install to /usr/share/perl5/vendor_lib. Kai wanted it to be in the installation path cause you can set $PERL5LIB. The same would apply to python ...


I used --with-perl-vendordir to be the same pattern as think --with-perl-archdir.
Comment 6 Stefan Metzmacher 2014-02-28 09:29:44 UTC
(In reply to comment #5)
> read_out('print $Config{vendorlib}')[0] will not work in autobuild! A user
> can't install to /usr/share/perl5/vendor_lib. Kai wanted it to be in the
> installation path cause you can set $PERL5LIB. The same would apply to python
> ...
> 
> 
> I used --with-perl-vendordir to be the same pattern as think
> --with-perl-archdir.

No, that uses

        if getattr(Options.options, 'perlarchdir', None):
                conf.env.ARCHDIR_PERL = Options.options.perlarchdir
        else:
                conf.env.ARCHDIR_PERL = read_out('print $Config{sitearch}')[0]

While you just used:

        conf.env.VENDORDIR_PERL = Options.options.perlvendordir
Comment 7 Stefan Metzmacher 2014-02-28 10:00:39 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > read_out('print $Config{vendorlib}')[0] will not work in autobuild! A user
> > can't install to /usr/share/perl5/vendor_lib. Kai wanted it to be in the
> > installation path cause you can set $PERL5LIB. The same would apply to python
> > ...
> > 
> > 
> > I used --with-perl-vendordir to be the same pattern as think
> > --with-perl-archdir.
> 
> No, that uses
> 
>         if getattr(Options.options, 'perlarchdir', None):
>                 conf.env.ARCHDIR_PERL = Options.options.perlarchdir
>         else:
>                 conf.env.ARCHDIR_PERL = read_out('print $Config{sitearch}')[0]
> 
> While you just used:
> 
>         conf.env.VENDORDIR_PERL = Options.options.perlvendordir

Also ${DATAROOTDIR}/perl5/vendor_perl, which typically leads to
/usr/share/perl5/vendor_perl is not always the correct path.
On my ubuntu 12.04 system I have 'vendorlib' => '/usr/share/perl5'.
Comment 8 Andreas Schneider 2014-02-28 10:43:33 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > read_out('print $Config{vendorlib}')[0] will not work in autobuild! A user
> > can't install to /usr/share/perl5/vendor_lib. Kai wanted it to be in the
> > installation path cause you can set $PERL5LIB. The same would apply to python
> > ...
> > 
> > 
> > I used --with-perl-vendordir to be the same pattern as think
> > --with-perl-archdir.
> 
> No, that uses
> 
>         if getattr(Options.options, 'perlarchdir', None):
>                 conf.env.ARCHDIR_PERL = Options.options.perlarchdir
>         else:
>                 conf.env.ARCHDIR_PERL = read_out('print $Config{sitearch}')[0]

The options default is None. So by default it uses the sitearch directory, which is e.g. /usr/lib64/perl5/sitearch. I've tried to push something like that to autobuild it failed cause it can't install to /usr !!!

> While you just used:
> 
>         conf.env.VENDORDIR_PERL = Options.options.perlvendordir

Yes, cause the option has a default. Which is default='${DATAROOTDIR}/perl5/vendor_perl'
Comment 9 Andreas Schneider 2014-02-28 10:44:59 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > read_out('print $Config{vendorlib}')[0] will not work in autobuild! A user
> > > can't install to /usr/share/perl5/vendor_lib. Kai wanted it to be in the
> > > installation path cause you can set $PERL5LIB. The same would apply to python
> > > ...
> > > 
> > > 
> > > I used --with-perl-vendordir to be the same pattern as think
> > > --with-perl-archdir.
> > 
> > No, that uses
> > 
> >         if getattr(Options.options, 'perlarchdir', None):
> >                 conf.env.ARCHDIR_PERL = Options.options.perlarchdir
> >         else:
> >                 conf.env.ARCHDIR_PERL = read_out('print $Config{sitearch}')[0]
> > 
> > While you just used:
> > 
> >         conf.env.VENDORDIR_PERL = Options.options.perlvendordir
> 
> Also ${DATAROOTDIR}/perl5/vendor_perl, which typically leads to
> /usr/share/perl5/vendor_perl is not always the correct path.
> On my ubuntu 12.04 system I have 'vendorlib' => '/usr/share/perl5'.

That's why I added an option so a packager can correctly specify the correct directory!

If you want read_out('print $Config{vendorlib}')[0] to be the default, you need to change autobuild and the buildfarm first!
Comment 10 Stefan Metzmacher 2014-02-28 10:54:54 UTC
Am 28.02.2014 11:43, schrieb samba-bugs@samba.org:
> https://bugzilla.samba.org/show_bug.cgi?id=10472
> 
> --- Comment #8 from Andreas Schneider <asn@samba.org> 2014-02-28 10:43:33 UTC ---
> (In reply to comment #6)
>> (In reply to comment #5)
>>> read_out('print $Config{vendorlib}')[0] will not work in autobuild! A user
>>> can't install to /usr/share/perl5/vendor_lib. Kai wanted it to be in the
>>> installation path cause you can set $PERL5LIB. The same would apply to python
>>> ...
>>>
>>>
>>> I used --with-perl-vendordir to be the same pattern as think
>>> --with-perl-archdir.
>>
>> No, that uses
>>
>>         if getattr(Options.options, 'perlarchdir', None):
>>                 conf.env.ARCHDIR_PERL = Options.options.perlarchdir
>>         else:
>>                 conf.env.ARCHDIR_PERL = read_out('print $Config{sitearch}')[0]
> 
> The options default is None. So by default it uses the sitearch directory,
> which is e.g. /usr/lib64/perl5/sitearch. I've tried to push something like that
> to autobuild it failed cause it can't install to /usr !!!

Then we need to explicitly specify it in autobuild, like we specify
--prefix.

metze
Comment 11 Stefan Metzmacher 2014-02-28 10:55:49 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > read_out('print $Config{vendorlib}')[0] will not work in autobuild! A user
> > > > can't install to /usr/share/perl5/vendor_lib. Kai wanted it to be in the
> > > > installation path cause you can set $PERL5LIB. The same would apply to python
> > > > ...
> > > > 
> > > > 
> > > > I used --with-perl-vendordir to be the same pattern as think
> > > > --with-perl-archdir.
> > > 
> > > No, that uses
> > > 
> > >         if getattr(Options.options, 'perlarchdir', None):
> > >                 conf.env.ARCHDIR_PERL = Options.options.perlarchdir
> > >         else:
> > >                 conf.env.ARCHDIR_PERL = read_out('print $Config{sitearch}')[0]
> > > 
> > > While you just used:
> > > 
> > >         conf.env.VENDORDIR_PERL = Options.options.perlvendordir
> > 
> > Also ${DATAROOTDIR}/perl5/vendor_perl, which typically leads to
> > /usr/share/perl5/vendor_perl is not always the correct path.
> > On my ubuntu 12.04 system I have 'vendorlib' => '/usr/share/perl5'.
> 
> That's why I added an option so a packager can correctly specify the correct
> directory!
> 
> If you want read_out('print $Config{vendorlib}')[0] to be the default, you need
> to change autobuild and the buildfarm first!

Then we need to do that:-)
Comment 12 Kai Blin 2014-02-28 11:16:32 UTC
What I'm worried about in that respect is that this breaks the use case of not having to be root to install to a --prefix I own as a user.
Comment 13 Andreas Schneider 2014-02-28 11:23:58 UTC
(In reply to comment #12)
> What I'm worried about in that respect is that this breaks the use case of not
> having to be root to install to a --prefix I own as a user.

You can still do that. You just have to specify --with-perl-vendorlib=/my/own/prefix/share/perl5
Comment 14 Stefan Metzmacher 2014-02-28 11:45:09 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > What I'm worried about in that respect is that this breaks the use case of not
> > having to be root to install to a --prefix I own as a user.
> 
> You can still do that. You just have to specify
> --with-perl-vendorlib=/my/own/prefix/share/perl5

We could also do some tricks comparing
vendorprefix with vendorlib and append the difference to
our 'prefix'.

My ubuntu 12.04 has

'vendorprefix' => '/usr',
'vendorlib' => '/usr/share/perl5',
'vendorarch' => '/usr/lib/perl5',

This could then result in default of ${prefix}/share/perl5 for vendorlib
and ${prefix}/lib/perl5 for vendorarch.

I'm not sure I really like it, but that could be possible...

Or we check if vendorprefix doesn't match our prefix and give an error
and enforce that --with-perl-vendorlib is explicitly specified.
Comment 15 Andreas Schneider 2014-02-28 16:22:17 UTC
New patchset ...

https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/pidl-waf
Comment 16 Andreas Schneider 2014-03-07 07:18:38 UTC
Created attachment 9753 [details]
v4-1-test patch
Comment 17 Andreas Schneider 2014-03-07 07:19:29 UTC
Created attachment 9754 [details]
v4-0-test patch

The pidl patches applied cleanly the autobuild script needed some fixing to apply.
Comment 18 Karolin Seeger 2014-03-25 10:03:16 UTC
(In reply to comment #17)
> Created attachment 9754 [details]
> v4-0-test patch
> 
> The pidl patches applied cleanly the autobuild script needed some fixing to
> apply.

Pushed patchsets to autobuild-v4-1-test and autobuild-v4-0-test.
Comment 19 Karolin Seeger 2014-04-04 18:55:59 UTC
Pushed to v4-1-test and v4-0-test.
Closing out bug report.

Thanks!
Comment 20 Tom Schulz 2014-05-05 17:31:50 UTC
I believe that the fix for this bug is what is causing configure to blow up.

We have a site compiled Perl installed, built mostly with the default configuration.  From perl -V I get:

  @INC:
    /opt/local/lib/perl5/5.8.9/i86pc-solaris
    /opt/local/lib/perl5/5.8.9
    /opt/local/lib/perl5/site_perl/5.8.9/i86pc-solaris
    /opt/local/lib/perl5/site_perl/5.8.9
    .

Configure dies with the following traceback:


Traceback (most recent call last):
  File "./buildtools/bin/waf", line 76, in <module>
    Scripting.prepare(t, cwd, VERSION, wafdir)
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Scripting.py", line 145, in prepare
    prepare_impl(t, cwd, ver, wafdir)
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Scripting.py", line 135, in prepare_impl
    main()
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/wscript", line 280, in main
    wildcard_main(wildcard_cmd)
  File "./buildtools/wafsamba/samba_wildcard.py", line 110, in wildcard_main
    fun(ctx)
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Scripting.py", line 241, in configure
    conf.sub_config([''])
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Configure.py", line 237, in sub_config
    self.recurse(k, name='configure')
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Utils.py", line 634, in recurse
    f(self)  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/wscript", line 140, in configure
    conf.RECURSE('pidl')
  File "./buildtools/wafsamba/samba_utils.py", line 469, in RECURSE
    return ctx.sub_config(relpath)
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Configure.py", line 237, in sub_config
    self.recurse(k, name='configure')
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Utils.py", line 634, in recurse
    f(self)
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/pidl/wscript", line 33, in configure
    conf.check_perl_ext_devel()
  File "/home/projects/tools/samba/samba-4.1.7.i386gcc.pt/buildtools/wafadmin/Tools/perl.py", line 104, in check_perl_ext_devel
    conf.env.PERL_VENDORARCH_DIR = read_out('print $Config{vendorarch}')[0]
IndexError: list index out of range

If I add
--with-perl-vendorarch=/opt/local/lib/perl5/site_perl/5.8.9/i86pc-solaris
and
--with-perl-vendorlib=/opt/local/lib/perl5/site_perl/5.8.9
to the configure command then configure works. This was previously not necessary. In any case, configure should not blow up in this manner.
Comment 21 Andreas Schneider 2014-05-06 07:25:22 UTC
Tom, can you test the patch here:

https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/perl_dectection
Comment 22 Tom Schulz 2014-05-06 17:33:27 UTC
The patched 4.1.7 configures, builds and installs without error.
It installs the perl stuff in prefix/lib/perl5
Previous versions of Samba installed the perl stuff in prefix/share/perl5
Comment 23 Tom Schulz 2014-05-06 23:37:08 UTC
I should add that if the perl module is installed in /opt/local/lib/perl5/site_perl/5.8.9 (part of our @INC), configure does find it and outputs  the message:

Checking for perl module Parse::Yapp::Driver 1.05            : ok
Comment 24 Stefan Metzmacher 2014-05-10 08:30:21 UTC
*** Bug 10602 has been marked as a duplicate of this bug. ***
Comment 25 Stefan Metzmacher 2014-05-10 08:31:26 UTC
Created attachment 9933 [details]
Additional patches for v4-1-test
Comment 26 Stefan Metzmacher 2014-05-10 08:34:21 UTC
Created attachment 9934 [details]
Additional patches for v4-0-test
Comment 27 Tom Schulz 2014-05-12 17:33:26 UTC
I tried the latest patch against 4.1.7. The output of configure changed as follows:

+ Checking for perl $Config{vendorarch}:                                                          : not found 
+ Checking for perl $Config{sitearch}:                                                            : '/opt/local/lib/perl5/site_perl/5.8.9/i86pc-solaris' 
+ PERL_ARCH_INSTALL_DIR:                                                                          : '/opt/local/lib/perl5/site_perl/5.8.9/i86pc-solaris' 
+ Checking for perl $Config{vendorlib}:                                                           : not found 
+ Checking for perl $Config{sitelib}:                                                             : '/opt/local/lib/perl5/site_perl/5.8.9' 
+ PERL_LIB_INSTALL_DIR:                                                                           : '/opt/local/lib/perl5/site_perl/5.8.9' 

+ Checking for perl $Config{vendorarch}:                                                          : not found 
+ Checking for perl $Config{sitearch}:                                                            : '/opt/local/lib/perl5/site_perl/5.8.9/i86pc-solaris' 
+ PERL_ARCH_INSTALL_DIR:                                                                          : '/opt/local/lib/perl5/site_perl/5.8.9/i86pc-solaris' 
+ Checking for perl $Config{vendorlib}:                                                           : not found 
+ Checking for perl $Config{sitelib}:                                                             : '/opt/local/lib/perl5/site_perl/5.8.9' 
+ PERL_LIB_INSTALL_DIR:                                                                           : '/opt/local/lib/perl5/site_perl/5.8.9' 


And the perl module is now installed in
opt/local/lib/perl5/site_perl/5.8.9/Parse
Comment 28 Andreas Schneider 2014-05-14 06:35:42 UTC
Comment on attachment 9934 [details]
Additional patches for v4-0-test

LGTM
Comment 29 Andreas Schneider 2014-05-14 06:36:16 UTC
Karolin, please add the additional patches!
Comment 30 Karolin Seeger 2014-05-20 09:42:44 UTC
(In reply to comment #29)
> Karolin, please add the additional patches!

Pushed additional patches to autobuild-v4-[0|1]-test.
Comment 31 Karolin Seeger 2014-05-27 19:12:36 UTC
(In reply to comment #30)
> (In reply to comment #29)
> > Karolin, please add the additional patches!
> 
> Pushed additional patches to autobuild-v4-[0|1]-test.

Pushed to both branches.
Closing out bug report.

Thanks!
Comment 32 Volker Lendecke 2014-07-11 08:58:54 UTC
Possibly unrelated, but current master's "make install" does not respect --prefix for pidl anymore. It always wants to install in system directories, although --prefix was given. This breaks non-root make install. Sure, you can make it install somewhere else with special configure options. But these options must default to something under --prefix.
Comment 33 Volker Lendecke 2014-07-11 08:59:30 UTC
Assigning to Andreas who has brought up this topic initially
Comment 34 Volker Lendecke 2014-07-18 11:06:02 UTC
Setting to critical. With this new policy, Samba ignores --prefix by default for pidl. This makes it impossible to run "make install" as non-root. Also it is impossible to install more than one independent Samba installation in independent subdirectories.
Comment 35 Stefan Metzmacher 2014-07-18 12:51:54 UTC
(In reply to comment #34)
> Setting to critical. With this new policy, Samba ignores --prefix by default
> for pidl. This makes it impossible to run "make install" as non-root. Also it
> is impossible to install more than one independent Samba installation in
> independent subdirectories.

We have explicit --with-perl-lib-install-dir= and --with-perl-arch-install-dir=
as workarround. But we should fix this by comparing

INSTALL_FILES() should take a perl_fixup similar to python_fix in order
to inject an rpath to the installed binary. We could then
compare perl's vendorprefix with our prefix and only use the perl provided
default paths if they match.
Comment 36 Volker Lendecke 2014-07-18 13:00:35 UTC
(In reply to comment #35)

> We have explicit --with-perl-lib-install-dir= and --with-perl-arch-install-dir=
> as workarround. But we should fix this by comparing
> 
> INSTALL_FILES() should take a perl_fixup similar to python_fix in order
> to inject an rpath to the installed binary. We could then
> compare perl's vendorprefix with our prefix and only use the perl provided
> default paths if they match.

If nobody finds the time to do it before the release of 4.2, I would vote to not install pidl at all in the default build.
Comment 37 Jelmer Vernooij 2014-10-12 22:40:21 UTC
This was working reasonably well when we were just invoking Makefile.PL from waf. Why did we move to using waf to install these files?

Please don't remove the installation of Pidl. We rely on it in the Debian packages.
Comment 38 Andreas Schneider 2014-10-28 10:40:06 UTC
Michael, could you please backport the patches you created for this bug report? Thanks!
Comment 39 Michael Adam 2014-11-10 15:23:42 UTC
Created attachment 10423 [details]
possible patchset for v4-1-test

Patches for v4-1-test, cherry-picked from master.

This patchset is the complete story, including
the required fixes for the dependencies for SAMBA_GENERATOR.
It also moves all the changes for this defect to wafsamba
and restores wafadmin into the original state.

Note: In order to be able to cleanly pick the patches
I have picked four initial patches to docs/dynconfig
by Garming and Andrew.

Is this OK? Desirable? Undesirable?

Comments please ... Michael
Comment 40 Stefan Metzmacher 2014-11-12 10:54:45 UTC
Created attachment 10427 [details]
Final patchset for v4-1-test
Comment 41 Stefan Metzmacher 2014-11-12 10:55:18 UTC
Created attachment 10428 [details]
Final patchset for v4-0-test
Comment 42 Michael Adam 2014-11-14 11:23:47 UTC
Karo, please push the "Final patchset..." attachments to 4.1 and 4.0, respectively.

Thanks - Michael
Comment 43 Karolin Seeger 2014-11-17 20:21:03 UTC
(In reply to Michael Adam from comment #42)
Pushed to autobuild-v4-[0|1]-test.
Comment 44 Karolin Seeger 2014-11-24 20:09:50 UTC
(In reply to Karolin Seeger from comment #43)
Pushed to both branches.
Closing out bug report.

Thanks!