Created attachment 5299 [details]
Adds args to Makefile creation to isolate rules & config inside test installation, eliminates pointless 'foo' subdirectory

See Bug 7005 for a verbose explanation. 

This patch does not fix the dubious choice of completely rebuilding an instance of SA for the test, but it isn't clear to me how to test compilation with the real build.

The use of /foo is likely to insure it never accidentally installs overtop of a real install.  

The installation of a fresh copy is then likely to get basic set of rules to compile.

I'm not sure I like this patch which just gets rid of foo.

Can you elaborate more on what problem this patch fixes for you because as discussed a bit on 7005, one of the bigger issues I have is in recreating some of the issues others are seeing in this matter.  I'd like to be clear on exactly which problem we are working to solve ;-)

(In reply to Kevin A. McGrail from comment #3)
> The use of /foo is likely to insure it never accidentally installs overtop
> of a real install.  

That is already assured. $instdir is defined as a subdirectory of the CWD of the test (i.e. t/log/d.sa_compile/) and is forcibly removed and recreated in preparation for the fresh install anyway. 

> The installation of a fresh copy is then likely to get basic set of rules to
> compile.

If I'm reading the test correctly, it defines a small set of rules to test compilation. 

> I'm not sure I like this patch which just gets rid of foo.

That's NOT all it does. That's just the change hitting the most lines, as minor cleanup of sloppy code often does. The critical bug fix is the change in line 129. 
 
> Can you elaborate more on what problem this patch fixes for you because as
> discussed a bit on 7005, one of the bigger issues I have is in recreating
> some of the issues others are seeing in this matter.  I'd like to be clear
> on exactly which problem we are working to solve ;-)

If the test is run anywhere under /opt (i.e. if the code tree and hence  $instdir starts with /opt/, as documented in the top-level README) the test either:

(1) pollutes the real system root by creating and putting files into /etc/opt/ and /var/opt/ 
OR
(2) fails because it cannot create or write to /etc/opt/ and/or /var/opt/. 

This is caused by the fact that the test creates a pristine build tree and does a completely fresh installation using whatever $instdir/foo/ happens to be as the PREFIX in the 'perl Makefile.PL' command, ignoring the fact that PREFIX=/opt/$DIR is a special case that forces some installation directories into the system's real root, *OUTSIDE* of PREFIX. 

This is fixed in my version by explicitly passing values for the escapee directories to the build configuration command which match what they would be if the test was being run under any top-level directory other than /opt. 

ALSO:

In working with the test code, I found "$instdir/foo" everywhere but never any clue to why 'foo' existed and never the creation of anything other than 'foo' inside $instdir. There's no clue in the SVN record of why 'foo' exists either. It's pure sloppy code-clutter, an opportunity for future tinkerers to make a typo or misunderstand what $instdir is: it's NOT the installation directory: that's foo. With the way automated build & packaging systems (e.g. MacPorts but not only MacPorts) tend to work, the path to $instdir is likely to be quite long anyway, and it is imprudent to add to that length unnecessarily, unless you want to test whether anyone still has a 256-char limit on paths or environment variables or whatever. 

AND FINALLY:

As I said in 7005 and above here, I'm highly suspicious of the design of this test, because it does a build and install of SA for the test that is entirely distinct from the build that will actually be installed, except that it has done a 'make tardist' from the existing build to get its own clean build tree. The point of that is an encapsulated install world, and my  fix to line 129 assures that, but there remains the problem of not testing the software that has been built prior to the test and may be installed for real. I also have no brilliant ideas for how to fix that.

Taking this myself to get the fix into 3.4.2

Bill, there is another patch in flight on this.  See 7181 which is fixed in trunk and awaiting a commit for 3.4 branch.  Does that fix your issue?

(In reply to Kevin A. McGrail from comment #6)
> Bill, there is another patch in flight on this.  See 7181 which is fixed in
> trunk and awaiting a commit for 3.4 branch.  Does that fix your issue?

No, I've already looked at that possibility. I also did build+test cycles with the patch in place and confirmed that it works on both trunk and 3.4

Committed to trunk: revision 1791043

Committed to 3.4 branch: revision 1791044

Rewrite of sa_compile in r1831429 regressed the fix to directories when PREFIX starts with /opt in r1791043. Reopening to re-fix

Upon further investigation... 

This is a different problem. I will open a new bug.

(In reply to Bill Cole from comment #9)
> Upon further investigation... 
> 
> This is a different problem. I will open a new bug.

Rather: see Bug #7413 for the extant problem.