Xen devel - Aug 2012 - [PATCH] tools: don't expand prefix and exec_prefix too early

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)])





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/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

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

>>> 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

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.

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)])
> 
> 
>

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} $@

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} $@

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.

>>> 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

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

>>> 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

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.

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