[Bug 6131] New: Makefile.PL is doing evil things to detect DESTDIR

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131

           Summary: Makefile.PL is doing evil things to detect DESTDIR
           Product: Spamassassin
           Version: 3.2.5
          Platform: All
               URL: https://rt.cpan.org/Public/Bug/Display.html?id=46807
        OS/Version: All
            Status: NEW
          Severity: major
          Priority: P5
         Component: Building & Packaging
        AssignedTo: dev@...
        ReportedBy: schwern@...


A ticket was reported to MakeMaker about spamassassin built with rpm trying to
install straight into /usr rather than /tmp.  The saga is recorded here:
https://rt.cpan.org/Public/Bug/Display.html?id=46807

Long story short: the Makefile.PL thinks MakeMaker doesn't support DESTDIR
because its looking at undocumented global MakeMaker variables which
**SURPRISE!** changed.  Please don't do that.  It means I have to spend an hour
tracking down your bugs.  If you find yourself having to do some awful hack to
MakeMaker at least send us a heads up at makemaker@....

I would recommend just requiring a newer version of MakeMaker and tearing out
all the hackery to support old bugs and missing features.  Its been over six
years since DESTDIR support was introduced.  Even a stock 5.8.1 supports it.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #1 from Karsten Bräckelmann <guenther@...>  2009-06-09 15:50:26 PST ---
(In reply to comment #0)
> It means I have to spend an hour tracking down your bugs.

That's the usual way a properly filed bug is born. Sorry. ;)

> I would recommend just requiring a newer version of MakeMaker and tearing out
> all the hackery to support old bugs and missing features.  Its been over six
> years since DESTDIR support was introduced.  Even a stock 5.8.1 supports it.

Patches highly appreciated, if you would be so nice, Michael. :)

Hmm, since 5.8.1, you say. What again was the min Perl version required? I
recall there was some change agreed upon about a year ago or something...

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131


Karsten Bräckelmann <guenther@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|Undefined                   |3.3.0




--- Comment #2 from Karsten Bräckelmann <guenther@...>  2009-06-09 15:54:06 PST ---
Forgot to set the Target Milestone, definitely good to have for 3.3. Probably
even for 3.2.6 if that version ever will be released and a potential patch
doesn't up the Perl dependency.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #3 from Justin Mason <jm@...>  2009-06-09 16:13:16 PST ---
SA's min perl version is 5.6.1 iirc....

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #4 from admin@...  2009-06-09 16:19:19 PST ---
There are a bunch of duplicates in the system all getting at the same problem.
See, for example, bug 5904, bug 5944, bug 5963.

The Makefile.PL needs to be fixed in how it handles DESTDIR.  Michael (of
MakeMaker) points to the hack where $ExtUtils::MakeMaker::Recognized_Att_Keys
is altered to include the keys that SA wants it to include.  This hack is a
work-around that breaks as of version 6.43 of MakeMaker, when that formerly
gloabl variable was made a lexical.

I don't know make well enough to create a patch for this myself, but it
shouldn't be too hard to fix by perhaps requiring a later version of MakeMaker
and taking out all the DESTDIR hackery.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #5 from admin@...  2009-06-09 16:28:07 PST ---
(In reply to comment #3)
> SA's min perl version is 5.6.1 iirc....

SA can keep a min perl version of 5.6.1, I think, provided it has a min
MakeMaker  version of 6.11(?) rather than 5.45.  At a MINIMUM, the logic of the
hack can be fixed: shouldn't the logic be:

$mm_has_destdir      = $mm_knows_destdir || $mm_has_good_destdir

rather than:

$mm_has_destdir      = $mm_knows_destdir || $mm_needs_destdir;

? It doesn't make sense that the "has" is defined by "knows" or "needs" (the
latter suggests the opposite!) rather than "knows" or "has_good". I think that
logic may be the root of the hack's failure.  So even a short fix to address
the hack might be a small step, short of finding a better work-around that
isn't targeting MakeMaker's private internals.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #6 from Karsten Bräckelmann <guenther@...>  2009-06-09 16:45:03 PST ---
(In reply to comment #3)
> SA's min perl version is 5.6.1 iirc....

As per Makefile.PL and the spec file, yes. That would be since 2003-09-29. Yay.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #7 from Michael Schwern <schwern@...>  2009-06-09 16:46:50 PST ---
(In reply to comment #1)

> (In reply to comment #0)
> > It means I have to spend an hour tracking down your bugs.
>
> That's the usual way a properly filed bug is born. Sorry. ;)
>
> > I would recommend just requiring a newer version of MakeMaker and tearing out
> > all the hackery to support old bugs and missing features.  Its been over six
> > years since DESTDIR support was introduced.  Even a stock 5.8.1 supports it.
>
> Patches highly appreciated, if you would be so nice, Michael. :)

Sorry, I don't use SA or Redhat.  It ain't my bug, it landed in my lap because
I maintain MakeMaker.  Just passing it along.

The latest MakeMaker will still install on 5.6.1, so you can require an updated
MakeMaker without breaking 5.6.1 compatibility.  I'd be pretty shocked if
anyone is seriously running 5.6.1 and hasn't already upgraded MakeMaker.

For reference here's Perl versions and the MakeMaker versions they ship with.

  5.006      5.45      
  5.006001   5.45      
  5.006002   6.17      
  5.007003   5.48_03  
  5.008      6.03      
  5.008001   6.17      
  5.008002   6.17      
  5.008003   6.17      
  5.008004   6.17      
  5.008005   6.17      
  5.008006   6.17      
  5.008007   6.17      
  5.008008   6.30      
  5.008009   6.48      
  5.009      6.17      
  5.009001   6.21_02  
  5.009002   6.25      
  5.009003   6.30_01  
  5.009004   6.30_02  
  5.009005   6.36      
  5.01       6.42      
  5.010000   6.42

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #8 from admin@...  2009-06-09 16:58:13 PST ---
Created an attachment (id=4459)
 --> (https://issues.apache.org/SpamAssassin/attachment.cgi?id=4459)
fix of the DESTDIR hack logic

This fixes the logic of the DESTDIR hack in the Makefile.PL, and with cursory
testing appears to fix the bad behavior. Should apply a similar fix to the
previous 3.1.x version as well, if it's to be made available still on
spamassassin.apache.org.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131


Justin Mason <jm@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P5                          |P2




--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131


Justin Mason <jm@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P2                          |P1




--- Comment #9 from Justin Mason <jm@...>  2009-06-29 04:19:30 PST ---
we should fix this for 3.3.0

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #10 from Justin Mason <jm@...>  2009-06-30 09:21:41 PST ---
ok, first draft:

http://taint.org/x/2009/bug6131.part1.patch

look sensible?

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #11 from Mark Martinec <Mark.Martinec@...>  2009-06-30 10:35:20 PST ---
> ok, first draft:
> http://taint.org/x/2009/bug6131.part1.patch
> look sensible?

Lots of deletions, good!

Applies cleanly, works out of the box for me, examining the patch
looks fine to me, although I'm not too intimate with Makefile.PL.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #12 from Justin Mason <jm@...>  2009-06-30 15:04:38 PST ---
: 649...; svn commit -m "bug 6131: simplify Makefile.PL and fix a bug in
DESTDIR support by increasing the minimum ExtUtils::MakeMaker version required
to 6.17" Makefile.PL
Sending        Makefile.PL
Transmitting file data .
Committed revision 789978 ( https://svn.apache.org/viewcvs.cgi?view=rev&rev=789978 ).


I think this still may need a review to check if the PACKAGING file is still
right...

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #13 from Justin Mason <jm@...>  2009-07-01 01:55:11 PST ---
actually, more than that, I'm going to write some tests for this. should be
easily testable

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131


Justin Mason <jm@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED




--- Comment #14 from Justin Mason <jm@...>  2009-07-02 15:49:58 PST ---

: 186...; svn commit -m "bug 6131: add test suite for Makefile.PL parameters" t
MANIFEST
Sending        t/SATest.pm
Adding         t/make_install.t
Transmitting file data ..
Committed revision 790753 ( https://svn.apache.org/viewcvs.cgi?view=rev&rev=790753 ).


(that t file is SVN-only btw.)

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131


Mark Martinec <Mark.Martinec@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |




--- Comment #15 from Mark Martinec <Mark.Martinec@...>  2009-08-04 17:24:27 PST ---
Hmm, I noticed this today, Perl 5.10.0, SA trunk.
The ExtUtils::MakeMaker 6.42 reports a warning,
the ExtUtils::MakeMaker 6.54 is happy.

Anything to worry about? Things appear to work fine anyway.


perl Makefile.PL
...
  optional module missing: Razor2
  optional module missing: Net::Ident
  warning: some functionality may not be available,
  please read the above report before continuing!

  WARNING: META_MERGE is not a known parameter.
  'META_MERGE' is not a known MakeMaker parameter name.
  Writing Makefile for Mail::SpamAssassin
  Makefile written by ExtUtils::MakeMaker 6.42


perl Makefile.PL
  ...
  optional module missing: Razor2
  optional module missing: Net::Ident
  warning: some functionality may not be available,
  please read the above report before continuing!  

  Writing Makefile for Mail::SpamAssassin
  Makefile written by ExtUtils::MakeMaker 6.54

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131





--- Comment #16 from Michael Schwern <schwern@...>  2009-08-04 18:22:14 PST ---
META_MERGE was added in MakeMaekr 6.46.  So you have to wrap it in a version
check or suffer a minor warning.

    ($ExtUtils::MakeMaker::VERSION < 6.46 ? () : (META_MERGE => {
        resources => {...}
    })),

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131


Justin Mason <jm@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |FIXED




--- Comment #17 from Justin Mason <jm@...>  2009-08-05 02:14:26 PST ---
(In reply to comment #16)
> META_MERGE was added in MakeMaekr 6.46.  So you have to wrap it in a version
> check or suffer a minor warning.
>
>     ($ExtUtils::MakeMaker::VERSION < 6.46 ? () : (META_MERGE => {
>         resources => {...}
>     })),

gotcha.  I'd been meaning to go back through the revs to figure out when that
was, but asking you would probably have been easier. ;)

: 216...; svn commit -m "bug 6131: META_MERGE is a recent addition to
ExtUtils::MakeMaker; don't add it if the MM version is less than 6.46"
Makefile.PL
Sending        Makefile.PL
Transmitting file data .
Committed revision 801099 ( https://svn.apache.org/viewcvs.cgi?view=rev&rev=801099 ).

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6131

Mark Martinec <Mark.Martinec@...> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |c.schiffler@...
                   |                            |e

--- Comment #18 from Mark Martinec <Mark.Martinec@...> 2009-09-18 17:14:42 PDT ---
*** Bug 5904 has been marked as a duplicate of this bug. ***

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.