[PATCH modular 03/13] build.sh: allow user to specify an alternate bin directory

[Gaetan Nadon]

On Wed, 2010-12-29 at 15:51 -0500, Trevor Woerner wrote:

> For this entire patch series I still can't understand why we need to
> define an explicit _SET variable which defines whether or not the base
> variable is defined, or not.
> 
> For every one of the new variables you have defined you have included
> the following:
> 
> > +# States if the user has exported BINDIR
> > +if [ X"$BINDIR" != X ]; then
> > +    BINDIR_SET=yes
> > +fi
> 
> And then you subsequently check to see if (using the specific
> variables for this specific patch) BINDIR_SET is defined to decide
> whether or not to use the value of BINDIR.
> 
> But why?
> 
> If BINDIR is not defined then BINDIR_SET will not be defined. So if
> you want to check whether BINDIR is defined why not just check to see
> if BINDIR is defined (instead of creating a new variable (BINDIR_SET)
> and checking whether it is defined or not)?
> 
> Maybe there's something about what you're doing that I don't
> understand. But if you're defining BINDIR_SET only if BINDIR is
> defined and then using BINDIR_SET to indicate whether or not BINDIR is
> defined then I think you're adding complexity for no reason.
> 
> PS. I have nothing against _what_ is being done, just the way in which
> it is being done.


I think I found a way to expose the need for this _SET variable. Perhaps
you can then help
me make it clearer as others will most likely have the same question, or
risk breaking
it when maintaining the script.

        
        # States if the user has exported BINDIR
        if [ X"$BINDIR" != X ]; then
            BINDIR_SET=yes
        fi

The comment is important. This variable is to tell us if the user has
exported an environment
variable called BINDIR, not to tell if it is empty or not.

        
        # Set the default value for BINDIR
        if [ X"$BINDIR_SET" = X ]; then
            BINDIR=$EPREFIX/bin
        fi

Th above part says: if the user has not exported the BINDIR variable,
then assign it a default value.
So far, no need for BINDIR_SET, I agree. Note that from now on, BINDIR
is always set, always.


    # Set the path so that locally built apps will be found and used
    PATH=${DESTDIR}${BINDIR}${PATH+:$PATH}
    export PATH

The above line needs BINDIR to hold a value, always.


    ${BINDIR_SET:+--bindir="$BINDIR"}

This line above says: if the user has exported the BINDIR variable, add
"--bindir=$BINDIR"
to the configure command line. It also says: do not add --bindir if the
user has not exported
the BINDIR environment variable. Recall that BINDIR will always have a
value, always.

There may be a more elegant way of handling this situation. The variable
should perhaps
be names BINDIR_USER_SET.

In the case of PREFIX, it can bet set by the user as an env var or on
the command line.
There are 2 places where PREFIX_SET is set.

I appreciate you are not giving up, having code that works is good,
having maintainable code that works is even better.

Gaetan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20101229/4bd442cd/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20101229/4bd442cd/attachment-0001.pgp>