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

Hello Peter,

thanks for your feedback. I'll post an update, but I'd like to wait for
Mathieu's feedback too.

Am 13.03.2017 um 15:15 schrieb Peter Rosin:
> Hi!
>
> FWIW, I like this (assuming Mathieu doesn't dig up some road block).
>
> On 2017-03-13 14:19, Thomas Martitz wrote:
>> Mathieu, based on your reaction on the other thread, I reworked my patch.
>>
>> This supersedes my other patch, "[PATCH] new option: object-shortname".
>> It is functionally the
>> same but does not introduce a new option, but ties the behavior to
>> subdir-objects instead. In
>> addition I made an additional bug fix and extended the test suite.
>>
>> Please merge.
>> Commit message follows:
>>
>> 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 unecessary, since uniqueness of the
>
> s/unecessary/unnecessary/
>
>> object file names
>> is already achieved by placing them next to the unique source files.
>
> s/is already/are already/
>
>>
>> Therefore, this changes strips paths components, that are caused by
>
> s/changes/change/
>
>> %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).
>>
>> Example:
>> previously:
>>      sub/Makefile.am:
>>      AUTOMAKE_OPTIONS = subdir-objects
>>      bin_PROGRAMS += %D%/foo
>>      %C%_foo_CFLAGS = $(AM_CFLAGS) -g
>>
>> resulted in objects:
>>      sub/sub_foo-foo.o
>>
>> now object file name is:
>>      sub/foo-foo.o
>>
>>
>
> The lines in the commit message are too long and you do not follow the
> GNU ChangeLog format.
>
> [I'm pasting various bits from the attachment inline for easier commenting]
>
>>  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 unecessary
>
> s/is unece/unnece/
>
>> +    in many cases. $var_SHORTNAME is discouraged anyway.
>> +
>>  * Bugs fixed:
>
> *snip*
>
>> -
>> +		# 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 strip the directory
>> +		# components of 'DERIVED_OBJECT' (that quite likely the result from
>> +		# %canon_reldir%/%C% usage). This enables avoiding explicit _SHORTNAME
>> +		# unecessary in many cases.
>
> s/unecessary //
>
>>  		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);
>> +		  }
>
> Will not the common $prefix always end with an underscore? In that
> case, why not simply use the length of $prefix instead? (And the
> comment above is misleading, since it fooled me into thinking that
> you do not handle underscores embedded in the filename correctly.
>

At this part of the code, I didn't want to reinforce that $derived is 
made up only from path components and the target file name. I wanted to 
have it work no matter how $derived is constructed (it's passed as a 
parameter to the transform function, so the logic how to built $derived 
is elsewhere).

In practice, you are right. I can make the change you suggested if wanted.

> *snip*
>
>> +$MAKE -f Makefile \
>> +	&& test -f one/one/test-test.o && test ! -f one/one/one_two_test-test.o \
>> +	&& test -f one/two/test-test.o && test ! -f one/two/one_two_test-test.o \
>> +	&& test -f one/two/sub/test-test.o && test ! -f one/two/sub/one_two_test-test.o \
>> +	&& test -f one/three/my_test-my_test.o && test ! -f one/three/one_three_my_test-my_test.o
>
> This is buggy. You need to consider the $OBJEXT variable.

You are right. Thanks.

Best regards