target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Doug Evans]

On Mon, Mar 3, 2014 at 5:36 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 03/04/2014 09:18 AM, Hui Zhu wrote:
>> I cannot understand about this OB is not right.  I have 2 questions to you:
>> 1. Before my patch, does target-delegates.c that generated by make-target-delegates is same with current target-delegates.c?
>
> No, as I said, I forgot to re-generate target-delegates.c.

Hmmm....

I don't even see target-delegates.c in Makefile.in.  That feels like a
bug. [Could be blind of course. :-)]
I realize there's a comment in target-delegates.c that says how to
regenerate it, but these kind of things are part of what makefiles are
for.

While I realize we don't want to require perl for building gdb (and I
for one would never advocate it), I wonder if we can do at least a bit
better.
I'm not sure I'd want to require perl for --enable-maintainer-mode
(which is a common trigger for enabling in makefiles the appropriate
rules to auto-regenerate checked-in machine-generated files), but it's
one thought.  Failing using --enable-maintainer-mode for this I think
it's a requirement to add a different --enable-foo option to turn on
the necessary makefile rules to regenerate target-delegates.c at build
time as needed.

Another thought would be to at least have makefile issue a warning if
target-delegates.c is out of date, perhaps predicated on
--enable-maintainer-mode, or some other configure option, since doing
so by default is problematic otherwise.

Re: target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Pedro Alves]

On 03/06/2014 05:20 AM, Doug Evans wrote:
> On Mon, Mar 3, 2014 at 5:36 PM, Yao Qi <yao@codesourcery.com> wrote:
>> On 03/04/2014 09:18 AM, Hui Zhu wrote:
>>> I cannot understand about this OB is not right.  I have 2 questions to you:
>>> 1. Before my patch, does target-delegates.c that generated by make-target-delegates is same with current target-delegates.c?
>>
>> No, as I said, I forgot to re-generate target-delegates.c.
> 
> Hmmm....
> 
> I don't even see target-delegates.c in Makefile.in.  That feels like a
> bug. [Could be blind of course. :-)]
> I realize there's a comment in target-delegates.c that says how to
> regenerate it, but these kind of things are part of what makefiles are
> for.

This also crossed my mind when initially reviewing the series (and
I'm sure Tom's too when writing it, as it's such an obvious thing),
but realized this is really no different from e.g., gdbarch.h|c.

So given the precedent, I don't consider this a particular bug of
target-delegates.c, but a more generic "IWBN if we had Makefile
rules for our generated files".

Of course, I'd welcome patches in that direction.

> I'm not sure I'd want to require perl for --enable-maintainer-mode
> (which is a common trigger for enabling in makefiles the appropriate
> rules to auto-regenerate checked-in machine-generated files), but it's
> one thought.

I don't see a problem there.  automake is perl as well, for instance,
and it's common for --enable-maintainer-mode to trigger automake/aclocal.

Even if that weren't true, by configuring with --enable-maintainer-mode,
by definition you're asserting you have the tools required for
regular gdb maintenance, and given make-target-delegates is perl,
well, any maintainer must have it handy.

-- 
Pedro Alves

Re: target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Doug Evans]

On Fri, Mar 7, 2014 at 10:33 AM, Pedro Alves <palves@redhat.com> wrote:
> On 03/06/2014 05:20 AM, Doug Evans wrote:
>> On Mon, Mar 3, 2014 at 5:36 PM, Yao Qi <yao@codesourcery.com> wrote:
>>> On 03/04/2014 09:18 AM, Hui Zhu wrote:
>>>> I cannot understand about this OB is not right.  I have 2 questions to you:
>>>> 1. Before my patch, does target-delegates.c that generated by make-target-delegates is same with current target-delegates.c?
>>>
>>> No, as I said, I forgot to re-generate target-delegates.c.
>>
>> Hmmm....
>>
>> I don't even see target-delegates.c in Makefile.in.  That feels like a
>> bug. [Could be blind of course. :-)]
>> I realize there's a comment in target-delegates.c that says how to
>> regenerate it, but these kind of things are part of what makefiles are
>> for.
>
> This also crossed my mind when initially reviewing the series (and
> I'm sure Tom's too when writing it, as it's such an obvious thing),
> but realized this is really no different from e.g., gdbarch.h|c.
>
> So given the precedent, I don't consider this a particular bug of
> target-delegates.c, but a more generic "IWBN if we had Makefile
> rules for our generated files".

It wouldn't be gdb if finding one bug didn't expose one or several more. :-)

In this case, these kinds of things are trivial to write, IMO they
should be part of the patch.
It's s.o.p.

> Of course, I'd welcome patches in that direction.

I'd say let's not make the problem worse.
This is trivial stuff, and someone has already tripped over this.
[--enable-maintainer-mode would likely *not* have prevented it the
first time for any individual (they're likely not aware of
--enable-maintainer-mode), but we *should* be providing the means for
this to automagically work]

>> I'm not sure I'd want to require perl for --enable-maintainer-mode
>> (which is a common trigger for enabling in makefiles the appropriate
>> rules to auto-regenerate checked-in machine-generated files), but it's
>> one thought.
>
> I don't see a problem there.  automake is perl as well, for instance,
> and it's common for --enable-maintainer-mode to trigger automake/aclocal.

I recognize the *technical* need of having perl installed for use by
automake, but that's not the context in which my comment is phrased.
How much perl do I need to know in order to use automake?
[If perl isn't installed and I do "yum install automake", the
necessary bits of perl get installed (I hope) but Joe Developer
needn't be aware of that.]
My comment is phrased the way it is because I was not prepared to
a-priori declare one needs to know perl when --enable-maintainer-mode
is turned on: If the user turns it on and that exposes a
problem/issue/whatever with target-delegates.c, then I expect the user
to have to deal with it, it's one more piece if implementation
infrastructure we've imposed on developers.  Today, if a developer
turns on --enable-maintainer-mode I'm not going to apologize for
having imposed on them some minimal comfortability with autoconf.
Typically one needn't know very much (hopefully just having the right
version installed will solve their problem), but IME that's not always
the case.
Help is always at hand of course, still there's a non-trivial
additional load being imposed on developers, and I'm not prepared to
a-priori impose it.

btw, for reference sake,
I can imagine another reason for wanting something other than
--enable-maintainer-mode:
Depending on what files one is expecting to be editing (configure.ac
or target.h or ...?) one might not have the right versions of the
tools installed in order to avoid spurious difference in the generated
files, and I can imagine IWBN if I was hacking on one file to not want
to have to make sure I have the right versions of all the tools that
--enable-maintainer-mode uses.
This is more IWBN though, the converse of course is a growing number
of --enable-foo options which has its own problems.

> Even if that weren't true, by configuring with --enable-maintainer-mode,
> by definition you're asserting you have the tools required for
> regular gdb maintenance, and given make-target-delegates is perl,
> well, any maintainer must have it handy.

Re: target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Pedro Alves]

Sorry, dropped the ball on this one.

On 03/09/2014 11:16 PM, Doug Evans wrote:
> On Fri, Mar 7, 2014 at 10:33 AM, Pedro Alves <palves@redhat.com> wrote:

>> I don't see a problem there.  automake is perl as well, for instance,
>> and it's common for --enable-maintainer-mode to trigger automake/aclocal.
> 
> I recognize the *technical* need of having perl installed for use by
> automake, but that's not the context in which my comment is phrased.
> How much perl do I need to know in order to use automake?

None.  Just as no perl knowledge is required to run to use new script.

> My comment is phrased the way it is because I was not prepared to
> a-priori declare one needs to know perl when --enable-maintainer-mode
> is turned on:

Note --enable-maintainer-mode is irrelevant in that consideration.  If
the file needs regeneration, it'll need it no matter whether
--enable-maintainer-mode is in effect.  --enable-maintainer-mode
just runs makes generator tools run automatically based on timestamps.
(and it's not on by default AFAIK because unpacking release
source tarballs may break the timestamps and the makefile would
otherwise try to regenerate the generated files).

> If the user turns it on and that exposes a
> problem/issue/whatever with target-delegates.c, then I expect the user
> to have to deal with it, it's one more piece if implementation
> infrastructure we've imposed on developers.  Today, if a developer
> turns on --enable-maintainer-mode I'm not going to apologize for
> having imposed on them some minimal comfortability with autoconf.

I think it's no issue really.  I myself don't know that much perl,
but I assume that if I do stumble on some problem, it shouldn't
be hard to fix, or if is, I'll just ask for help.  The generated
file is checked into the tree.  If joe-hacker changes target.h and
the generator fails, I'm sure joe-hack can just report the issue,
tweak the generated file by hand, and move on until someone else
fixes the issue.  Perl hackers shouldn't be hard to find.  This is
not that different IMO from say, finding a bug in gdbarch.sh.
Now I wouldn't expect random-joe-hacker to know that much about
debugging shell scripts either.

> Typically one needn't know very much (hopefully just having the right
> version installed will solve their problem), but IME that's not always
> the case.
> Help is always at hand of course, still there's a non-trivial
> additional load being imposed on developers, and I'm not prepared to
> a-priori impose it.

I honestly believe problems with the script itself will
be quite rare.  If it turns out more problematic than expected,
we can always consider rewriting it in another language then.

-- 
Pedro Alves

Re: target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Yao Qi]

On 03/06/2014 01:20 PM, Doug Evans wrote:
> While I realize we don't want to require perl for building gdb (and I
> for one would never advocate it), I wonder if we can do at least a bit
> better.

perl is required by git, and GDB source is version controlled by git.
I assume that most of the host machine used for gdb development have
git and perl installed.  People may get source from release, and
generate patch on top of it (without git/perl installed).  It isn't
common, is it?

Anyway, we can check whether perl is installed.  If perl is installed,
run make-target-delegates during make and error out if new-generated
target-delegates.c is different from the one in gdb source.  Is it good?
If yes, I'll submit a patch for this.

Using --enable-maintainer-mode or adding new --enable-foo option isn't
appealing to me.

-- 
Yao (齐尧)

Re: target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Hui Zhu]

Now, GDB has supported python.  And most distributions depth depend on
the python.
What about change this script to python.

Thanks,
Hui

On Mon, Mar 10, 2014 at 10:45 AM, Yao Qi <yao@codesourcery.com> wrote:
> On 03/06/2014 01:20 PM, Doug Evans wrote:
>> While I realize we don't want to require perl for building gdb (and I
>> for one would never advocate it), I wonder if we can do at least a bit
>> better.
>
> perl is required by git, and GDB source is version controlled by git.
> I assume that most of the host machine used for gdb development have
> git and perl installed.  People may get source from release, and
> generate patch on top of it (without git/perl installed).  It isn't
> common, is it?
>
> Anyway, we can check whether perl is installed.  If perl is installed,
> run make-target-delegates during make and error out if new-generated
> target-delegates.c is different from the one in gdb source.  Is it good?
> If yes, I'll submit a patch for this.
>
> Using --enable-maintainer-mode or adding new --enable-foo option isn't
> appealing to me.
>
> --
> Yao (齐尧)

Re: target-delegates.c needs some TLC [was Re: [OB PATCH] target.h (to_traceframe_info): Fix TARGET_DEFAULT_RETURN]

[Doug Evans]

On Sun, Mar 9, 2014 at 7:45 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 03/06/2014 01:20 PM, Doug Evans wrote:
>> While I realize we don't want to require perl for building gdb (and I
>> for one would never advocate it), I wonder if we can do at least a bit
>> better.
>
> perl is required by git, and GDB source is version controlled by git.
> I assume that most of the host machine used for gdb development have
> git and perl installed.  People may get source from release, and
> generate patch on top of it (without git/perl installed).  It isn't
> common, is it?

Apologies for not being clear enough.
This is not, and never has been, an issue of needing perl installed per se.
The issue is whether we should impose on people the requirement of
some perl knowledge, however minimal, when one turns on
--enable-maintainer-mode.
If people decide to, fine by me.  But I was not prepared to do that
without a discussion.

> Anyway, we can check whether perl is installed.  If perl is installed,
> run make-target-delegates during make and error out if new-generated
> target-delegates.c is different from the one in gdb source.  Is it good?
> If yes, I'll submit a patch for this.

There is no need for this AFAICS.
Does AM_MAINTAINER_MODE do this for autoconf?
Maybe it does and I can't find it.
And if it doesn't do so for autoconf, why do it for perl?