Bug 59705 - Fix out of source builddir - autogen
Summary: Fix out of source builddir - autogen
Status: RESOLVED DUPLICATE of bug 94391
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-01-22 10:11 UTC by Alban Browaeys
Modified: 2016-09-01 15:13 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Fix out of source builddir - autogen (1.09 KB, patch)
2013-01-22 10:11 UTC, Alban Browaeys
Details | Splinter Review
Fix out of source builddir - autogen (1.14 KB, patch)
2013-03-30 17:16 UTC, Alban Browaeys
Details | Splinter Review
Fix out of source builddir - autogen (1.15 KB, patch)
2013-04-09 08:04 UTC, Alban Browaeys
Details | Splinter Review

Description Alban Browaeys 2013-01-22 10:11:45 UTC
Created attachment 73439 [details] [review]
Fix out of source builddir - autogen

source tools (autoreconf, gtkdocize, glib-gettextize) are run
from source directory while build tools (configure) are run from
builddir (while still located in the sourcedir).
Comment 1 Simon McVittie 2013-01-28 14:07:08 UTC
Is there an expectation that autogen.sh can always be run in an out-of-tree build directory, via its absolute path to the copy in the source directory?

Workaround: NOCONFIGURE=1 ./autogen.sh in your source directory, then configure in each build directory. That's what I do when building several out-of-tree copies with differing configuration - it also avoids repeating autoreconf.
Comment 2 Simon McVittie 2013-01-28 14:23:53 UTC
Comment on attachment 73439 [details] [review]
Fix out of source builddir - autogen

Review of attachment 73439 [details] [review]:
-----------------------------------------------------------------

::: autogen.sh
@@ +1,5 @@
>  #!/bin/sh
>  set -e
>  
> +srcdir=`dirname $0`
> +test -z "$srcdir" && srcdir=.

We're running under 'set -e', so it's not absolutely clear why this won't make the script abort in the case where $srcdir is nonempty. In practice, I think it usually won't abort... but I don't know how portable that is.

POSIX says commands in an && or || list are exempt from 'set -e', and dash(1) says the left operand of && or || is exempt from 'set -e'.

I also think using the short-circuiting behaviour of || and especially && as a pseudo-conditional is less clear than using a real conditional.

I'd feel happier about this if it was spelled out explicitly:

    if test -z "$srcdir"; then
        srcdir=.
    fi

I was slightly worried about whether this works correctly on certain non-Linux systems where $0 is just the basename, but I think it does, because dirname will return either "." or the empty string, either of which is fine - it just won't support out-of-tree builds in the way you request on such systems.

As general good shell practice, I would prefer it properly quoted

    srcdir=`dirname "$0"`

so it does the right thing if $0 contains spaces (although I think Autoconf/Automake explicitly don't support srcdir or builddir containing spaces anyway).

@@ +23,2 @@
>  autoreconf -i -f
> +)

From general defensive shell programming, I'd prefer "$srcdir" properly quoted, as I said for "$0".

Also, I'd prefer consistent semicolon/no-semicolon and newline positioning, in whichever of these three forms you prefer:

    (
    cd "$srcdir"
    gtkdocize
    autoreconf -i -f
    )

    (
        cd "$srcdir"
        gtkdocize
        autoreconf -i -f
    )

    ( cd "$srcdir" && gtkdocize )
    ( cd "$srcdir" && autoreconf -i -f )

(unless there's a specific reason for the way you wrote it, in which case, please say).

@@ +40,4 @@
>  fi
>  
>  if test $run_configure = true; then
> +    $srcdir/configure "$@"

Again, I'd prefer "$srcdir/configure" quoted.
Comment 3 Alban Browaeys 2013-03-30 17:16:34 UTC
Created attachment 77234 [details] [review]
Fix out of source builddir - autogen

fixed and strengthened (dirname -- ) and full path srcdir
Comment 4 Simon McVittie 2013-04-02 13:40:24 UTC
Comment on attachment 77234 [details] [review]
Fix out of source builddir - autogen

Review of attachment 77234 [details] [review]:
-----------------------------------------------------------------

::: autogen.sh
@@ +1,4 @@
>  #!/bin/sh
>  set -e
>  
> +srcdir="`dirname -- \"$0\"`"

I'm not sure how portable "dirname -- x" is: -- is a GNUism.

I'm pretty sure this doesn't need to be this complicated, because word splitting isn't performed on the result of `` when it's the right-hand side of an assignment:

    $ a=`echo "b c d"`
    $ echo $a
    b c d

so... srcdir=`dirname "$0"` perhaps?

It looks as though GNOME projects typically do something like this:

    test -n "$srcdir" || srcdir=`dirname "$0"`
    test -n "$srcdir" || srcdir=.

(I usually prefer if/fi rather than ||, &&, but perhaps this is a case simple enough that || is OK.)

@@ +1,5 @@
>  #!/bin/sh
>  set -e
>  
> +srcdir="`dirname -- \"$0\"`"
> +srcdir="`( cd \"$srcdir\"; pwd )`"

Is there a special reason why srcdir needs to be made absolute? I don't see anything here that wouldn't work if it was relative, and I believe Autotools is meant to support relative paths to the srcdir, like this usage sometimes seen in Debian packages:

    ( mkdir build && cd build && ../configure )
    ( mkdir build2 && cd build2 && ../configure --disable-badgers )
    make -C build
    make -C build2
Comment 5 Alban Browaeys 2013-04-09 08:04:56 UTC
Created attachment 77653 [details] [review]
Fix out of source builddir - autogen

thanks . De over engineered the last fix of the patch.
Comment 6 George Kiagiadakis 2016-09-01 15:13:36 UTC
Bug 94391 was also about the same thing and I hadn't noticed your patch and the review here before merging. I have now modified the previous patch to match this one, as it looks more portable & clean.

*** This bug has been marked as a duplicate of bug 94391 ***


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.