Re: [PATCH] new option: object-shortname

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] new option: object-shortname

Paul Eggert
Have you looked at the similar change that is planned for Automake 1.16? To
build that, you can do this:

git clone -b minor git://git.savannah.gnu.org/automake.git
cd automake
./bootstrap.sh
./configure --prefix=/your/favorite/directory
make

Assuming your needs aren't satisfied already by 1.16's plans, please look into
folding your code into draft 1.16.

I would either not bother with the paranoid check, or have Automake fail if the
paranoid check fails.

The main thing missing, though, is the documentation. It needs explanation in
the manual (doc/automake.texi), probably with your use case. Plus, NEWS needs
updating.

One more thing -- I assume you're OK with assigning copyright to the FSF for
your change? If so, I can get the ball rolling on that.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] new option: object-shortname

Thomas Martitz
Am 29.08.2016 um 04:17 schrieb Paul Eggert:
> Have you looked at the similar change that is planned for Automake 1.16?
> To build that, you can do this:

I've looked previously but haven't found anything helpful. But perhaps I
overlooked. I'll check again but can you give me a hint at which
change/new feature should I be looking at specifically?

I can only find the subdir-objects fix which is loosely of related. This
fix doesn't cover my use case. However, as my use case also involves
subdir-objects this fix is hugely appreciated anyway.

Thanks for looking at my patch. Yes, I'm OK with assigning copyright.
It's a relatively small change anyway (in terms of LOC changes).

I'll post an updated patch later today according to your comments.

Best regards


Reply | Threaded
Open this post in threaded view
|

[PATCH v2] new option: object-shortname

Thomas Martitz
This option is intended to be used in conjunction with subdir-objects and
Automake-time substitutions for included makefile fragments (%C%, %D%).
Enabling the option shortens the file name of object files such that the
prefix
derived (after canonicalization) from the path is not included.

Enabling the option is basically equivalent to setting foo_SHORTNAME =
foo. However, it also works flawlessly if a Makefile fragment is
conditionally included. Note that actually setting foo_SHORTNAME
still overrides the object name, regardless of this option. This can improve
the modularity of Automake-using projects.

Example:
without object-shortname
   sub/Makefile.am:
   bin_PROGRAMS += %D%/foo
   %C%_foo_CFLAGS = $(AM_CFLAGS) -g

results in objects:
   sub/sub_foo-foo.o

with object-shortname the object file name is:
   sub/foo-foo.o

And it allows the following in $(top_srcdir)/Makefile.am (not possible
with foo_SHORTNAME=f
   if ENABLE_SUB
   include sub/Makefile.am
   endif


0001-new-option-object-shortname.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] new option: object-shortname

Paul Eggert
Some minor comments:

> +  - This option affects the file name automake uses for object files.
> +
> +    Enabling the option shortens the file name such that the prefix
> +    derived (after canonicalization) from the path is not included. For
> +    instance, it is always foo-foo.o regardless of the path leading to the
> +    source file.
> +
> +    It does not change the directory where these object files will be placed.
> +    Thus, it is recommended to combine this option with subdir-objects.
> +
> +  - Please read the corresponding manual entry for an extensive description.

Please make the NEWS item a bit shorter. No need for the last sentence, for example.

> + if (option 'object-shortname') {

Please use the same style for indenting that the code already uses, with the {
on the next line and indented by two columns.

> + # If object-shortname is enabled the object's filename shall not contain the parts

Please keep everything within 80 columns.

> +If this option is specified, then object names constructed by automake are

Capitalize "automake" when used this way.

> +therefore ommitting the canonicalized path (@pxref{Canonicalization}). The

misspelled "omitting". Two spaces after sentence-ending periods.

> +effect is particularly visible if you use Makefile fragment inclusion feature

@file{Makefile}

> +foo}. However, it also works flawlessly if a Makefile fragment is

Omit "flawlessly".

> +This is best combined with @option{subdir-objects} because it file name
> +conflicts become more likely.

Can't parse this.

> +The rationale for this option is to allow a setup where there is a top-level
> +@file{Makefile.am} which includes fragments from subdirectories, which also
> +generate a Makefile.

Is one Makefile being generated, or many? It's not clear from the sentence.


Reply | Threaded
Open this post in threaded view
|

[PATCH v3] new option: object-shortname

Thomas Martitz
This option is intended to be used in conjunction with subdir-objects and
Automake-time substitutions for included makefile fragments (%C%, %D%).
Enabling the option shortens the file name of object files such that the
prefix
derived (after canonicalization) from the path is not included.

Enabling the option is basically equivalent to setting foo_SHORTNAME =
foo. However, it also works flawlessly if a Makefile fragment is
conditionally included. Note that actually setting foo_SHORTNAME
still overrides the object name, regardless of this option. This can improve
the modularity of Automake-using projects.

Example:
without object-shortname
   sub/Makefile.am:
   bin_PROGRAMS += %D%/foo
   %C%_foo_CFLAGS = $(AM_CFLAGS) -g

results in objects:
   sub/sub_foo-foo.o

with object-shortname the object file name is:
   sub/foo-foo.o

And it allows the following in $(top_srcdir)/Makefile.am (not possible
with foo_SHORTNAME=foo)
   if ENABLE_SUB
   include sub/Makefile.am
   endif

0001-new-option-object-shortname.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 29.08.2016 um 23:05 schrieb Thomas Martitz:
> This option is intended to be used in conjunction with subdir-objects and
> Automake-time substitutions for included makefile fragments (%C%, %D%).
> Enabling the option shortens the file name of object files such that
> the prefix
> derived (after canonicalization) from the path is not included.


Huge thanks for accepting my patch!

So, naturally I'm now eagerly awaiting a release (also for the much
needed fixes for subdir-objects and $(srcdir) substitution). Is there
any planning?

Best regards

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 04.09.2016 um 21:54 schrieb Thomas Martitz:

> Am 29.08.2016 um 23:05 schrieb Thomas Martitz:
>> This option is intended to be used in conjunction with subdir-objects
>> and
>> Automake-time substitutions for included makefile fragments (%C%, %D%).
>> Enabling the option shortens the file name of object files such that
>> the prefix
>> derived (after canonicalization) from the path is not included.
>
>
> Huge thanks for accepting my patch!

Err, Oops. I noticed my git clone was misconfigured and I misinterpreted
the log. My patch isn't accepted yet. Please tell me if there's anything
left for me to do?

Best regards

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Paul Eggert
Thomas Martitz wrote:
> Please tell me if there's anything left for me to do?

Not that I know of; someone needs to free up enough cycles to review it, that's all.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 09.09.2016 um 09:14 schrieb Paul Eggert:
> Thomas Martitz wrote:
>> Please tell me if there's anything left for me to do?
>
> Not that I know of; someone needs to free up enough cycles to review it,
> that's all.
>


Oh. I thought you'd review and merge. Who else can do it? Everyone else
is inactive apparently.

Best regards.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Paul Eggert
Thomas Martitz wrote:
> I thought you'd review and merge.

It might be me at some point. Just not today, I'm afraid.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 09.09.2016 um 10:54 schrieb Paul Eggert:
> Thomas Martitz wrote:
>> I thought you'd review and merge.
>
> It might be me at some point. Just not today, I'm afraid.
>

Hello,

I just wanted to re-new my review request. If there is still anything
wrong with my patch, please let me know.

The copyright assignment paperwork is completed.

Best regards.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 04.10.2016 um 09:49 schrieb Thomas Martitz:

> Am 09.09.2016 um 10:54 schrieb Paul Eggert:
>> Thomas Martitz wrote:
>>> I thought you'd review and merge.
>>
>> It might be me at some point. Just not today, I'm afraid.
>>
>
> Hello,
>
> I just wanted to re-new my review request. If there is still anything
> wrong with my patch, please let me know.
>
> The copyright assignment paperwork is completed.

This is another ping. Please review and merge my patch, or let me know
if there's work left to do.

Best regards.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 25.10.2016 um 11:11 schrieb Thomas Martitz:

> Am 04.10.2016 um 09:49 schrieb Thomas Martitz:
>> Am 09.09.2016 um 10:54 schrieb Paul Eggert:
>>> Thomas Martitz wrote:
>>>> I thought you'd review and merge.
>>>
>>> It might be me at some point. Just not today, I'm afraid.
>>>
>>
>
> This is another ping. Please review and merge my patch, or let me know
> if there's work left to do.
>
> Best regards.
>

Hello,

I'd appreciate if anyone could review and potentially merge the patch.
It really that large is it and shouldn't affect existing setups since
the new behavior is opt-in.

Best regards.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 21.11.2016 um 09:57 schrieb Thomas Martitz:

> Am 25.10.2016 um 11:11 schrieb Thomas Martitz:
>> Am 04.10.2016 um 09:49 schrieb Thomas Martitz:
>>> Am 09.09.2016 um 10:54 schrieb Paul Eggert:
>>>> Thomas Martitz wrote:
>>>>> I thought you'd review and merge.
>>>>
>>>> It might be me at some point. Just not today, I'm afraid.
>>>>
>>>
>>
>> This is another ping. Please review and merge my patch, or let me
>> know if there's work left to do.
>>
>> Best regards.
>>
>
> Hello,
>
> I'd appreciate if anyone could review and potentially merge the patch.
> It really that large is it and shouldn't affect existing setups since
> the new behavior is opt-in.

It *is not* that large...sorry for the typo.

Best regards

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
In reply to this post by Thomas Martitz
Am 04.10.2016 um 09:49 schrieb Thomas Martitz:

> Am 09.09.2016 um 10:54 schrieb Paul Eggert:
>> Thomas Martitz wrote:
>>> I thought you'd review and merge.
>>
>> It might be me at some point. Just not today, I'm afraid.
>>
>
> Hello,
>
> I just wanted to re-new my review request. If there is still anything
> wrong with my patch, please let me know.
>
> The copyright assignment paperwork is completed.

Hello,

this is one more attempt to get my patch reviewed. Can I assist in any way?

Best regards.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Paul Eggert
On 12/22/2016 06:38 AM, Thomas Martitz wrote:
> this is one more attempt to get my patch reviewed. Can I assist in any
> way?

Well, what we really need is an Automake maintainer, to do this sort of
review work. Is that something you'd be willing to do (and be qualified
for)? It's not a job to be undertaken lightly, of course.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Mathieu Lirzin
Hello,

Paul Eggert <[hidden email]> writes:

> On 12/22/2016 06:38 AM, Thomas Martitz wrote:
>> this is one more attempt to get my patch reviewed. Can I assist in
>> any way?
>
> Well, what we really need is an Automake maintainer, to do this sort
> of review work. Is that something you'd be willing to do (and be
> qualified for)? It's not a job to be undertaken lightly, of course.

The question was not directed towards myself, however I would be
interested in serving as Automake maintainer or co-maintainer.  I am not
a Perl expert yet, but I am quite knowledgeable about Automake usage.

WDYT?

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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
In reply to this post by Paul Eggert
Am 22.12.2016 um 19:20 schrieb Paul Eggert:
> On 12/22/2016 06:38 AM, Thomas Martitz wrote:
>> this is one more attempt to get my patch reviewed. Can I assist in
>> any way?
>
> Well, what we really need is an Automake maintainer, to do this sort
> of review work. Is that something you'd be willing to do (and be
> qualified for)? It's not a job to be undertaken lightly, of course.
>

What would qualify me? I've touched Automake sources very lightly so far
and I'm far from being a perl expert.

I also don't know which direction Automake should take, I have no
vision, so I couldn't really do anything more than operate in
maintenance mode.

On the other hand, any maintainer is better than no maintainer.

Since my dayjob depends on Automake I could probably devote some small
but fixed amount of time to Automake maintenance.

Best regards.



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Jim Meyering
In reply to this post by Thomas Martitz
On Mon, Aug 29, 2016 at 11:05 PM, Thomas Martitz <[hidden email]> wrote:

> This option is intended to be used in conjunction with subdir-objects and
> Automake-time substitutions for included makefile fragments (%C%, %D%).
> Enabling the option shortens the file name of object files such that the
> prefix
> derived (after canonicalization) from the path is not included.
>
> Enabling the option is basically equivalent to setting foo_SHORTNAME =
> foo. However, it also works flawlessly if a Makefile fragment is
> conditionally included. Note that actually setting foo_SHORTNAME
> still overrides the object name, regardless of this option. This can improve
> the modularity of Automake-using projects.
>
> Example:
> without object-shortname
>   sub/Makefile.am:
>   bin_PROGRAMS += %D%/foo
>   %C%_foo_CFLAGS = $(AM_CFLAGS) -g
>
> results in objects:
>   sub/sub_foo-foo.o
>
> with object-shortname the object file name is:
>   sub/foo-foo.o
>
> And it allows the following in $(top_srcdir)/Makefile.am (not possible with
> foo_SHORTNAME=foo)
>
>   if ENABLE_SUB
>   include sub/Makefile.am
>   endif

Hi Thomas,

Thanks for the addition.

I've only spent a few minutes reading discussion about this patch and
even less looking at the actual code, but so far, I have seen no
addition to the test suite. I suggest you copy an existing test as a
starting point, add something like the above in its Makefile.am
section, and then ensure that the new artifacts appear in the
generated Makefile.in.

Automake is at a point in its development for which we should ensure
(whenever possible) that any nontrivial change includes a test suite
addition to exercise the new behavior. If it's a bug fix, the new
test(s) should fail before the fix and pass once it is applied. For a
feature addition like this, it is often similar: the new test should
exercise/demonstrate the new code, and should usually fail without the
patch.

Jim

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] new option: object-shortname

Thomas Martitz
Am 05.01.2017 um 15:31 schrieb Jim Meyering:

> On Mon, Aug 29, 2016 at 11:05 PM, Thomas Martitz <[hidden email]> wrote:
>> This option is intended to be used in conjunction with subdir-objects and
>> Automake-time substitutions for included makefile fragments (%C%, %D%).
>> Enabling the option shortens the file name of object files such that the
>> prefix
>> derived (after canonicalization) from the path is not included.
>>
>> Enabling the option is basically equivalent to setting foo_SHORTNAME =
>> foo. However, it also works flawlessly if a Makefile fragment is
>> conditionally included. Note that actually setting foo_SHORTNAME
>> still overrides the object name, regardless of this option. This can improve
>> the modularity of Automake-using projects.
>>
>> Example:
>> without object-shortname
>>   sub/Makefile.am:
>>   bin_PROGRAMS += %D%/foo
>>   %C%_foo_CFLAGS = $(AM_CFLAGS) -g
>>
>> results in objects:
>>   sub/sub_foo-foo.o
>>
>> with object-shortname the object file name is:
>>   sub/foo-foo.o
>>
>> And it allows the following in $(top_srcdir)/Makefile.am (not possible with
>> foo_SHORTNAME=foo)
>>
>>   if ENABLE_SUB
>>   include sub/Makefile.am
>>   endif
>
> Hi Thomas,
>
> Thanks for the addition.
>
> I've only spent a few minutes reading discussion about this patch and
> even less looking at the actual code, but so far, I have seen no
> addition to the test suite. I suggest you copy an existing test as a
> starting point, add something like the above in its Makefile.am
> section, and then ensure that the new artifacts appear in the
> generated Makefile.in.
>
You are absolutely right. I added a test case. I hope this is alright?

Best regards


0001-new-option-object-shortname.patch (8K) Download Attachment
0002-tests-add-test-for-new-object-shortname-option.patch (3K) Download Attachment
12