CHECK_STRUCTS/dmpstruct.h mechanism is broken.

classic Classic list List threaded Threaded
68 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Alan Mackenzie
Hello, Emacs.

The CHECK_STRUCT/dmpstruct.h mechanism is a very clever way of ensuring
that Emacs cannot be built after amending certain structs, or even the
comments within them.

I have amended such a comment, thus cannot build my Emacs.

This mechanism is broken, since it would appear to be entirely
undocumented.  Or does this documentation exist, somewhere obscure?
There is nothing about it in INSTALL.REPO, for example.

For the sake of my sanity, will whoever it is please tell me what I have
now to do to build my Emacs.  make bootstrap fails.  This is a bug; make
bootstrap should _never_ fail.  I've done make dmpstruct.h.  Again to no
avail.

There's a file dmpstruct.awk involved in this, but it contains no
instructions in its header comments; how is it meant to be called for
example?  What else needs to be run to make it work, for another
example?

And what's it all for?  Why should make bootstrap be broken?  What's the
point of all this?  Why are there no explanatory comments in the
generated file dmpstruct.h?

Help!

--
Alan Mackenzie (Nuremberg, Germany).

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Eli Zaretskii
> Date: Thu, 28 Feb 2019 20:21:46 +0000
> From: Alan Mackenzie <[hidden email]>
>
> For the sake of my sanity, will whoever it is please tell me what I have
> now to do to build my Emacs.  make bootstrap fails.  This is a bug; make
> bootstrap should _never_ fail.  I've done make dmpstruct.h.  Again to no
> avail.

I don't understand why it doesn't work for you.  Just to make sure
there's no mystery, I renamed src/dmpstruct.h and then said "make" at
top level -- the file was regenerated, and the result was identical to
the renamed version; and then a new Emacs binary was built
successfully.

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Alan Mackenzie
In reply to this post by Alan Mackenzie
Hello, Emacs.

On Thu, Feb 28, 2019 at 20:21:46 +0000, Alan Mackenzie wrote:
> The CHECK_STRUCT/dmpstruct.h mechanism is a very clever way of ensuring
> that Emacs cannot be built after amending certain structs, or even the
> comments within them.

> I have amended such a comment, thus cannot build my Emacs.

> This mechanism is broken, since it would appear to be entirely
> undocumented.  Or does this documentation exist, somewhere obscure?
> There is nothing about it in INSTALL.REPO, for example.

> For the sake of my sanity, will whoever it is please tell me what I have
> now to do to build my Emacs.  make bootstrap fails.  This is a bug; make
> bootstrap should _never_ fail.  I've done make dmpstruct.h.  Again to no
> avail.

OK, I've worked out what's to be done, and done it.  dmpstruct.h is a
file containing up to date hashes of structures.  The old hash in
pdumper.c needs to replaced by hand by the updated hash from
dmpstruct.h.  My Emacs now builds.

This is anything but clear from the comment at L71 in pdumper.c.  It
says "and update the hash..." without saying which hash, and without
saying how.  Which utility is needed to update this hash?  (Answer:
none).

> There's a file dmpstruct.awk involved in this, but it contains no
> instructions in its header comments; how is it meant to be called for
> example?  What else needs to be run to make it work, for another
> example?

> And what's it all for?  Why should make bootstrap be broken?  What's the
> point of all this?  Why are there no explanatory comments in the
> generated file dmpstruct.h?

Again, is all this really needed?  Is pdumper.c really that fragile,
that it can't cope with changes in certain structs?

> Help!

> --
> Alan Mackenzie (Nuremberg, Germany).

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Eli Zaretskii
> Date: Thu, 28 Feb 2019 20:59:55 +0000
> From: Alan Mackenzie <[hidden email]>
>
> OK, I've worked out what's to be done, and done it.  dmpstruct.h is a
> file containing up to date hashes of structures.  The old hash in
> pdumper.c needs to replaced by hand by the updated hash from
> dmpstruct.h.  My Emacs now builds.
>
> This is anything but clear from the comment at L71 in pdumper.c.  It
> says "and update the hash..." without saying which hash, and without
> saying how.  Which utility is needed to update this hash?  (Answer:
> none).

Please feel free to improve the comments to clarify anything that is
not crystal clear.

TIA

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Alan Mackenzie
Hello, Eli.

On Fri, Mar 01, 2019 at 09:31:08 +0200, Eli Zaretskii wrote:
> > Date: Thu, 28 Feb 2019 20:59:55 +0000
> > From: Alan Mackenzie <[hidden email]>

> > OK, I've worked out what's to be done, and done it.  dmpstruct.h is a
> > file containing up to date hashes of structures.  The old hash in
> > pdumper.c needs to replaced by hand by the updated hash from
> > dmpstruct.h.  My Emacs now builds.

> > This is anything but clear from the comment at L71 in pdumper.c.  It
> > says "and update the hash..." without saying which hash, and without
> > saying how.  Which utility is needed to update this hash?  (Answer:
> > none).

> Please feel free to improve the comments to clarify anything that is
> not crystal clear.

Done.

> TIA

--
Alan Mackenzie (Nuremberg, Germany).

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Paul Eggert
In reply to this post by Alan Mackenzie
On 2/28/19 12:59 PM, Alan Mackenzie wrote:
> is all this really needed?  Is pdumper.c really that fragile,
> that it can't cope with changes in certain structs?

No, it's not needed, and in my experience the mechanism's costs far
exceed any benefit. Every time it has complained to me, it has been a
false alarm. Conversely, it has not caught any of the mistakes I've made
when making changes that affect the pdumper. So its batting average is
.000 and it's cost me far too much irritation (admittedly minor each
time, but it adds up ...).

The Emacs source code has thousands of interconnections, all of which
have to be just right to get it to work. There is nothing special about
the interconnections to the pdumper that justify this extra level of
bureaucracy.


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Paul Eggert
On 3/4/19 6:17 PM, Paul Eggert wrote:
> On 2/28/19 12:59 PM, Alan Mackenzie wrote:
>> is all this really needed?  Is pdumper.c really that fragile,
>> that it can't cope with changes in certain structs?
> No, it's not needed, and in my experience the mechanism's costs far
> exceed any benefit.

No further comment and the mechanism just bit me again, so I installed
the attached patch to disable it. We can reenable it later if needed
(which I hope won't happen....).


0001-Remove-dmpstruct.h.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Andy Moreton-3
On Tue 09 Apr 2019, Paul Eggert wrote:

> On 3/4/19 6:17 PM, Paul Eggert wrote:
>> On 2/28/19 12:59 PM, Alan Mackenzie wrote:
>>> is all this really needed?  Is pdumper.c really that fragile,
>>> that it can't cope with changes in certain structs?
>> No, it's not needed, and in my experience the mechanism's costs far
>> exceed any benefit.
>
> No further comment and the mechanism just bit me again, so I installed
> the attached patch to disable it. We can reenable it later if needed
> (which I hope won't happen....).

This patch seems to be ok, but your following patch in commit d826037475
("Remove the need for temacs.in") breaks out of tree builds.

It seems to me that the patch contains a mixture of changes to remove
temacs.in support, and a number of unrelated changes to Makefile.in
files which break the build.

For example, this breaks out-of-tree builds, as make tries to use
/path/to/emacs/admin/charsets/Makefile rather than the correct path
<builddir>/admin/charsets/Makefile:

    diff --git a/src/Makefile.in b/src/Makefile.in
    index 0613a0dbed..f8a2ffadc2 100644
    --- a/src/Makefile.in
    +++ b/src/Makefile.in
    @@ -533,7 +533,7 @@ ${lispintdir}/cp51932.el ${lispintdir}/eucjp-ms.el:

     charsets = ${top_srcdir}/admin/charsets/charsets.stamp
     ${charsets}: FORCE
    - ${MAKE} -C ../admin/charsets all
    + $(MAKE) -C $(dir $@) all

     charscript = ${lispintdir}/charscript.el
     ${charscript}: FORCE

The unrelated changes should have been committed in a separate patch
for easier bisection: a patch should contain a single logical change.

Please revert all of the unrelated makefile path handling changes so
master is buildable (the example above is one of many breakages).

    AndyM


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Andy Moreton-3
On Wed 10 Apr 2019, Andy Moreton wrote:

> On Tue 09 Apr 2019, Paul Eggert wrote:
>
>> On 3/4/19 6:17 PM, Paul Eggert wrote:
>>> On 2/28/19 12:59 PM, Alan Mackenzie wrote:
>>>> is all this really needed?  Is pdumper.c really that fragile,
>>>> that it can't cope with changes in certain structs?
>>> No, it's not needed, and in my experience the mechanism's costs far
>>> exceed any benefit.
>>
>> No further comment and the mechanism just bit me again, so I installed
>> the attached patch to disable it. We can reenable it later if needed
>> (which I hope won't happen....).
>
> This patch seems to be ok, but your following patch in commit d826037475
> ("Remove the need for temacs.in") breaks out of tree builds.
>
> It seems to me that the patch contains a mixture of changes to remove
> temacs.in support, and a number of unrelated changes to Makefile.in
> files which break the build.
>
> For example, this breaks out-of-tree builds, as make tries to use
> /path/to/emacs/admin/charsets/Makefile rather than the correct path
> <builddir>/admin/charsets/Makefile:
>
>     diff --git a/src/Makefile.in b/src/Makefile.in
>     index 0613a0dbed..f8a2ffadc2 100644
>     --- a/src/Makefile.in
>     +++ b/src/Makefile.in
>     @@ -533,7 +533,7 @@ ${lispintdir}/cp51932.el ${lispintdir}/eucjp-ms.el:
>
>      charsets = ${top_srcdir}/admin/charsets/charsets.stamp
>      ${charsets}: FORCE
>     - ${MAKE} -C ../admin/charsets all
>     + $(MAKE) -C $(dir $@) all
>
>      charscript = ${lispintdir}/charscript.el
>      ${charscript}: FORCE
>
> The unrelated changes should have been committed in a separate patch
> for easier bisection: a patch should contain a single logical change.
>
> Please revert all of the unrelated makefile path handling changes so
> master is buildable (the example above is one of many breakages).

The following changes (a partial revert of commit d826037475) make it
possible to build emacs out of tree on Windows (msys2 mingw64):


diff --git a/src/Makefile.in b/src/Makefile.in
index f8a2ffadc2..6816b8bf8d 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -533,7 +533,7 @@ ${lispintdir}/cp51932.el ${lispintdir}/eucjp-ms.el:
 
 charsets = ${top_srcdir}/admin/charsets/charsets.stamp
 ${charsets}: FORCE
- $(MAKE) -C $(dir $@) all
+ $(MAKE) -C ../admin/charsets all
 
 charscript = ${lispintdir}/charscript.el
 ${charscript}: FORCE
@@ -586,7 +586,7 @@ $(etc)/DOC:
 
 $(libsrc)/make-docfile$(EXEEXT) $(libsrc)/make-fingerprint$(EXEEXT): \
   $(lib)/libgnu.a
- $(MAKE) -C $(dir $@) $(notdir $@)
+ $(MAKE) -C $(libsrc) make-docfile$(EXEEXT)
 
 buildobj.h: Makefile
  $(AM_V_GEN)for i in $(ALLOBJS); do \
@@ -614,7 +614,7 @@ $(ALLOBJS):
 LIBEGNU_ARCHIVE = $(lib)/lib$(if $(HYBRID_MALLOC),e)gnu.a
 
 $(LIBEGNU_ARCHIVE): $(config_h)
- $(MAKE) -C $(dir $@) all
+ $(MAKE) -C $(lib) all
 
 FINGERPRINTED = $(LIBXMENU) $(ALLOBJS) $(LIBEGNU_ARCHIVE) $(EMACSRES)
 fingerprint.c: $(FINGERPRINTED) $(libsrc)/make-fingerprint$(EXEEXT)
@@ -639,15 +639,15 @@ temacs$(EXEEXT):
 ## The following oldxmenu-related rules are only (possibly) used if
 ## HAVE_X11 && !USE_GTK, but there is no harm in always defining them.
 $(lwlibdir)/liblw.a: $(config_h) globals.h lisp.h FORCE
- $(MAKE) -C $(dir $@) $(notdir $@)
+ $(MAKE) -C $(lwlibdir) liblw.a
 $(oldXMenudir)/libXMenu11.a: FORCE
- $(MAKE) -C $(dir $@) $(notdir $@)
+ $(MAKE) -C -C $(oldXMenudir) libXMenu11.a
 FORCE:
 .PHONY: FORCE
 
 .PRECIOUS: ../config.status Makefile
 ../config.status: $(top_srcdir)/configure.ac $(top_srcdir)/m4/*.m4
- $(MAKE) -C $(dir $@) $(notdir $@)
+ $(MAKE) -C .. $(notdir $@)
 Makefile: ../config.status $(srcdir)/Makefile.in
  $(MAKE) -C .. src/$@
 
@@ -703,7 +703,7 @@ extraclean:
 ETAGS = ../lib-src/etags${EXEEXT}
 
 ${ETAGS}: FORCE
- $(MAKE) -C $(dir $@) $(notdir $@)
+ $(MAKE) -C ../lib-src $(notdir $@)
 
 # Remove macuvs.h and fingerprint.c since they'd cause `src/emacs`
 # to be built before we can get TAGS.
@@ -728,8 +728,11 @@ TAGS:
 
 ## Arrange to make tags tables for ../lisp and ../lwlib,
 ## which the above TAGS file for the C files includes by reference.
-../lisp/TAGS $(lwlibdir)/TAGS: FORCE
- $(MAKE) -C $(dir $@) $(notdir $@) ETAGS="$(ETAGS)"
+../lisp/TAGS: FORCE
+ $(MAKE) -C ../lisp TAGS ETAGS="$(ETAGS)"
+
+$(lwlibdir)/TAGS: FORCE
+ $(MAKE) -C $(lwlibdir) TAGS ETAGS="$(ETAGS)"
 
 tags: TAGS ../lisp/TAGS $(lwlibdir)/TAGS
 .PHONY: tags
@@ -765,7 +768,7 @@ VCSWITNESS =
 
 $(lispsource)/loaddefs.el: $(VCSWITNESS) | \
  bootstrap-emacs$(EXEEXT) $(bootstrap_pdmp)
- $(MAKE) -C $(dir $@) autoloads EMACS="$(bootstrap_exe)"
+ $(MAKE) -C ../lisp autoloads EMACS="$(bootstrap_exe)"
 
 ## Dump an Emacs executable named bootstrap-emacs containing the
 ## files from loadup.el in source form.


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Alan Mackenzie
In reply to this post by Paul Eggert
Hello, Paul.

On Tue, Apr 09, 2019 at 15:47:17 -0700, Paul Eggert wrote:
> On 3/4/19 6:17 PM, Paul Eggert wrote:
> > On 2/28/19 12:59 PM, Alan Mackenzie wrote:
> >> is all this really needed?  Is pdumper.c really that fragile,
> >> that it can't cope with changes in certain structs?
> > No, it's not needed, and in my experience the mechanism's costs far
> > exceed any benefit.

> No further comment and the mechanism just bit me again, so I installed
> the attached patch to disable it.

Why?  Completely removing this mechanism seems very heavy handed.  All
that was needed to disable it was a single line change to set
CHECK_STRUCTS to zero.

You would have got further comment if you'd proposed a patch, rather
than just a vague idea that might or might not have been followed
through.  Discussing the merits of a patch is always best done before
committing it to master, not afterwards.

What does Daniel say?

> We can reenable it later if needed (which I hope won't happen....).

Funnily enough, it was of use to me recently when it reminded me to
amend dump_subr after extending struct subr.

My main problem with this mechanism was the vagueness of the error
messages it generated and of the comment in the code they pointed at,
which wasted lots of time.  Those defects have since been fixed.

> >>From 891e507d06c3bfcd9ac181de6bb0ff9c27dfa4aa Mon Sep 17 00:00:00 2001
> From: Paul Eggert <[hidden email]>
> Date: Tue, 9 Apr 2019 15:42:10 -0700
> Subject: [PATCH] Remove dmpstruct.h
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The hassles of updating the dmpstruct.h-using code bit me again.
> These updates are more trouble than they???re worth.  See:
> https://lists.gnu.org/r/emacs-devel/2019-03/msg00122.html
> As I???m the main person who???s made changes in this area since
> dmpstruct.h was introduced, I???m the most motivated to clean up
> the situation.
> * make-dist (possibly_non_vc_files): Remove src/dmpstruct.h.
> * src/Makefile.in (dmpstruct_headers, dmpstruct.h): Remove.
> (pdumper.o): Do not depend on dmpstruct.h.
> (mostlyclean): Do not remove dmpstruct.h.
> * src/dmpstruct.awk: Remove.
> * src/pdumper.c: Do not include dmpstruct.h.
> (CHECK_STRUCTS): Remove.  All uses removed.
> ---
>  .gitignore        |  1 -
>  make-dist         |  2 +-
>  src/Makefile.in   | 10 +-----
>  src/dmpstruct.awk | 45 ------------------------
>  src/pdumper.c     | 89 -----------------------------------------------
>  5 files changed, 2 insertions(+), 145 deletions(-)
>  delete mode 100755 src/dmpstruct.awk

[ .... ]

--
Alan Mackenzie (Nuremberg, Germany).

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Paul Eggert
In reply to this post by Andy Moreton-3
On 4/10/19 6:12 AM, Andy Moreton wrote:
> this breaks out-of-tree builds, as make tries to use
> /path/to/emacs/admin/charsets/Makefile rather than the correct path
> <builddir>/admin/charsets/Makefile:

Sorry, my tests didn't include a complete out-of-tree bootstrap. I
installed the attached patch to fix that problem, along with the
problem's other instance. The other -C changes should be OK. You're
correct that some of the -C changes were unrelated to the original
patch. Still, they make the Makefile more consistent and I'd like to
keep the ones that work.

PS. I'd like Emacs to switch to a nonrecursive 'make' process, as this
subsidiary $(MAKE) -C business is for the birds. We've made the switch
for other GNU projects and it's been a win. This is not a new idea; see:

Miller P. Recursive Make considered harmful. AAUGN J of AUUG.
1998;19(1):14-25. https://grosskurth.ca/bib/1997/miller.pdf


0001-Fix-MAKE-C-for-out-of-tree-bootstraps.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Paul Eggert
In reply to this post by Alan Mackenzie
On 4/10/19 9:22 AM, Alan Mackenzie wrote:
> All that was needed to disable it was a single line change to set
> CHECK_STRUCTS to zero.

That would also have sufficed, but the code clutter (and slowdown in the
build due to the computation of hashes which forces that part of the
build to be sequential) was also annoying. If you really want it we can
bring back the mechanism (with CHECK_STRUCTS off by default please!),
though I'd rather not - see below.

> it was of use to me recently when it reminded me to
> amend dump_subr after extending struct subr.
I didn't observe that in master - presumably this was in some
experimental branch? In master, dump_subr is currently the same as it
was when the pdumper was installed.

Before making the recent change, I reviewed all the changes made to
pdumper.c in master, and observed none where the hashes helped and
several where they hurt. Since the portable dumper was added I have made
four commits that involved the hashes, and the hashes only got into my
way and slowed me down. You made one commit
(9c0fa1172fd987a8f23b115145270383a11c12fc) that involved the buffer.h
hash, and portions of that commit were mistakenly pushed in pdumper.c's
previous commit so the hashes didn't seem to have helped there. The only
other persons to make hash-related commits to pdumper.c were Alan and
Stefan, and the hashes didn't help there either.

As the hashes get in the way of ordinary development (mostly affecting
me) and don't seem to help in practice, I really want them to go.


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Daniel Colascione-5
In reply to this post by Alan Mackenzie
> Hello, Paul.
>
> On Tue, Apr 09, 2019 at 15:47:17 -0700, Paul Eggert wrote:
>> On 3/4/19 6:17 PM, Paul Eggert wrote:
>> > On 2/28/19 12:59 PM, Alan Mackenzie wrote:
>> >> is all this really needed?  Is pdumper.c really that fragile,
>> >> that it can't cope with changes in certain structs?
>> > No, it's not needed, and in my experience the mechanism's costs far
>> > exceed any benefit.
>
>> No further comment and the mechanism just bit me again, so I installed
>> the attached patch to disable it.
>
> Why?  Completely removing this mechanism seems very heavy handed.  All
> that was needed to disable it was a single line change to set
> CHECK_STRUCTS to zero.
>
> You would have got further comment if you'd proposed a patch, rather
> than just a vague idea that might or might not have been followed
> through.  Discussing the merits of a patch is always best done before
> committing it to master, not afterwards.
>
> What does Daniel say?

Please revert this removal. Without CHECK_STRUCTS, we *will* break dumped
Emacs in some subtle way sooner or later.

The only other safe option is to just codegen all the lisp.h structure
definitions from some higher-level description from which we can also
generate pdumper dumping code. If you want to do that, be my guest. Until
then, please restore CHECK_STRUCTS.

>> We can reenable it later if needed (which I hope won't happen....).
>
> Funnily enough, it was of use to me recently when it reminded me to
> amend dump_subr after extending struct subr.
>
> My main problem with this mechanism was the vagueness of the error
> messages it generated and of the comment in the code they pointed at,
> which wasted lots of time.  Those defects have since been fixed.

Thanks for improving that.


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Paul Eggert
On 4/10/19 11:47 AM, Daniel Colascione wrote:
> Without CHECK_STRUCTS, we *will* break dumped
> Emacs in some subtle way sooner or later.

We're gonna break dumped Emacs in subtle ways no matter what. After all,
pdumper.c was broken in subtle ways when it was first introduced,
despite CHECK_STRUCTS. That's just life, and it's not the issue here.
The issue is whether the benefits of the CHECK_STRUCTS mechanism is
worth the hassle. In my experience it's definitely not worth it.

The more gingerbread we put into the Emacs build procedure, the more
it's off-putting to other potential developers (and the more work it is
for us). We already have too much build overhead (starting with
Autoconf! I hate Autoconf! It should go away!) and we should not neglect
its costs when looking at the benefits of a new hassle we add to the
build procedure. So far the benefits of CHECK_STRUCT have been zero in
the master, and the costs have been slowdown and significant annoyance
to the developer who's made the most changes to this part of the code.


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Daniel Colascione-5
> On 4/10/19 11:47 AM, Daniel Colascione wrote:
>> Without CHECK_STRUCTS, we *will* break dumped
>> Emacs in some subtle way sooner or later.
>
> We're gonna break dumped Emacs in subtle ways no matter what. After all,
> pdumper.c was broken in subtle ways when it was first introduced,
> despite CHECK_STRUCTS. That's just life, and it's not the issue here.
> The issue is whether the benefits of the CHECK_STRUCTS mechanism is
> worth the hassle. In my experience it's definitely not worth it.
>
> The more gingerbread we put into the Emacs build procedure, the more
> it's off-putting to other potential developers (and the more work it is
> for us). We already have too much build overhead (starting with
> Autoconf! I hate Autoconf! It should go away!) and we should not neglect
> its costs when looking at the benefits of a new hassle we add to the
> build procedure. So far the benefits of CHECK_STRUCT have been zero in
> the master, and the costs have been slowdown and significant annoyance
> to the developer who's made the most changes to this part of the code.

CHECK_STRUCTS was very useful when pdumper was out of tree and I had to
rebase it regularly. You can disable the thing locally for iterative
development.


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Eli Zaretskii
In reply to this post by Paul Eggert
> From: Paul Eggert <[hidden email]>
> Date: Wed, 10 Apr 2019 11:58:20 -0700
> Cc: [hidden email]
>
> So far the benefits of CHECK_STRUCT have been zero in the master

Not zero, at least not for me: it caught my mistake at least once.

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Andy Moreton-3
In reply to this post by Paul Eggert
On Wed 10 Apr 2019, Paul Eggert wrote:

> On 4/10/19 6:12 AM, Andy Moreton wrote:
>> this breaks out-of-tree builds, as make tries to use
>> /path/to/emacs/admin/charsets/Makefile rather than the correct path
>> <builddir>/admin/charsets/Makefile:
>
> Sorry, my tests didn't include a complete out-of-tree bootstrap. I
> installed the attached patch to fix that problem, along with the
> problem's other instance. The other -C changes should be OK. You're
> correct that some of the -C changes were unrelated to the original
> patch. Still, they make the Makefile more consistent and I'd like to
> keep the ones that work.

The minimal fixes you have committed work for me for a bootstrap build
from a completely clean tree (after 'git clean -Xdf').

I'm not convinced that computing filenames using GNU make functions
rather than using literal values is clearer, and the makefiles include
both styles, so more work is needed to achieve consistency using either
approach.

> PS. I'd like Emacs to switch to a nonrecursive 'make' process, as this
> subsidiary $(MAKE) -C business is for the birds. We've made the switch
> for other GNU projects and it's been a win. This is not a new idea; see:
>
> Miller P. Recursive Make considered harmful. AAUGN J of AUUG.
> 1998;19(1):14-25. https://grosskurth.ca/bib/1997/miller.pdf

This is a good idea, and should result in better dependency tracking and
possibly faster parallel builds. Please make sure it works for in tree
and out of tree builds, as both are useful.

    AndyM


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Daniel Colascione-5
> On Wed 10 Apr 2019, Paul Eggert wrote:
>
>> On 4/10/19 6:12 AM, Andy Moreton wrote:
>>> this breaks out-of-tree builds, as make tries to use
>>> /path/to/emacs/admin/charsets/Makefile rather than the correct path
>>> <builddir>/admin/charsets/Makefile:
>>
>> Sorry, my tests didn't include a complete out-of-tree bootstrap. I
>> installed the attached patch to fix that problem, along with the
>> problem's other instance. The other -C changes should be OK. You're
>> correct that some of the -C changes were unrelated to the original
>> patch. Still, they make the Makefile more consistent and I'd like to
>> keep the ones that work.
>
> The minimal fixes you have committed work for me for a bootstrap build
> from a completely clean tree (after 'git clean -Xdf').
>
> I'm not convinced that computing filenames using GNU make functions
> rather than using literal values is clearer, and the makefiles include
> both styles, so more work is needed to achieve consistency using either
> approach.
>
>> PS. I'd like Emacs to switch to a nonrecursive 'make' process, as this
>> subsidiary $(MAKE) -C business is for the birds. We've made the switch
>> for other GNU projects and it's been a win. This is not a new idea; see:
>>
>> Miller P. Recursive Make considered harmful. AAUGN J of AUUG.
>> 1998;19(1):14-25. https://grosskurth.ca/bib/1997/miller.pdf
>
> This is a good idea, and should result in better dependency tracking and
> possibly faster parallel builds. Please make sure it works for in tree
> and out of tree builds, as both are useful.

Agreed on moving to nonrecursive make.


Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Alan Mackenzie
In reply to this post by Paul Eggert
Hello, Paul.

On Wed, Apr 10, 2019 at 11:05:02 -0700, Paul Eggert wrote:
> On 4/10/19 9:22 AM, Alan Mackenzie wrote:
> > All that was needed to disable it was a single line change to set
> > CHECK_STRUCTS to zero.

> That would also have sufficed, but the code clutter (and slowdown in the
> build due to the computation of hashes which forces that part of the
> build to be sequential) was also annoying. If you really want it we can
> bring back the mechanism (with CHECK_STRUCTS off by default please!),
> though I'd rather not - see below.

According to Daniel (and my experience backs it up), the problems
CHECK_STRUCTS detects happen when merging master into a branch.  I'm not
sure you do such merges much.  But if you are making frequent changes to
the structures guarded by the mechanism, having deleted that mechanism,
you are saving effort for yourself, but imposing extra effort on other
people.

> > it was of use to me recently when it reminded me to
> > amend dump_subr after extending struct subr.
> I didn't observe that in master - presumably this was in some
> experimental branch? In master, dump_subr is currently the same as it
> was when the pdumper was installed.

Naturally.  I don't change basic structures in master without (usually
extensive) discussion on emacs-devel.  The changes were in branch
scratch/accurate-warning-pos.  The hashes were helpful in making these
changes.

> Before making the recent change, I reviewed all the changes made to
> pdumper.c in master, and observed none where the hashes helped and
> several where they hurt. Since the portable dumper was added I have made
> four commits that involved the hashes, and the hashes only got into my
> way and slowed me down.

The hashes do slow us down, yes.  But not really by very much, IMAO.
For example I timed make dmpstruct.h, and it only took 0.16s.  The extra
time taken in a build can't be more than a very small number of seconds.

Having a build aborted by CHECK_STRUCTS, and having manually to change
the hashes in pdumper.c is what takes the time.  But really, how often
are we changing those structures of an evening?

> You made one commit (9c0fa1172fd987a8f23b115145270383a11c12fc) that
> involved the buffer.h hash, and portions of that commit were
> mistakenly pushed in pdumper.c's previous commit so the hashes didn't
> seem to have helped there.

That was actually Eli's commit.

> The only other persons to make hash-related commits to pdumper.c were
> Alan and Stefan, and the hashes didn't help there either.

I think the hashes are a hindrance when developing on master, but a help
on branches (when merging from master).

> As the hashes get in the way of ordinary development (mostly affecting
> me) and don't seem to help in practice, I really want them to go.

I think I would prefer the hashes to stay, but I can see the other side
of the argument, too.

--
Alan Mackenzie (Nuremberg, Germany).

Reply | Threaded
Open this post in threaded view
|

Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

Daniel Colascione-5
> Hello, Paul.
>
> On Wed, Apr 10, 2019 at 11:05:02 -0700, Paul Eggert wrote:
>> On 4/10/19 9:22 AM, Alan Mackenzie wrote:
>> > All that was needed to disable it was a single line change to set
>> > CHECK_STRUCTS to zero.
>
>> That would also have sufficed, but the code clutter (and slowdown in the
>> build due to the computation of hashes which forces that part of the
>> build to be sequential) was also annoying. If you really want it we can
>> bring back the mechanism (with CHECK_STRUCTS off by default please!),
>> though I'd rather not - see below.
>
> According to Daniel (and my experience backs it up), the problems
> CHECK_STRUCTS detects happen when merging master into a branch.  I'm not
> sure you do such merges much.  But if you are making frequent changes to
> the structures guarded by the mechanism, having deleted that mechanism,
> you are saving effort for yourself, but imposing extra effort on other
> people.
>
>> > it was of use to me recently when it reminded me to
>> > amend dump_subr after extending struct subr.
>> I didn't observe that in master - presumably this was in some
>> experimental branch? In master, dump_subr is currently the same as it
>> was when the pdumper was installed.
>
> Naturally.  I don't change basic structures in master without (usually
> extensive) discussion on emacs-devel.  The changes were in branch
> scratch/accurate-warning-pos.  The hashes were helpful in making these
> changes.
>
>> Before making the recent change, I reviewed all the changes made to
>> pdumper.c in master, and observed none where the hashes helped and
>> several where they hurt. Since the portable dumper was added I have made
>> four commits that involved the hashes, and the hashes only got into my
>> way and slowed me down.
>
> The hashes do slow us down, yes.  But not really by very much, IMAO.
> For example I timed make dmpstruct.h, and it only took 0.16s.  The extra
> time taken in a build can't be more than a very small number of seconds.
>
> Having a build aborted by CHECK_STRUCTS, and having manually to change
> the hashes in pdumper.c is what takes the time.  But really, how often
> are we changing those structures of an evening?
>
>> You made one commit (9c0fa1172fd987a8f23b115145270383a11c12fc) that
>> involved the buffer.h hash, and portions of that commit were
>> mistakenly pushed in pdumper.c's previous commit so the hashes didn't
>> seem to have helped there.
>
> That was actually Eli's commit.
>
>> The only other persons to make hash-related commits to pdumper.c were
>> Alan and Stefan, and the hashes didn't help there either.
>
> I think the hashes are a hindrance when developing on master, but a help
> on branches (when merging from master).
>
>> As the hashes get in the way of ordinary development (mostly affecting
>> me) and don't seem to help in practice, I really want them to go.
>
> I think I would prefer the hashes to stay, but I can see the other side
> of the argument, too.

What would make the hashes easier to deal with? Some make target for
updating them automatically? An easier way to disable the check at
configure time? Transformation into a prominent runtime warning instead of
a build break?


1234