SA Bugzilla – Bug 2388
Revamp Makefile.PL
Last modified: 2003-09-12 10:15:33 UTC
Guys, our Makefile.PM is a complete mess and completely incompatible (I'm talking about all that installdir messing). I just talked to Michael Schwern on IRC and I think I/we found a solution. I'll put together a patch tomorrow. That one will have the consequence: * Most of the command line vars will go. * The packagers will need to use a recent ExtUtils::MakeMaker (6.11) which supports a directive DESTDIR for them. * There will be a file PACKAGING which describes the correct way of doing so. * Only packagers have to switch, for Joe Sixpack everything will stay as it is. * It will be done Right(tm) ie. the Perl way Sorry for this change coming so late but I really want to change this now so we dont have to carry all that PKG_* stuff around. And I dont want to have to expect that fragile kludges in there to break one day or another and have to release another version just to fix the build issues.
assigning to me, setting milestone, adding dependencies
I was just about to suggest a small addition to the current Makefile.PL. If I use 'perl Makefile.PL CONTACT_ADDRESS=' (on purpose), "spammassassin --lint" complains about the option "report_contact" without value in 10_misc.cf. The attached patches put a '#' in front of the option. Maybe you can include this in your rewrite.
Created attachment 1299 [details] replace 'report_contact' by a @@-variable
Created attachment 1300 [details] if CONTACT_ADDRESS is empty (or consists only of white space), put a # in front of report_contact
Patch is undere development. I'll see what I can do about the CONTACT stuff.
Created attachment 1304 [details] Patch against Makefile.PL Whoa, just dived back out of the depths of ExtUtils::MakeMaker. And here's the output. What has changed: * All those old variables are now obsolete and it will die with a reference to INSTALL and PACKAGING (those files I still have to update/create). * Packagers will need to install EU::MM 6.11 if they want to pre-build their stuff somewhere. * That's because EU::MM 6.11+ supports a variable DESTDIR which is simply prepended to all paths on make install. So if you want to build a package for /usr/local but want to pre-install it in /tmp/build you just say perl Makefile.PL PREFIX=/usr/local DESTDIR=/tmp/build * It now supports INSTALLDIRS=vendor (and INSTALLDIRS=perl even though that doesn't make too much sense) * It's a bit more intelligent about the perl to put into the #! line and determining the version. * I didn't look at the CONTACT_ADDRESS="" stuff yet. Please test that patch and report any bugs here.
*** Bug 2374 has been marked as a duplicate of this bug. ***
*** Bug 2386 has been marked as a duplicate of this bug. ***
Subject: Re: [SAdev] New: Revamp Makefile.PL > Guys, our Makefile.PM is a complete mess and completely incompatible (I'm > talking about all that installdir messing). I just talked to Michael Schwern > on IRC and I think I/we found a solution. I'll put together a patch tomorrow. > That one will have the consequence: > * Most of the command line vars will go. > * The packagers will need to use a recent ExtUtils::MakeMaker (6.11) which > supports a directive DESTDIR for them. > * There will be a file PACKAGING which describes the correct way of doing so. > * Only packagers have to switch, for Joe Sixpack everything will stay as it > is. > * It will be done Right(tm) ie. the Perl way So this MakeMaker supports a SYSCONFDIR concept as well? that's one problem with the current one -- perl doesn't have to know about /etc so MakeMaker doesn't seem to either. --j.
Yes, the SYSCONFDIR is now chosen FHS-compliant: /usr -> /etc /usr/local -> /etc /opt/... -> /etc/opt /foo/... -> /foo/.../etc The basedir still can be changed via SYSCONFDIR, DEFRULESDIR and LOCALRULESDIR are also available (now without underscores). The dir for shared stuff does now always work and not break from time to time (bug 2386 was the third time it was broken).
*** Bug 2399 has been marked as a duplicate of this bug. ***
Hmm... Justin, I just read your comment again and I think I misread it first. So, no MakeMaker doesn't (and probably never will) support SYSCONFDIR. Neither /usr/share. But the hacks I did to generate the dirs are now at the places where all that other constants/macros are generated and follow the EU::MM logic and do (hopefully) respect all possibilites.
I would like it if both Duncan and Theo could verify this fix as this may affect the Debian and Red Hat packages (and they can verify that it works on both). The packaging files for either or both might require changes too. As a rule, doing things the Perl way sounds fine to me.
I tested this now with 5.005_03, 5.6.1, 5.8.0 and 5.8.1-rc2 and all looks good. At least if you've got EU::MM 6.11 installed. As Michael Schwern pointed out to me, did some versions before (including the one from 5.8.1-rc2) already support DESTDIR but it was buggy; seems like they mixed something up with the prefixes: "Writing/tmp/sa-test/home/mss/projects/bugassassin/lib/perl5/site_perl/5.8.1/i686-linux/auto/Mail/SpamAssassin/.packlist" So in my local copy I added a die() if you try to use DESTDIR and its not supported and a warn() if it is supported but EU::MM is older than 6.11.
Created attachment 1308 [details] revised patch against Makefile.PL *grmbl* I did so much testing with special cases that I didn't notice that installing the standard way (without any set vars) didn't work if EU::MM prior to 6.11 was installed. Fixed in this patch (replaces the previous).
I haven't had a chance to look at this too much yet, but the main issue I have so far is that anyone installing into a location which isn't the final destination (likely anyone building packages, which does include end users) will have to upgrade their version of MM?
Yes, anybody who wants to install the stuff somewhere which isn't the final place has to use EU::MM >= 6.11. That should be only packagers; I can't see any reason for end users to install the stuff somewhere else and then copy it over. But you don't even have to upgrade your system's EU::MM; you just have to use that version. "Häh?" you ask? :) The docu I'm currently writing will include these instructions: 1. Go to http://search.cpan.org/search?module=ExtUtils::MakeMaker 2. Get the latest ExtUtils::MakeMaker package (maintained by Michael G Schwern); when this text was written the current version was 6.16. Save it in some temporary location: cd /tmp wget http://search.cpan.org/.../ExtUtils-MakeMaker-6.16.tar.gz [that url is too long for Bugzilla's input field) 3. Uncompress the received file: tar xvfz ExtUtils-MakeMaker-6.16.tar.gz 4. Now set the environment variable PERL5LIB so that it contains the subdirectory 'lib' of the extracted package: export PERL5LIB=`pwd`/ExtUtils-MakeMaker-6.16/lib:$PERL5LIB 5. Now build SpamAssassin. Perl will use the updated ExtUtils::MakeMaker as long as the environment variabe PERL5LIB is set correctly.
Subject: Re: Revamp Makefile.PL On Wed, Sep 03, 2003 at 11:50:51AM -0700, bugzilla-daemon@bugzilla.spamassassin.org wrote: > Yes, anybody who wants to install the stuff somewhere which isn't the final > place has to use EU::MM >= 6.11. That should be only packagers; I can't see > any reason for end users to install the stuff somewhere else and then copy it > over. Well, people who want to (re)build an SA package on their own box. Which is pretty much everyone w/ RPM.
Subject: Re: [SAdev] Revamp Makefile.PL > I haven't had a chance to look at this too much yet, but the main issue I have > so far is that anyone installing into a location which isn't the final > destination (likely anyone building packages, which does include end users) will > have to upgrade their version of MM? Good point -- that's not going to happen. --j.
Ok, I see three possible solutions: 1. The user building RPMs needs to have a EU::MM 6.11+ around -- as I described above is that not necessarily an updated system lib but can also be just a copy somewhere around. They would not even have to set the PERL5LIB, the Makefile.PL could look automagically in a directory where the user just had to copy the contents of the 'lib' dir of the EU::MM package. 2. The libs of EU::MM 6.16 could be included in the SRPM (possible?). that would increase the size of the SRPM by about 500k -- removing the unneeded platforms would make that about 350k. 3. I could hack support for DESTDIR into old EU::MMs via our Makefile.PL. That's not too hard but requires rewriting of the Makefile (just some search-and-replace of var names) so might break somewhere -- I currently know of two places/pattern which I have to exclude from rewriting but those might be different between EU::MM version. I checked all the shipped versions of the various Perls I have installed and cant find any difference but that doesn't mean that there actually is a deifferent version out there. The last solution is the most convenient for package builders but would shoot down some advantages my current rewrite has -- less hacks which might break some time or another. I'd personally prefer the first.
Created attachment 1312 [details] patch against spamassassin.spec and debian/rules to show off with the advantages of the new build system :o)
If you install MakeMaker 6.16 SA 2.55 doesn't build correctly anymore because MakeMaker > 6.06 doesn't provide PREFIX :-(. According to the Changes file PERLPREFIX is now the path prefix for the perl libraries. I wonder how many perl modules may fail due do this incompatibility...
What exactly are trying to do? EU::MM doesn't 6.06+ support make install PREFIX=/foo anymore but everywhere else PREFIX works. PREFIX overwrites PERLPREFIX, SITEPREFIX and VENDORPREFIX. INSTALLDIRS=site is the correct one for "normal" building (and the default), INSTALLDIRS=vendor is for packages and INSTALLDIRS=PERL is *only* for modules shipped with Perl and Perl istelf. For a description of that stuff (and the rationale for all those changes), please see the following urls: [1]http://archive.develooper.com/perl5-porters@perl.org/msg94113.html [2]https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=78053 [3]http://www.mail-archive.com/makemaker@perl.org/msg00779.html [4]http://www.debian.org/doc/packaging-manuals/perl-policy/ch-module_packages.html#s-vendor_dirs
I was testing your patches for the Makefile.PL in 2.60-rc3 with EU::MM 6.16 and happened to reinstall 2.55 as well. Try 'perl Makefile.PL' with SA 2.55 and EU::MM > 6.06 installed. It will install the rules in /share/spamassassin because PREFIX is undefined. Makefile: PREFIX = PERLPREFIX = /local/pkg ... # --- MakeMaker postamble section: INST_PREFIX = SYSCONFDIR = /etc INST_SYSCONFDIR = /etc PKG_DEF_RULES_DIR = $(PREFIX)/share/spamassassin DEF_RULES_DIR = $(INST_PREFIX)/share/spamassassin PKG_LOCAL_RULES_DIR = $(SYSCONFDIR)/mail/spamassassin LOCAL_RULES_DIR = $(INST_SYSCONFDIR)/mail/spamassassin INST_SITELIB = $(INSTALLSITELIB) The default installation of SA 2.55 depended on PREFIX being provided by EU::MM. I wonder whether there are other perl packages which do the same and might break with EU::MM > 6.06 (this is where the change happened). Maybe this was discussed before the change, I may take a look at those URLs you mentioned in the evening.
Oh, yeah sorry you're right (I thought you did set some PREFIX and that didn't work). That's another thing my patch fixes (it was even the reason why I went that far): In the new EU::MM PREFIX is only used to override the other three PREFIXes. If you dont set anything, it will be empty. So the Makefile.PL has to choose one of the (PERL|SITE|VENDOR)PREFIX ones. Which one is the right depends on the value of INSTALLDIRS, which defaults to 'site'.
well, I do like the new build system, I just dislike the requirement of the new MakeMaker since I know for certain that people will be trying to build their own packages (it's even the recommended method for RPM at least). if I can get some time this weekend, I'll boot up my vmware redhat installs and do some testing as to whether the patches will work. assuming they do, we need to decide whether or not we want/need to make the changes for 2.60.
I thought about it and I think hacking DESTDIR into old EU::MMs isn't that bad I thought. I'll try to put together a patch tomorrow.
I'm not inclined to approve any major changes to this for 2.6x. Is there some small fix we can do? This is the main thing holding up 2.60 right now (everything else is trivial and can be bumped to 2.61 if needed).
I tried to do this in smaller steps but I had to works around so many possible places where it could break that I somewhere got confused by my own many if() blocks and threw everything away to Do It Right.
Created attachment 1323 [details] revision 2 This patch replaces 1308, * adds DESTDIR support for old EU::MMs * actually makes this work with *very* old EU::MMs (5.005_03) which didn't set any of the (PERL|SITE|VENDOR)PREFIXes. The latter is a bug which would have to be considered also for "simpler" solutions -- I think this patch is now pretty well-done; I'd really not like at all to go back to the original and try to fix all the stuff I did till now with the old structure. A new patched tarball is available here: http://msquadrat.de/pub/softlib/spamassassin/Mail-SpamAssassin-2.60-rc3-mss3.tar.gz
Sorry -- I really can't +1 it as it stands, it hurts my head too much. First off, there's too many nonessential changes -- whitespace twiddling etc. -- which make the patch bigger and harder to read than it should be. It would be best to ignore those -- leave 'em to CVS later... + $prefix =~ s/\Q$(\E([^)]+)\Q)\E/$self->{$1} || ''/ge; # expand vars could we do that with a function at least? there's a lot of: + foreach (qw(SYSCONFDIR SHARED CONFIG)) { + my $v = sprintf('%s%s%s', + $_ ne 'SYSCONFDIR' ? 'INSTALL' : '', + $target || ($_ eq 'SYSCONFDIR' ? 'PERL' : ''), + $_ + ); + push(@code, qq{$v = $self->{$v}}); + push(@code, qq{DEST$v = \$(DESTDIR)\$($v)}) if $v =~ /^INSTALL/ and $mm_has_destdir; + } as Dan says, that's mixing code with data very heavily -- if there was some way to avoid generating the code on the fly like this, *even* if it meant a longer-in-terms-of-lines Makefile.PL, it would be much easier to comprehend (and +1).
+0.5: a few minor issues: mixing code and data is confusing. This is much easier to read: --- my @att_keys = qw(SYSCONFDIR DEFRULESDIR LOCALRULESDIR ENABLE_SSL CONTACT_ADDRESS RUN_RAZOR_TESTS RUN_RAZOR2_TESTS PERL_BIN PERL_WARN PERL_TAINT PERL_VERSION); foreach my $var (@att_keys) { $ExtUtils::MakeMaker::Recognized_Att_Keys{$var} = 1; } --- I can't make heads or tails of this and almost don't want to: foreach (qw(SHARED CONFIG LIB)) { $code .= sprintf('I_%s = $(%s%s%s)', $_, 'INSTALL', $target, $_, ) . "\n"; $code .= sprintf('B_%s = $(%s%s%s)', $_, $mm_has_destdir ? 'DESTINSTALL' : 'I_', $mm_has_destdir ? $target : '', $_, ) . "\n"; }
Sorry about the whitespace changes -- I forgot to use the switch for diff. I tried to do the other stuff as readable as possible with as less lines as possible ;-) I'll give it some more space, maybe that will relieve any headaches :)
Created attachment 1327 [details] the new Makefile.PL The whole new file. Makeing patches doesn't really make sense with that many changes. It's easier to use two editor to compare the files (and you all know how to use 'patch' ;-). I hope its now better to understand. I tested it with several different Perls and it looks good. A patch against INSTALL for the docu will follow. Of course it can break somewhere; that's why I had the diea to ship *two* Makefile.PLs. I see two possibilites: * Move the old Makefile.PL to Makefile-Old.PL and if somebody can't get his SA to build and asks on the list we can tell him "use Makefile-Old.PL but only if you don't have Perl >= 5.8.0 or EU::MM >= 6.06". * I check in the new Makefile.PL as Makefile-New.PL. When somebody calls the old one, the first thing he gets is a notice that it will switch to the new build syste now. If that does break, he should come back and say "no" (restrictions as above). If he doesn't say "no", it will exec() the new one.
Created attachment 1328 [details] the new file PACKAGING here's the documentation for all that stuff. I'll also add some references to that document to the appropriate parts of INSTALL.
Created attachment 1329 [details] a biiiig patch containing all the changes
+1 so malte can get some sleep ;)
+1 ditto.
committed. I keep my fingers crossed...
no no, we're back... $ perl Makefile.PL < /dev/null [...] $ make [...] cd spamd; ./configure --prefix="/usr" --sysconfdir="" --datadir="/usr/share/spamassassin" --enable-ssl="no" configure: error: expected an absolute directory name for --sysconfdir: make: *** [spamd/binaries.mk] Error 1 perl 5.6.1, MM 5.45
Question: is the installation process supposed to remove the -T flag from the top of sa-learn, spamassassin, etc. ?
Created attachment 1336 [details] suggested fix I really need sleep since I posted the following to the wrong bug and almost along with this patch ... after some digging, the problem is that by default: SYSCONFDIR = PERLSYSCONFDIR = $(SYSCONFDIR) SITESYSCONFDIR = $(SYSCONFDIR) VENDORSYSCONFDIR = $(SYSCONFDIR) which doesn't get very far. if I comment out line 837 in Makefile.PL: set_macro('SYSCONFDIR', "") unless get_macro('SYSCONFDIR'); I end up with: SYSCONFDIR = PERLSYSCONFDIR = /etc SITESYSCONFDIR = /etc VENDORSYSCONFDIR = /etc which looks a bit better. however, what I think you wanted was unless SYSCONFDIR is already set, calculate it from PREFIX, and leave the others defaulted ala: _set_macro_SYSCONFDIR('') unless get_macro('SYSCONFDIR'); which does produce what I expected: SYSCONFDIR = /etc PERLSYSCONFDIR = $(SYSCONFDIR) SITESYSCONFDIR = $(SYSCONFDIR) VENDORSYSCONFDIR = $(SYSCONFDIR) patch to be up in a second. :)
+1 if okay with Malte, looks good to me
Created attachment 1337 [details] better fix :) That's a leftover of my try to make the SYSCONDIR macro more intelligent yesterday night. It must not use defined() but (also) check for empty strings. Thats where I got alsmost desparate yesterday because depending on the version of EU::MM, unset vars are sometimes undef and sometimes an empty string :-( Theo, your suggested fix also only works with your oldish EU::MM -- that's exactly one of the problems fixed by this beasty: In newer EU::MMs PREFIX is empty if its not set explicitly. Only the (PERL|SITE|VENDOR)PREFIXes are computed. So I followed that behaviour with SYSCONFDIR to avoid introducing that version-dependent problem. So your second suggestion was the correct solution: SYSCONFDIR = PERLSYSCONFDIR = /etc SITESYSCONFDIR = /etc VENDORSYSCONFDIR = /etc where (PERL|SITE|VENDOR)SYSCONFDIR is determined from (PERL|SITE|VENDOR)PREFIX (which can be different) and SYSCONFDIR overwriting all of then (like PREFIX to its counterparts). Quite complicated but that's how EU::MM works now *sigh* Daniel: I didn't change anything in the preprocessor, did the scripts keep the -T before? (I'd say it shouldn't "remove" the flag, I'll attach a patch to make it do this in a moment.)
Created attachment 1338 [details] patch for taint mode Here's another patch which enables the taint mode in the installed apps per default, too. Can be disabled with PERL_TAINT=no. It also fixes a small bug in Makefile.PL which made setting PERL_WARN and PERL_TAINT not work (because yesno() is in another package and I actually never tried those stupid options ;)
with the patch, I can't build RPMs now ... Executing(%install): /bin/sh -e /home/felicity/src/RPMS/BROOT/rpm-tmp.70485 + umask 022 + cd /home/felicity/src/RPMS/BUILD + cd Mail-SpamAssassin-2.60 + '[' /home/felicity/src/RPMS/BROOT/spamassassin-root '!=' / ']' + rm -rf /home/felicity/src/RPMS/BROOT/spamassassin-root + make prefix=/home/felicity/src/RPMS/BROOT/spamassassin-root/usr exec_prefix=/home/ felicity/src/RPMS/BROOT/spamassassin-root/usr bindir=/home/felicity/src/RPMS/BROOT/ spamassassin-root/usr/bin sbindir=/home/felicity/src/RPMS/BROOT/spamassassin-root/usr/sbin sysconfdir=/home/felicity/src/RPMS/BROOT/spamassassin-root/etc datadir=/home/felicity/src/ RPMS/BROOT/spamassassin-root/usr/share includedir=/home/felicity/src/RPMS/BROOT/ spamassassin-root/usr/include libdir=/home/felicity/src/RPMS/BROOT/spamassassin-root/usr/ lib libexecdir=/home/felicity/src/RPMS/BROOT/spamassassin-root/usr/libexec localstatedir=/ home/felicity/src/RPMS/BROOT/spamassassin-root/var sharedstatedir=/home/felicity/src/RPMS/ BROOT/spamassassin-root/usr/com mandir=/home/felicity/src/RPMS/BROOT/spamassassin- root/usr/share/man infodir=/home/felicity/src/RPMS/BROOT/spamassassin-root/usr/share/info install Warning: You do not have permissions to install into /usr/lib/perl5/site_perl/5.6.1/i386-linux at / usr/lib/perl5/5.6.1/ExtUtils/Install.pm line 85. Cannot forceunlink /usr/lib/perl5/site_perl/5.6.1/Mail/SpamAssassin.pm: Permission denied at / usr/lib/perl5/5.6.1/File/Find.pm line 525 make: *** [pure_site_install] Error 255 error: Bad exit status from /home/felicity/src/RPMS/BROOT/rpm-tmp.70485 (%install)
poking around some more: install :: all pure_install doc_install conf__install data__install pure_install is what dies with the permission issue. > make doc_install /bin/sh: /perllocal.pod: No such file or directory make: [doc_site_install] Error 1 (ignored) Appending installation info to /perllocal.pod conf__install and data__install work as expected.
Created attachment 1339 [details] another patch on top the DESTDIR hack didn't work for the MAN* dirs (because it has numbers :-/)
+1 for 1337 and 1339. lets me build RPMs now. :)
Created attachment 1340 [details] patch for PERL_WARN and PERL_TAINT as discussed on IRC: let's enable taint and warn perl default
+1 for 1340
Created attachment 1343 [details] another doc fix on top
Comment on attachment 1329 [details] a biiiig patch containing all the changes applied already
+1 1337 1339 1340 1343 :)
+1 for 1337, 1339, 1340, and 1343
+1 I haven't tested it, but 1343 looks ok
Whoa. That beasty doesn't should really work now.
sorry. the NO_META stuff still occurs with rc4. which should have the patch installed.
let's discuss this in bug 2434 (there are three bugs marked as duplicates of this one which also change state every time this down *sigh*)
*** Bug 1966 has been marked as a duplicate of this bug. ***