Re: [PATCH] aclocal: handle ACLOCAL_PATH environment variable

[Stefano Lattarini]

On Wednesday 03 November 2010, Paolo Bonzini wrote:
> On 11/03/2010 04:24 PM, Stefano Lattarini wrote:
> >> +  # Add any directory listed in the `ACLOCAL_PATH' environment
> >> +  # variable.
> >> +  if (defined $ENV{"ACLOCAL_PATH"})
> >> +    {
> >> +      foreach my $dir (split /:/, $ENV{"ACLOCAL_PATH"})
> > Shouldn't we use address@hidden@' here instead of `:', for better
> > portability to windows?
> 
> Yes, I think so.
> 
> >> +        {
> >> +          push (@system_includes, $dir) if $dir ne ''&&  -d $dir;
> > Hmmm... IMHO the right place where to add directories from `ACLOCAL_PATH'
> > is address@hidden', not address@hidden'.  Also, the testcase should
> > verify that extra directories from `-I' command line options take
> > precedence over the ones from `ACLOCAL_PATH', and that these latter
> > ones take precedence over the extra directories from system includes.
> 
> This is true (and, I think, handled by pushing at the end of 
> @system_includes).
> 
> On the other hand, I considered @user_includes as "per-package includes" 
> and @system_includes as "installation-wide includes" (albeit per 
> account), which means ACLOCAL_PATH would count as a system include 
> directory.
Yes, makes sense.  Still, having "per-account" includes in @system_includes
doesn't seem much clear to me.  Maybe we should procees with a rename like
@system_includes => @global_includes and @user_includes => @local_includes.
But that's bikeshedding, and for a follow-up patch anyway; and I'm now OK
with your patch if the tests pass (by the way, the testcase I provided are
buggy, since they use `:' rather than `$PATH_SPERATOR' in definition of
ACLOCAL_PATH; that should be fixed).
 
> > I've attached two tentative testcases checking for the behaviour I'd
> > like to see from ACLOCAL_PATH.
> >
> > But as an afterthought, I see that installing third party macros in
> > directories provided by `ACLOCAL_PATH' when the `--install' option
> > is used is probably a bad idea.  Any idea about what the best way to
> > address this would be?
> 
> Keep it into @system_includes? :)
Right.  Refactoring and renamings can be done as follow-ups, if needed.

Thanks,
   Stefano