All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tools: don't expand prefix and exec_prefix too early
@ 2012-08-08 14:26 Jan Beulich
  2012-08-08 14:39 ` Jan Beulich
  2012-08-08 15:10 ` Matt Wilson
  0 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2012-08-08 14:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Ian Campbell, Matt Wilson

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

A comment in tools/configure says that it is intended for these to be
command line overridable, so they shouldn't get expanded at configure
time.

The patch is fixing tools/m4/default_lib.m4 as far as I can see myself
doing this, but imo it is flawed altogether and should rather be
removed:
- setting prefix and exec_prefix to default values is being done later
  in tools/configure anyway
- setting LIB_PATH based on the (non-)existence of a lib64 directory
  underneath ${exec_prefix} is plain wrong (it can obviously exist on a
  32-bit installation)
- I wasn't able to locate any use of LIB_PATH
(I did see IanC's comment in c/s 25594:ad08cd8e7097 that removing it
supposedly causes other problems, but I don't see how that would
happen).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
This will require tools/configure to be re-generated.

--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -1,5 +1,6 @@
 # Prefix and install folder
-PREFIX              := @prefix@
+prefix              := @prefix@
+PREFIX              := $(prefix)
 exec_prefix         := @exec_prefix@
 LIBDIR              := @libdir@
 
--- a/tools/m4/default_lib.m4
+++ b/tools/m4/default_lib.m4
@@ -1,14 +1,19 @@
 AC_DEFUN([AX_DEFAULT_LIB],
-[AS_IF([test "\${exec_prefix}/lib" = "$libdir"],
-    [AS_IF([test "$exec_prefix" = "NONE" && test "$prefix" != "NONE"],
-        [exec_prefix=$prefix])
-    AS_IF([test "$exec_prefix" = "NONE"], [exec_prefix=$ac_default_prefix])
-    AS_IF([test -d "${exec_prefix}/lib64"], [
+[AS_IF([test "\${exec_prefix}/lib" = "$libdir"], [
+    AS_IF([test "$prefix" = "NONE"], [prefix=$ac_default_prefix])
+    AS_IF([test "$exec_prefix" = "NONE"], [exec_prefix='${prefix}'])
+    AS_IF([eval test -d "${exec_prefix}/lib64"], [
         LIB_PATH="lib64"
     ],[
         LIB_PATH="lib"
     ])
 ], [
     LIB_PATH="${libdir:`expr length "$exec_prefix" + 1`}"
+    AS_IF([test -z "${libdir##\$\{exec_prefix\}/*}"], [
+        LIB_PATH="${libdir:15}"
+    ])
+    AS_IF([test -z "${libdir##\$exec_prefix/*}"], [
+        LIB_PATH="${libdir:13}"
+    ])
 ])
 AC_SUBST(LIB_PATH)])




[-- Attachment #2: tools-cfg-libdir-x86_64.patch --]
[-- Type: text/plain, Size: 2154 bytes --]

A comment in tools/configure says that it is intended for these to be
command line overridable, so they shouldn't get expanded at configure
time.

The patch is fixing tools/m4/default_lib.m4 as far as I can see myself
doing this, but imo it is flawed altogether and should rather be
removed:
- setting prefix and exec_prefix to default values is being done later
  in tools/configure anyway
- setting LIB_PATH based on the (non-)existence of a lib64 directory
  underneath ${exec_prefix} is plain wrong (it can obviously exist on a
  32-bit installation)
- I wasn't able to locate any use of LIB_PATH
(I did see IanC's comment in c/s 25594:ad08cd8e7097 that removing it
supposedly causes other problems, but I don't see how that would
happen).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
This will require tools/configure to be re-generated.

--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -1,5 +1,6 @@
 # Prefix and install folder
-PREFIX              := @prefix@
+prefix              := @prefix@
+PREFIX              := $(prefix)
 exec_prefix         := @exec_prefix@
 LIBDIR              := @libdir@
 
--- a/tools/m4/default_lib.m4
+++ b/tools/m4/default_lib.m4
@@ -1,14 +1,19 @@
 AC_DEFUN([AX_DEFAULT_LIB],
-[AS_IF([test "\${exec_prefix}/lib" = "$libdir"],
-    [AS_IF([test "$exec_prefix" = "NONE" && test "$prefix" != "NONE"],
-        [exec_prefix=$prefix])
-    AS_IF([test "$exec_prefix" = "NONE"], [exec_prefix=$ac_default_prefix])
-    AS_IF([test -d "${exec_prefix}/lib64"], [
+[AS_IF([test "\${exec_prefix}/lib" = "$libdir"], [
+    AS_IF([test "$prefix" = "NONE"], [prefix=$ac_default_prefix])
+    AS_IF([test "$exec_prefix" = "NONE"], [exec_prefix='${prefix}'])
+    AS_IF([eval test -d "${exec_prefix}/lib64"], [
         LIB_PATH="lib64"
     ],[
         LIB_PATH="lib"
     ])
 ], [
     LIB_PATH="${libdir:`expr length "$exec_prefix" + 1`}"
+    AS_IF([test -z "${libdir##\$\{exec_prefix\}/*}"], [
+        LIB_PATH="${libdir:15}"
+    ])
+    AS_IF([test -z "${libdir##\$exec_prefix/*}"], [
+        LIB_PATH="${libdir:13}"
+    ])
 ])
 AC_SUBST(LIB_PATH)])

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 14:26 [PATCH] tools: don't expand prefix and exec_prefix too early Jan Beulich
@ 2012-08-08 14:39 ` Jan Beulich
  2012-08-08 14:45   ` Matt Wilson
  2012-08-08 15:34   ` Ian Jackson
  2012-08-08 15:10 ` Matt Wilson
  1 sibling, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2012-08-08 14:39 UTC (permalink / raw)
  To: Matt Wilson, Ian Campbell, Ian Jackson; +Cc: xen-devel

>>> On 08.08.12 at 16:26, "Jan Beulich" <JBeulich@suse.com> wrote:
> A comment in tools/configure says that it is intended for these to be
> command line overridable, so they shouldn't get expanded at configure
> time.

In addition, it would have been _very_ nice if it had been
prominently announced that with (I believe) 25594:ad08cd8e7097
it is now _required_ to configure with --libdir on x86-64, or else all
the .so-s end up under /usr/lib. Figuring this out and getting the
patch here in the right form to be able to use the most compatible
form --libdir='${exec_prefix}'/lib64 has taken me a good part of
the day, which could have been avoided if this whole configure
adjustment (much like had apparently been missing already in
earlier cases) had been done properly. I just can't imagine I'm
the only one having used no options at all, and things working
nevertheless despite .../lib not being in the library search paths
used when running xl et al.

Jan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 14:39 ` Jan Beulich
@ 2012-08-08 14:45   ` Matt Wilson
  2012-08-08 14:52     ` Jan Beulich
  2012-08-08 15:34   ` Ian Jackson
  1 sibling, 1 reply; 14+ messages in thread
From: Matt Wilson @ 2012-08-08 14:45 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ian Jackson, Ian Campbell, xen-devel

On Wed, Aug 08, 2012 at 07:39:13AM -0700, Jan Beulich wrote:
> >>> On 08.08.12 at 16:26, "Jan Beulich" <JBeulich@suse.com> wrote:
> > A comment in tools/configure says that it is intended for these to be
> > command line overridable, so they shouldn't get expanded at configure
> > time.
> 
> In addition, it would have been _very_ nice if it had been
> prominently announced that with (I believe) 25594:ad08cd8e7097
>
> it is now _required_ to configure with --libdir on x86-64, or else all
> the .so-s end up under /usr/lib. Figuring this out and getting the
> patch here in the right form to be able to use the most compatible
> form --libdir='${exec_prefix}'/lib64 has taken me a good part of
> the day, which could have been avoided if this whole configure
> adjustment (much like had apparently been missing already in
> earlier cases) had been done properly. I just can't imagine I'm
> the only one having used no options at all, and things working
> nevertheless despite .../lib not being in the library search paths
> used when running xl et al.

I'm sorry for the trouble it cause you today, Jan. I thought I called
it out sufficiently in the commit log:

    With this change, packagers can supply the desired location for
    shared libraries on the ./configure command line. Packagers need
    to note that the default behaviour on 64-bit Linux systems will be
    to install shared libraries in /usr/lib, not /usr/lib64, unless a
    --libdir value is provided to ./configure.

The new behavior is consistent with all packages that use autoconf.

Would a separate email on the topic to xen-devel, apart from the patch
discussion, have helped raise awareness?

Matt

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 14:45   ` Matt Wilson
@ 2012-08-08 14:52     ` Jan Beulich
  2012-08-08 14:55       ` Ian Campbell
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2012-08-08 14:52 UTC (permalink / raw)
  To: Matt Wilson; +Cc: Ian Jackson, Ian Campbell, xen-devel

>>> On 08.08.12 at 16:45, Matt Wilson <msw@amazon.com> wrote:
> On Wed, Aug 08, 2012 at 07:39:13AM -0700, Jan Beulich wrote:
>> >>> On 08.08.12 at 16:26, "Jan Beulich" <JBeulich@suse.com> wrote:
>> > A comment in tools/configure says that it is intended for these to be
>> > command line overridable, so they shouldn't get expanded at configure
>> > time.
>> 
>> In addition, it would have been _very_ nice if it had been
>> prominently announced that with (I believe) 25594:ad08cd8e7097
>>
>> it is now _required_ to configure with --libdir on x86-64, or else all
>> the .so-s end up under /usr/lib. Figuring this out and getting the
>> patch here in the right form to be able to use the most compatible
>> form --libdir='${exec_prefix}'/lib64 has taken me a good part of
>> the day, which could have been avoided if this whole configure
>> adjustment (much like had apparently been missing already in
>> earlier cases) had been done properly. I just can't imagine I'm
>> the only one having used no options at all, and things working
>> nevertheless despite .../lib not being in the library search paths
>> used when running xl et al.
> 
> I'm sorry for the trouble it cause you today, Jan. I thought I called
> it out sufficiently in the commit log:
> 
>     With this change, packagers can supply the desired location for
>     shared libraries on the ./configure command line. Packagers need
>     to note that the default behaviour on 64-bit Linux systems will be
>     to install shared libraries in /usr/lib, not /usr/lib64, unless a
>     --libdir value is provided to ./configure.

No, this comment says "can", not "have to". Plus you don't really
expect people to read every changeset's description, do you?

> The new behavior is consistent with all packages that use autoconf.

That indeed appears to be the case, admittedly to my not
insignificant surprise.

> Would a separate email on the topic to xen-devel, apart from the patch
> discussion, have helped raise awareness?

Yes. I for my part follow only very selected tools side discussions,
and expect the tools maintainers to get things sorted out. But of
course I need to build and run the tools, so if some change gets
done to the way things need to get built (or run) successfully
then announcing this independently of the patch would certainly
be helpful in general.

Jan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 14:52     ` Jan Beulich
@ 2012-08-08 14:55       ` Ian Campbell
  2012-08-08 15:11         ` Olaf Hering
  0 siblings, 1 reply; 14+ messages in thread
From: Ian Campbell @ 2012-08-08 14:55 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ian Jackson, Matt Wilson, xen-devel

On Wed, 2012-08-08 at 15:52 +0100, Jan Beulich wrote:
> >>> On 08.08.12 at 16:45, Matt Wilson <msw@amazon.com> wrote:
> > On Wed, Aug 08, 2012 at 07:39:13AM -0700, Jan Beulich wrote:
> >> >>> On 08.08.12 at 16:26, "Jan Beulich" <JBeulich@suse.com> wrote:
> >> > A comment in tools/configure says that it is intended for these to be
> >> > command line overridable, so they shouldn't get expanded at configure
> >> > time.
> >> 
> >> In addition, it would have been _very_ nice if it had been
> >> prominently announced that with (I believe) 25594:ad08cd8e7097
> >>
> >> it is now _required_ to configure with --libdir on x86-64, or else all
> >> the .so-s end up under /usr/lib. Figuring this out and getting the
> >> patch here in the right form to be able to use the most compatible
> >> form --libdir='${exec_prefix}'/lib64 has taken me a good part of
> >> the day, which could have been avoided if this whole configure
> >> adjustment (much like had apparently been missing already in
> >> earlier cases) had been done properly. I just can't imagine I'm
> >> the only one having used no options at all, and things working
> >> nevertheless despite .../lib not being in the library search paths
> >> used when running xl et al.
> > 
> > I'm sorry for the trouble it cause you today, Jan. I thought I called
> > it out sufficiently in the commit log:
> > 
> >     With this change, packagers can supply the desired location for
> >     shared libraries on the ./configure command line. Packagers need
> >     to note that the default behaviour on 64-bit Linux systems will be
> >     to install shared libraries in /usr/lib, not /usr/lib64, unless a
> >     --libdir value is provided to ./configure.
> 
> No, this comment says "can", not "have to". Plus you don't really
> expect people to read every changeset's description, do you?
> 
> > The new behavior is consistent with all packages that use autoconf.
> 
> That indeed appears to be the case, admittedly to my not
> insignificant surprise.
> 
> > Would a separate email on the topic to xen-devel, apart from the patch
> > discussion, have helped raise awareness?
> 
> Yes. I for my part follow only very selected tools side discussions,
> and expect the tools maintainers to get things sorted out. But of
> course I need to build and run the tools, so if some change gets
> done to the way things need to get built (or run) successfully
> then announcing this independently of the patch would certainly
> be helpful in general.

FWIW I've just added it http://wiki.xen.org/wiki/Xen_4.2_Release_Notes
which probably wouldn't have helped you but should help in general.

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 14:26 [PATCH] tools: don't expand prefix and exec_prefix too early Jan Beulich
  2012-08-08 14:39 ` Jan Beulich
@ 2012-08-08 15:10 ` Matt Wilson
  2012-08-08 15:53   ` Jan Beulich
  1 sibling, 1 reply; 14+ messages in thread
From: Matt Wilson @ 2012-08-08 15:10 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ian Jackson, Ian Campbell, xen-devel

On Wed, Aug 08, 2012 at 07:26:35AM -0700, Jan Beulich wrote:
> A comment in tools/configure says that it is intended for these to be
> command line overridable, so they shouldn't get expanded at configure
> time.

I'm not sure which comment you're referencing, or which command line
it's intended that you're able to override prefix/exec_prefix. Could
you point it out? What's not working?

> The patch is fixing tools/m4/default_lib.m4 as far as I can see myself
> doing this, but imo it is flawed altogether and should rather be
> removed:
> - setting prefix and exec_prefix to default values is being done later
>   in tools/configure anyway
> - setting LIB_PATH based on the (non-)existence of a lib64 directory
>   underneath ${exec_prefix} is plain wrong (it can obviously exist on a
>   32-bit installation)
> - I wasn't able to locate any use of LIB_PATH

I believe that the LIB_PATH portions can now be removed. I also
believe you're right that all of this can be removed. Did you attempt
that as well?

> (I did see IanC's comment in c/s 25594:ad08cd8e7097 that removing it
> supposedly causes other problems, but I don't see how that would
> happen).

The tail of the thread where IanC had trouble is here: 
  http://lists.xen.org/archives/html/xen-devel/2012-07/msg00268.html

If we're going back into this code, I think that adding a lowercase
prefix variable and removing default_lib.m4 altogether should resolve
remaining problems. But we should probably only do that for 4.2 if
something's very broken with what's in the tree now.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
> This will require tools/configure to be re-generated.

This is typically part of the same commit.

Matt

> --- a/config/Tools.mk.in
> +++ b/config/Tools.mk.in
> @@ -1,5 +1,6 @@
>  # Prefix and install folder
> -PREFIX              := @prefix@
> +prefix              := @prefix@
> +PREFIX              := $(prefix)
>  exec_prefix         := @exec_prefix@
>  LIBDIR              := @libdir@
>  
> --- a/tools/m4/default_lib.m4
> +++ b/tools/m4/default_lib.m4
> @@ -1,14 +1,19 @@
>  AC_DEFUN([AX_DEFAULT_LIB],
> -[AS_IF([test "\${exec_prefix}/lib" = "$libdir"],
> -    [AS_IF([test "$exec_prefix" = "NONE" && test "$prefix" != "NONE"],
> -        [exec_prefix=$prefix])
> -    AS_IF([test "$exec_prefix" = "NONE"], [exec_prefix=$ac_default_prefix])
> -    AS_IF([test -d "${exec_prefix}/lib64"], [
> +[AS_IF([test "\${exec_prefix}/lib" = "$libdir"], [
> +    AS_IF([test "$prefix" = "NONE"], [prefix=$ac_default_prefix])
> +    AS_IF([test "$exec_prefix" = "NONE"], [exec_prefix='${prefix}'])
> +    AS_IF([eval test -d "${exec_prefix}/lib64"], [
>          LIB_PATH="lib64"
>      ],[
>          LIB_PATH="lib"
>      ])
>  ], [
>      LIB_PATH="${libdir:`expr length "$exec_prefix" + 1`}"
> +    AS_IF([test -z "${libdir##\$\{exec_prefix\}/*}"], [
> +        LIB_PATH="${libdir:15}"
> +    ])
> +    AS_IF([test -z "${libdir##\$exec_prefix/*}"], [
> +        LIB_PATH="${libdir:13}"
> +    ])
>  ])
>  AC_SUBST(LIB_PATH)])
> 
> 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 14:55       ` Ian Campbell
@ 2012-08-08 15:11         ` Olaf Hering
  2012-08-08 15:18           ` Ian Campbell
  0 siblings, 1 reply; 14+ messages in thread
From: Olaf Hering @ 2012-08-08 15:11 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Ian Jackson, Matt Wilson, Jan Beulich, xen-devel

On Wed, Aug 08, Ian Campbell wrote:

> FWIW I've just added it http://wiki.xen.org/wiki/Xen_4.2_Release_Notes
> which probably wouldn't have helped you but should help in general.

Maybe we can simplify configure like this (untested patch):

diff -r f9f47654484e configure
--- a/configure
+++ b/configure
@@ -1,2 +1,7 @@
 #!/bin/sh -e
-cd tools && ./configure $@
+_easy_lib64=
+if test "`arch`" = "x86_64"
+then
+	_easy_lib64=--libdir=/usr/lib64
+fi
+cd tools && ./configure ${_easy_lib64} $@

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 15:11         ` Olaf Hering
@ 2012-08-08 15:18           ` Ian Campbell
  0 siblings, 0 replies; 14+ messages in thread
From: Ian Campbell @ 2012-08-08 15:18 UTC (permalink / raw)
  To: Olaf Hering; +Cc: Ian Jackson, Matt Wilson, Jan Beulich, xen-devel

On Wed, 2012-08-08 at 16:11 +0100, Olaf Hering wrote:
> On Wed, Aug 08, Ian Campbell wrote:
> 
> > FWIW I've just added it http://wiki.xen.org/wiki/Xen_4.2_Release_Notes
> > which probably wouldn't have helped you but should help in general.
> 
> Maybe we can simplify configure like this (untested patch):

Ew, what a horribly opaque way to go about this.

Unless there is some common autoconf macro which all autoconf-using
projects use to get this sort of behaviour we should just leave things
as is, i.e. our behaviour should match the behaviour of every other
autoconf-using project not invent our own madness.

> 
> diff -r f9f47654484e configure
> --- a/configure
> +++ b/configure
> @@ -1,2 +1,7 @@
>  #!/bin/sh -e
> -cd tools && ./configure $@
> +_easy_lib64=
> +if test "`arch`" = "x86_64"
> +then
> +	_easy_lib64=--libdir=/usr/lib64
> +fi
> +cd tools && ./configure ${_easy_lib64} $@

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 14:39 ` Jan Beulich
  2012-08-08 14:45   ` Matt Wilson
@ 2012-08-08 15:34   ` Ian Jackson
  1 sibling, 0 replies; 14+ messages in thread
From: Ian Jackson @ 2012-08-08 15:34 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ian Campbell, Matt Wilson, xen-devel

Jan Beulich writes ("Re: [Xen-devel] [PATCH] tools: don't expand prefix and exec_prefix too early"):
> In addition, it would have been _very_ nice if it had been
> prominently announced that with (I believe) 25594:ad08cd8e7097
> it is now _required_ to configure with --libdir on x86-64,

This is only required on FHS-noncompliant systems.  On (for example)
a Debian amd64 system the default directory /usr/lib is correct.

I expect we will revisit all of this in 4.3 since it might be a good
idea to support multiarch.

Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 15:10 ` Matt Wilson
@ 2012-08-08 15:53   ` Jan Beulich
  2012-08-08 16:06     ` Matt Wilson
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2012-08-08 15:53 UTC (permalink / raw)
  To: Matt Wilson; +Cc: Ian Jackson, Ian Campbell, xen-devel

>>> On 08.08.12 at 17:10, Matt Wilson <msw@amazon.com> wrote:
> On Wed, Aug 08, 2012 at 07:26:35AM -0700, Jan Beulich wrote:
>> A comment in tools/configure says that it is intended for these to be
>> command line overridable, so they shouldn't get expanded at configure
>> time.
> 
> I'm not sure which comment you're referencing, or which command line
> it's intended that you're able to override prefix/exec_prefix. Could
> you point it out? What's not working?

The comment (around line 790 currently) reads
"# Installation directory options.
 # These are left unexpanded so users can "make install exec_prefix=/foo"
 # and all the variables that are supposed to be based on exec_prefix
 # by default will actually change.
 # Use braces instead of parens because sh, perl, etc. also accept them.
 # (The list follows the same order as the GNU Coding Standards.)"

The thing that wasn't working for me was passing
--libdir='${exec_prefix}'/lib64 to ./configure.

>> The patch is fixing tools/m4/default_lib.m4 as far as I can see myself
>> doing this, but imo it is flawed altogether and should rather be
>> removed:
>> - setting prefix and exec_prefix to default values is being done later
>>   in tools/configure anyway
>> - setting LIB_PATH based on the (non-)existence of a lib64 directory
>>   underneath ${exec_prefix} is plain wrong (it can obviously exist on a
>>   32-bit installation)
>> - I wasn't able to locate any use of LIB_PATH
> 
> I believe that the LIB_PATH portions can now be removed. I also
> believe you're right that all of this can be removed. Did you attempt
> that as well?

No, I didn't, not the least because I don't have the right autoconf
version around and didn't dare to re-generate tools/configure
myself (instead I did the resulting adjustments manually).

>> This will require tools/configure to be re-generated.
> 
> This is typically part of the same commit.

Sure, just that generally one wouldn't include generated parts
in the patch, but expect them to be produced when committing
(and as said above, I'd have to rely on the tools maintainers
for this in order to not risk corrupting something).

Jan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 15:53   ` Jan Beulich
@ 2012-08-08 16:06     ` Matt Wilson
  2012-08-08 16:08       ` Jan Beulich
  0 siblings, 1 reply; 14+ messages in thread
From: Matt Wilson @ 2012-08-08 16:06 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ian Jackson, Ian Campbell, xen-devel

On Wed, Aug 08, 2012 at 08:53:28AM -0700, Jan Beulich wrote:
> >>> On 08.08.12 at 17:10, Matt Wilson <msw@amazon.com> wrote:
> > On Wed, Aug 08, 2012 at 07:26:35AM -0700, Jan Beulich wrote:
> >> A comment in tools/configure says that it is intended for these to be
> >> command line overridable, so they shouldn't get expanded at configure
> >> time.
> > 
> > I'm not sure which comment you're referencing, or which command line
> > it's intended that you're able to override prefix/exec_prefix. Could
> > you point it out? What's not working?
> 
> The comment (around line 790 currently) reads
> "# Installation directory options.
>  # These are left unexpanded so users can "make install exec_prefix=/foo"
>  # and all the variables that are supposed to be based on exec_prefix
>  # by default will actually change.
>  # Use braces instead of parens because sh, perl, etc. also accept them.
>  # (The list follows the same order as the GNU Coding Standards.)"
> 
> The thing that wasn't working for me was passing
> --libdir='${exec_prefix}'/lib64 to ./configure.

Aah, OK. That's not something that I commonly see packaging control
files (RPM .spec / dpkg rules) do, but you're right that it's intended
to work.

> >> The patch is fixing tools/m4/default_lib.m4 as far as I can see myself
> >> doing this, but imo it is flawed altogether and should rather be
> >> removed:
> >> - setting prefix and exec_prefix to default values is being done later
> >>   in tools/configure anyway
> >> - setting LIB_PATH based on the (non-)existence of a lib64 directory
> >>   underneath ${exec_prefix} is plain wrong (it can obviously exist on a
> >>   32-bit installation)
> >> - I wasn't able to locate any use of LIB_PATH
> > 
> > I believe that the LIB_PATH portions can now be removed. I also
> > believe you're right that all of this can be removed. Did you attempt
> > that as well?
> 
> No, I didn't, not the least because I don't have the right autoconf
> version around and didn't dare to re-generate tools/configure
> myself (instead I did the resulting adjustments manually).

Indeed, it's a bit of a pain to set it up. I imagine that's why the
autogenerated files are committed into mercurial, which is a practice
I don't like...

> >> This will require tools/configure to be re-generated.
> > 
> > This is typically part of the same commit.
> 
> Sure, just that generally one wouldn't include generated parts
> in the patch, but expect them to be produced when committing
> (and as said above, I'd have to rely on the tools maintainers
> for this in order to not risk corrupting something).

Fair enough. :-)

Would you like me to take another pass at fixing this the "right" way,
by removing this bit altogether and adding lowercase variables to
tools/Config.mk?

Matt

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 16:06     ` Matt Wilson
@ 2012-08-08 16:08       ` Jan Beulich
  2012-08-13 13:50         ` Ian Jackson
  0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2012-08-08 16:08 UTC (permalink / raw)
  To: Matt Wilson; +Cc: Ian Jackson, Ian Campbell, xen-devel

>>> On 08.08.12 at 18:06, Matt Wilson <msw@amazon.com> wrote:
> Would you like me to take another pass at fixing this the "right" way,
> by removing this bit altogether and adding lowercase variables to
> tools/Config.mk?

Oh, yes, if you can do this in a more complete fashion, removing
the suspicious default_dir.m4 altogether, that would be wonderful.
Not sure what the tools maintainers think of this, though.

Jan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-08 16:08       ` Jan Beulich
@ 2012-08-13 13:50         ` Ian Jackson
  2012-08-13 17:21           ` Matt Wilson
  0 siblings, 1 reply; 14+ messages in thread
From: Ian Jackson @ 2012-08-13 13:50 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ian Campbell, Matt Wilson, xen-devel

Jan Beulich writes ("Re: [Xen-devel] [PATCH] tools: don't expand prefix and exec_prefix too early"):
> >>> On 08.08.12 at 18:06, Matt Wilson <msw@amazon.com> wrote:
> > Would you like me to take another pass at fixing this the "right" way,
> > by removing this bit altogether and adding lowercase variables to
> > tools/Config.mk?
> 
> Oh, yes, if you can do this in a more complete fashion, removing
> the suspicious default_dir.m4 altogether, that would be wonderful.
> Not sure what the tools maintainers think of this, though.

That would be fine by me.  Before throwing it into 4.2 I will do a few
build tests and check that differences in the output file layout are
as expected.

Thanks,
Ian.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] tools: don't expand prefix and exec_prefix too early
  2012-08-13 13:50         ` Ian Jackson
@ 2012-08-13 17:21           ` Matt Wilson
  0 siblings, 0 replies; 14+ messages in thread
From: Matt Wilson @ 2012-08-13 17:21 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Ian Campbell, Jan Beulich, xen-devel

On Mon, Aug 13, 2012 at 06:50:01AM -0700, Ian Jackson wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH] tools: don't expand prefix and exec_prefix too early"):
> > >>> On 08.08.12 at 18:06, Matt Wilson <msw@amazon.com> wrote:
> > > Would you like me to take another pass at fixing this the "right" way,
> > > by removing this bit altogether and adding lowercase variables to
> > > tools/Config.mk?
> > 
> > Oh, yes, if you can do this in a more complete fashion, removing
> > the suspicious default_dir.m4 altogether, that would be wonderful.
> > Not sure what the tools maintainers think of this, though.
> 
> That would be fine by me.  Before throwing it into 4.2 I will do a few
> build tests and check that differences in the output file layout are
> as expected.

OK, I'll get my development environment set up again with the right
version of autoconf and work on implementing this in the next few
days.

Matt

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-08-13 17:21 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-08 14:26 [PATCH] tools: don't expand prefix and exec_prefix too early Jan Beulich
2012-08-08 14:39 ` Jan Beulich
2012-08-08 14:45   ` Matt Wilson
2012-08-08 14:52     ` Jan Beulich
2012-08-08 14:55       ` Ian Campbell
2012-08-08 15:11         ` Olaf Hering
2012-08-08 15:18           ` Ian Campbell
2012-08-08 15:34   ` Ian Jackson
2012-08-08 15:10 ` Matt Wilson
2012-08-08 15:53   ` Jan Beulich
2012-08-08 16:06     ` Matt Wilson
2012-08-08 16:08       ` Jan Beulich
2012-08-13 13:50         ` Ian Jackson
2012-08-13 17:21           ` Matt Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.