Re: [PATCH v2] Shorter object file names under subdir-objects

[Mathieu Lirzin]

Hello,

Thomas Martitz <address@hidden> writes:

> here's the updated patch. Changes since v1:
>
> - The test that checks for exact object file names was replaced by
> another one that tests if object files clash due to truncation of the
> name
> - When there were two programs or libraries with the same base name
> (but in different dirs), then the wrong object was linked into one of
> the programs/libs.
>
> Note: There is still one extreme edge case which is not handled
> yet. You can apparently name a program libfoo_la. If there is also
> library libfoo.la generated in another dir, then the objects will
> clash again. My fix above only checks for clashes within programs (and
> within libraries). This is due to the code structure (libraries are
> written out before programs parsed at all). I wanted to get opinions
> on how to tackle this before going a wrong route. Maybe change the
> code so that all programs and libs are parsed first, checked for
> clashes second and then written out?

For now you are using a '%blacklist' which is local to each function
('handle_programs' and 'handle_libraries').  Maybe if you make it global
and resettable in '&initialize_per_input', the name clash detection
could work across programs and libraries?

I haven't tried implemented this idea so maybe there are some issues
with it.

> From fe1130722d542d42d081696cf76a94746e0abdfc Mon Sep 17 00:00:00 2001
> From: Thomas Martitz <address@hidden>
> Date: Mon, 13 Mar 2017 12:41:59 +0100
> Subject: [PATCH] Shorter object file names under subdir-objects
>
> With the %reldir% feature, object file names can become very long, because the
> file names are prefixed with the %canon_reldir% substitution. The reason is to
> achieve unique object file names when target-specific CFLAGS or similar are
> used. When subdir-objects is also in effect, these long file names are also
> placed in potentially deep subdirectories.
>
> But with subdir-objects this is unnecessary, since uniqueness of the object
> file names is already achieved by placing them next to the unique source 
> files.
>
> Therefore, this changes strips paths components, that are caused by
> %canon_reldir% or otherwise, from the object file names. The object file name
> is prefixed by the target in case of target-specific CFLAGS. As a result, the
> build tree looks less scary and many cases where $var_SHORTNAME was necessary
> can now be avoided. Remember that the use of $var_SHORTNAME is discouraged 
> (and
> is not always an option since it does not work inside conditionals).
>
> There is one exception to the above: if the same source file is linked to
> separate programs/libraries with per-executable flags and the
> programs/libraries have identical names (but are in different directories),
> then uniqueness of truncated object file names is broken. Therefore the
> truncation is prevented for these targets if there happens to be identically
> named progams or libraries.
>
> The handling of the above case is ensured in a test case. The test ensures 
> that
> programs and libraries with equal base names get unique object files even if
> object file name truncation is in effect.
>
> Example:
> previously:
>   AUTOMAKE_OPTIONS = subdir-objects
>   bin_PROGRAMS += path/to/foo
>   path_to_foo_CFLAGS = $(AM_CFLAGS) -g
>
> resulted in objects:
>   sub/path_to_foo-foo.o
>
> now object file name is:
>   sub/foo-foo.o

Regarding the commit message, it needs to have a ChangeLog style at the
end.  Here is what I am proposing with a shorter rationale which IMO
gives enough context:

--8<---------------cut here---------------start------------->8---
automake: Shorter object file names under subdir-objects

Combining the 'subdir-objects' option with target-specific flags, had
the consequence of producing long object file names.  This was done to
preventively ensure the uniqueness of object file names.  We are now
using shorter names by default, and handle long names when an actual
conflict is detected.  This will hopefully reduce the necessity of
using the 'prog_SHORTNAME' facility.

Example Makefile.am:
  AUTOMAKE_OPTIONS = subdir-objects
  bin_PROGRAMS += path/to/foo
  path_to_foo_CFLAGS = $(AM_CFLAGS) -g

previously resulted in object:
  $(top_builddir)/path/to/path_to_foo-foo.o

now object file name is:
  $(top_builddir)/path/to/foo-foo.o

* bin/automake.in (longest_common_prefix, may_truncate)
(find_duplicate_basenames): New subroutines.
(handle_programs, handle_libraries): Keep track of duplicate names.
(handle_single_transform): Truncate object file names when possible.
* t/subobj-objname-clash.sh: New test.
* t/list-of-tests.mk (handwritten_TESTS): Add it.
* NEWS: Update.
--8<---------------cut here---------------end--------------->8---

> ---
>  NEWS                      |  6 ++++
>  bin/automake.in           | 90 
> ++++++++++++++++++++++++++++++++++++++++++++---
>  t/list-of-tests.mk        |  1 +
>  t/subobj-objname-clash.sh | 86 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 178 insertions(+), 5 deletions(-)
>  create mode 100644 t/subobj-objname-clash.sh
>
> diff --git a/NEWS b/NEWS
> index af904d442..a1f4f37c1 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -64,6 +64,12 @@
>  
>  New in 1.16:
>  
> +* Miscellaneous changes
> +
> +  - When subdir-objects is in effect, Automake will now construct shorter
> +    object file names. This should make the use of $var_SHORTNAME is 
> unnecessary
                                                                     ^^^
                                                             extra 'is'.
> +    in many cases. $var_SHORTNAME is discouraged anyway.
                                   
Please take care of not moving above the 79 characters limit per line.
Here is my proposition which explicitely state the cases where the name
will be shorter:

--8<---------------cut here---------------start------------->8---
  - When subdir-objects is in effect, Automake will now construct
    shorter object file names when no programs and libraries name
    clashes are encountered.  This should make the discouraged use of
    'foo_SHORTNAME' unnecessary in many cases.
--8<---------------cut here---------------end--------------->8---

>  * Bugs fixed:
>  
>    - Automatic dependency tracking has been fixed to work also when the
> diff --git a/bin/automake.in b/bin/automake.in
> index 09a1c956b..aaca1321b 100644
> --- a/bin/automake.in
> +++ b/bin/automake.in
> @@ -1562,6 +1562,29 @@ sub check_libobjs_sources
>      }
>  }
>  
> +# http://linux.seindal.dk/2005/09/09/longest-common-prefix-in-perl/
> +sub longest_common_prefix {
> +    my $prefix = shift;
> +
> +    for (@_)
> +      {
> +     chop $prefix while (! /^$prefix/);
> +      }
> +    return $prefix;
> +}
> +
> +sub may_truncate {
> +    my ($shortname, %transform) = @_;
> +    my $blacklist = $transform{'TRUNCATE_OBJNAME_BLACKLIST'};
> +
> +    # disallow truncation if $derived ends with any of the blacklisted 
> targets
> +    # the blacklist still contains the non-canonicalize basename 
> ($name.$ext).
> +    foreach (@$blacklist)
> +      {
> +     return 0 if (canonicalize($_) eq $shortname);
> +      }
> +    return 1;
> +}

Even when the subroutine seems trivial, I think it is nice to have a
docstring which explains what are the expected inputs, and what is the
returned value or the side effects.

I have updated the indentation to use GNU style, added docstrings, and
added prototypes:

--8<---------------cut here---------------start------------->8---
# $PREFIX
# longest_common_prefix (@WORDS)
# ------------------------------
# Return the longest common prefix of @WORDS.  If there is no common prefix
# return the empty string.
#
# Implementation taken from
# http://linux.seindal.dk/2005/09/09/longest-common-prefix-in-perl/
sub longest_common_prefix (@)
{
  my $prefix = shift;
  for (@_)
    {
      chop $prefix
        while (! /^$prefix/);
    }

  return $prefix;
}

# $BOOLEAN
# may_truncate ($SHORTNAME, %TRANSFORM)
# -------------------------------------
# Check if $SHORTNAME can be truncated based on the
# 'TRUNCATE_OBJNAME_BLACKLIST' property of %TRANSFORM.
sub may_truncate ($%)
{
  my ($shortname, %transform) = @_;
  my $blacklist = $transform{"TRUNCATE_OBJNAME_BLACKLIST"};

  # Disallow truncation if $derived ends with any of the blacklisted targets
  # the blacklist still contains the non-canonicalize basename ($name.$ext).
  foreach my $basename (@$blacklist)
    {
      return 0
        if (canonicalize ($basename) eq $shortname);
    }
  return 1;
}
--8<---------------cut here---------------end--------------->8---

>  
>  # @OBJECTS
>  # handle_single_transform ($VAR, $TOPPARENT, $DERIVED, $OBJ, $FILE, 
> %TRANSFORM)
> @@ -1710,8 +1733,34 @@ sub handle_single_transform
>               # name that is too long for losing systems, in
>               # some situations.  So we provide _SHORTNAME to
>               # override.
> -
> +             # If subdir-object is in effect, it's not necessary to
> +             # use the complete 'DERIVED_OBJECT' since objects are
> +             # placed next to their source file. Therefore it is already
> +             # unique (within that directory). Thus, we try to avoid un
> +             # necessarily long file names by stripping the directory
> +             # components of 'DERIVED_OBJECT' (that quite likely the result 
> from
> +             # %canon_reldir%/%C% usage). This allows to avoid explicit 
> _SHORTNAME
> +             # usage in many cases.
> +             # NOTE: In some setups, the stripping may lead to duplicated 
> objects,
> +             # even in presence of per-executable flags, for example if there
> +             # are identically-named programs in different directories, so
> +             # these targets must be exempted using a blacklist
>               my $dname = $derived;
> +             if ($directory ne '' && option 'subdir-objects')
> +               {
> +                 # At this point, we don't clear information about what parts
> +                 # of $derived are truly path components. We can determine
> +                 # by comparing against the canonicalization of $directory.
> +                 # And if $directory is empty there is nothing to strip 
> anyway.
> +                 my $canon_dirname = canonicalize ($directory) . "_";
> +                 my @names = ($derived, $canon_dirname);
> +                 my $prefix = longest_common_prefix (@names);
> +                 # After canonicalization, "_" separates directories, thus 
> use
> +                 # everything after the the last separator.
> +                 $dname = substr ($derived, rindex ($prefix, "_")+1);
                                                                    ^^^
...) + 1)

> +                 # check if this dname is on the blacklist
> +                 $dname = $derived unless may_truncate ($dname, %transform);
> +               }
>               my $var = var ($derived . '_SHORTNAME');
>               if ($var)
>               {
> @@ -2431,6 +2479,18 @@ sub handle_libtool ()
>                                  LTRMS => join ("\n", @libtool_rms));
>  }
>  
> +# Expects to be passed a list as returned via am_install_var()
> +sub find_duplicate_basenames
> +{
> +  my %seen = ();
> +  my @dups = ();
> +  foreach my $pair (@_) {
> +    my $base = basename(@$pair[1]);
> +    push (@dups, $base) if ($seen{$base});
> +    $seen{$base} = $base;
> +  }
> +  return @dups;
> +}

I have added a docstring and corrected the indentation, like for the
previous subroutines:

--8<---------------cut here---------------start------------->8---
# @DUPLICATES
# find_duplicate_basenames (@VARIABLES)
# -------------------------------------
# Expects @VARIABLES to be a list as returned via '&am_install_var'.
sub find_duplicate_basenames (@)
{
  my %seen = ();
  my @dups = ();
  foreach my $pair (@_)
    {
      my $base = basename (@$pair[1]);
      push (@dups, $base)
        if ($seen{$base});
      $seen{$base} = $base;
    }
  return @dups;
}
--8<---------------cut here---------------end--------------->8---

>  
>  sub handle_programs ()
>  {
> @@ -2443,6 +2503,12 @@ sub handle_programs ()
>    my $seen_global_libobjs =
>      var ('LDADD') && handle_lib_objects ('', 'LDADD');
>  
> +  # Check if we have programs with identical basename. In this case, the
> +  # object file name truncation is disabled for this program (and other 
> programs
                                                                               
^^^
line is too long.

> +  # with the same basename, because the object file names may clash
> +  # (this only applies to subdir-object setups).
> +  my @blacklist = find_duplicate_basenames (@proglist);
> +
>    foreach my $pair (@proglist)
>      {
>        my ($where, $one_file) = @$pair;
> @@ -2461,7 +2527,8 @@ sub handle_programs ()
>        $where->set (INTERNAL->get);
>  
>        my $linker = handle_source_transform ($xname, $one_file, $obj, $where,
> -                                            NONLIBTOOL => 1, LIBTOOL => 0);
> +                                            NONLIBTOOL => 1, LIBTOOL => 0,
> +                                            TRUNCATE_OBJNAME_BLACKLIST => 
> address@hidden);
                                                                                
^^^
line is too long.  Please indent like this:

--8<---------------cut here---------------start------------->8---
      my $linker =
        handle_source_transform ($xname, $one_file, $obj, $where,
                                 NONLIBTOOL => 1, LIBTOOL => 0,
                                 TRUNCATE_OBJNAME_BLACKLIST => address@hidden);
--8<---------------cut here---------------end--------------->8---

>  
>        if (var ($xname . "_LDADD"))
>       {
> @@ -2541,6 +2608,12 @@ sub handle_libraries ()
>    define_variable ('ARFLAGS', 'cru', INTERNAL);
>    define_verbose_tagvar ('AR');
>  
> +  # Check if we have libraries with identical basename. In this case, the
> +  # object file name truncation is disabled for this library (and other 
> libraries
> +  # with the same basename, because the object file names may clash
> +  # (this only applies to subdir-object setups).
> +  my @blacklist = find_duplicate_basenames (@liblist);
> +
>    foreach my $pair (@liblist)
>      {
>        my ($where, $onelib) = @$pair;
> @@ -2597,7 +2670,8 @@ sub handle_libraries ()
>        set_seen ('EXTRA_' . $xlib . '_DEPENDENCIES');
>  
>        handle_source_transform ($xlib, $onelib, $obj, $where,
> -                               NONLIBTOOL => 1, LIBTOOL => 0);
> +                               NONLIBTOOL => 1, LIBTOOL => 0,
> +                               TRUNCATE_OBJNAME_BLACKLIST => address@hidden);
>  
>        # If the resulting library lies in a subdirectory,
>        # make sure this directory will exist.
> @@ -2728,6 +2802,11 @@ sub handle_ltlibraries ()
>        skip_ac_subst => 1);
>      }
>  
> +  # Check if we have libraries with identical basename. In this case, the 
> automatic
> +  # object file name truncation is disabled because the object file names 
> may clash
                                                                               
^^^
lines are too long.

> +  # (this only applies to subdir-object setups)
> +  my @blacklist = find_duplicate_basenames (@liblist);
> +
>    foreach my $pair (@liblist)
>      {
>        my ($where, $onelib) = @$pair;
> @@ -2800,7 +2879,8 @@ sub handle_ltlibraries ()
>  
>  
>        my $linker = handle_source_transform ($xlib, $onelib, $obj, $where,
> -                                            NONLIBTOOL => 0, LIBTOOL => 1);
> +                                            NONLIBTOOL => 0, LIBTOOL => 1,
> +                                            TRUNCATE_OBJNAME_BLACKLIST => 
> address@hidden);

line is too long.  Please indent like previous case.

>        # Determine program to use for link.
>        my($xlink, $vlink) = define_per_target_linker_variable ($linker, 
> $xlib);
> diff --git a/t/list-of-tests.mk b/t/list-of-tests.mk
> index defca1361..a5b253504 100644
> --- a/t/list-of-tests.mk
> +++ b/t/list-of-tests.mk
> @@ -1062,6 +1062,7 @@ t/subobjname.sh \
>  t/subobj-clean-pr10697.sh \
>  t/subobj-clean-lt-pr10697.sh \
>  t/subobj-indir-pr13928.sh \
> +t/subobj-objname-clash.sh \
>  t/subobj-vpath-pr13928.sh \
>  t/subobj-pr13928-more-langs.sh \
>  t/subpkg.sh \
> diff --git a/t/subobj-objname-clash.sh b/t/subobj-objname-clash.sh
> new file mode 100644
> index 000000000..11fc9631b
> --- /dev/null
> +++ b/t/subobj-objname-clash.sh
> @@ -0,0 +1,86 @@
> +#! /bin/sh
> +# Copyright (C) 1996-2017 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Make sure that object names don't clash when using subdir-objects.
> +# The check is done for programs and libraries separately.
> +
> +. test-init.sh
> +
> +mkdir -p src
> +
> +cat >> configure.ac << 'END'
> +AC_PROG_CC
> +AC_PROG_RANLIB
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am << 'END'
> +AUTOMAKE_OPTIONS = subdir-objects foreign
> +noinst_PROGRAMS =
> +noinst_LIBRARIES =
> +include src/local.mk
> +noinst_PROGRAMS += foo
> +foo_SOURCES = src/foo.c src/main.c
> +foo_CPPFLAGS = -DVAL=0
> +noinst_LIBRARIES += libbar.a
> +libbar_a_SOURCES = src/foo.c
> +libbar_a_CPPFLAGS = -DVAL=0
> +noinst_PROGRAMS += bar
> +bar_SOURCES = src/main.c
> +bar_LDADD = libbar.a
> +END
> +
> +cat > src/local.mk << 'END'
> +noinst_PROGRAMS += src/foo
> +src_foo_CPPFLAGS = -DVAL=1
> +src_foo_SOURCES = src/foo.c src/main.c
> +noinst_PROGRAMS += src/bar
> +noinst_LIBRARIES += src/libbar.a
> +src_libbar_a_SOURCES = src/foo.c
> +src_libbar_a_CPPFLAGS = -DVAL=1
> +src_bar_SOURCES = src/main.c
> +src_bar_LDADD = src/libbar.a
> +END

For more clarity, I would prefer if the progam and librarie parts of the
Makefiles were separated them in two Makefile fragments with a comment
for each.  See my proposition:

--8<---------------cut here---------------start------------->8---
cat > Makefile.am << 'END'
AUTOMAKE_OPTIONS = subdir-objects foreign

include prog.mk
include lib.mk
END

# Check that programs with same basename don't end up sharing the same
# object files.
cat > prog.mk << 'END'
noinst_PROGRAMS = foo src/foo
src_foo_CPPFLAGS = -DVAL=1
src_foo_SOURCES = src/foo.c src/main.c

foo_CPPFLAGS = -DVAL=0
foo_SOURCES = src/foo.c src/main.c
END

# Check the same thing for libraries.
cat > lib.mk << 'END'
noinst_LIBRARIES = libbar.a src/libbar.a

libbar_a_SOURCES = src/foo.c
libbar_a_CPPFLAGS = -DVAL=0

src_libbar_a_SOURCES = src/foo.c
src_libbar_a_CPPFLAGS = -DVAL=1

noinst_PROGRAMS += bar src/bar

bar_SOURCES = src/main.c
bar_LDADD = libbar.a

src_bar_SOURCES = src/main.c
src_bar_LDADD = src/libbar.a
END
--8<---------------cut here---------------end--------------->8---

> +
> +cat > src/foo.c << 'END'
> +int
> +foo ()
> +{
> +  return VAL;
> +}
> +END
> +
> +cat > src/main.c << 'END'
> +int foo (void);
> +
> +int
> +main ()
> +{
> +  return foo ();
> +}
> +END
> +
> +$ACLOCAL
> +$AUTOCONF
> +$AUTOMAKE --add-missing
> +
> +./configure
> +$MAKE
> +./foo || fail_ "./foo should return 0"
> +./src/foo && fail_ "./src/foo should return 1"
> +./bar || fail_ "./bar should return 0"
> +./src/bar && fail_ "./src/bar should return 1"
> +$MAKE clean

Here is an updated patch which includes all the requested changes.

0001-automake-Shorter-object-file-names-under-subdir-obje.patch
Description: Text Data

Thanks.

-- 
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37