mh-e/mh-acros.el advices `require' incorrectly

mh-e/mh-acros.el advices `require' incorrectly

[Kenichi Handa]

This is in the latest CVS.

The origianl `require' returns FEATURE even if it is already
provided, but the adviced `require' returns nil in such a
case.  So, once mh-acros.el is loaded, a code depending on
the return value of `require' stops working.

Ex:
(require 'mh-e)
(require 'enriched) => enriched
(require 'enriched) => nil

How about this change?

*** mh-acros.el	13 Jan 2006 10:27:43 +0900	1.13
--- mh-acros.el	13 Jan 2006 16:45:04 +0900	
***************
*** 152,161 ****
  
  (defadvice require (around mh-prefer-el activate)
    "Modify `require' to load uncompiled MH-E files."
!   (or (featurep (ad-get-arg 0))
!       (and (string-match "^mh-" (symbol-name (ad-get-arg 0)))
!            (load (format "%s.el" (ad-get-arg 0)) t t))
!       ad-do-it))
  
  (defmacro mh-assoc-ignore-case (key alist)
    "Check if KEY is present in ALIST while ignoring case to do the comparison.
--- 152,162 ----
  
  (defadvice require (around mh-prefer-el activate)
    "Modify `require' to load uncompiled MH-E files."
!   (if (or (featurep (ad-get-arg 0))
! 	  (and (string-match "^mh-" (symbol-name (ad-get-arg 0)))
! 	       (load (format "%s.el" (ad-get-arg 0)) t t))
! 	  ad-do-it)
!       (setq ad-return-value (ad-get-arg 0))))
  
  (defmacro mh-assoc-ignore-case (key alist)
    "Check if KEY is present in ALIST while ignoring case to do the comparison.

---
Kenichi Handa
handa@m17n.org

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Kenichi Handa <handa@m17n.org> writes:

> This is in the latest CVS.
>
> The origianl `require' returns FEATURE even if it is already
> provided, but the adviced `require' returns nil in such a
> case.  So, once mh-acros.el is loaded, a code depending on
> the return value of `require' stops working.
>
> Ex:
> (require 'mh-e)
> (require 'enriched) => enriched
> (require 'enriched) => nil
>
> How about this change?
>
> *** mh-acros.el	13 Jan 2006 10:27:43 +0900	1.13
> --- mh-acros.el	13 Jan 2006 16:45:04 +0900	
> ***************
> *** 152,161 ****
>   
>   (defadvice require (around mh-prefer-el activate)
>     "Modify `require' to load uncompiled MH-E files."
> !   (or (featurep (ad-get-arg 0))
> !       (and (string-match "^mh-" (symbol-name (ad-get-arg 0)))
> !            (load (format "%s.el" (ad-get-arg 0)) t t))
> !       ad-do-it))
>   
>   (defmacro mh-assoc-ignore-case (key alist)
>     "Check if KEY is present in ALIST while ignoring case to do the comparison.
> --- 152,162 ----
>   
>   (defadvice require (around mh-prefer-el activate)
>     "Modify `require' to load uncompiled MH-E files."
> !   (if (or (featurep (ad-get-arg 0))
> ! 	  (and (string-match "^mh-" (symbol-name (ad-get-arg 0)))
> ! 	       (load (format "%s.el" (ad-get-arg 0)) t t))
> ! 	  ad-do-it)
> !       (setq ad-return-value (ad-get-arg 0))))
>   
>   (defmacro mh-assoc-ignore-case (key alist)
>     "Check if KEY is present in ALIST while ignoring case to do the comparison.

Satyaki,

What do you (and others) think about this?

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

RE: mh-e/mh-acros.el advices `require' incorrectly

[Drew Adams]

    > The origianl `require' returns FEATURE even if it is already
    > provided, but the adviced `require' returns nil in such a
    > case.  So, once mh-acros.el is loaded, a code depending on
    > the return value of `require' stops working.
    >
    > How about this change?

    What do you (and others) think about this?

Besides the fact that we shouldn't use defadvice in vanilla Emacs code, I
don't think we should change the return value of `require' at all. It has
acted this way for a very long time, and there is undoubtedly code that
depends on this behavior (at least some external libraries). Besides, we
certainly should not make such a change just before a release.



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: mh-e/mh-acros.el advices `require' incorrectly

[Stefan Monnier]

> *** mh-acros.el	13 Jan 2006 10:27:43 +0900	1.13
> --- mh-acros.el	13 Jan 2006 16:45:04 +0900	
> ***************
> *** 152,161 ****
  
>   (defadvice require (around mh-prefer-el activate)
>     "Modify `require' to load uncompiled MH-E files."
> !   (or (featurep (ad-get-arg 0))
> !       (and (string-match "^mh-" (symbol-name (ad-get-arg 0)))
> !            (load (format "%s.el" (ad-get-arg 0)) t t))
> !       ad-do-it))
  
For what it's worth:
We usually try to avoid defadvice within Emacs and I think for
good reasons.  Now, some elisp packages do you use defadvice, but they only
only do it when it's absolutely necessary.  The above advice seem to be just
a minor convenience hack to making recompiling occasionally easier.

I'd rather try and come up with a generic fix for the problem.
I personally use the patch below for now.


        Stefan


--- orig/lisp/emacs-lisp/bytecomp.el
+++ mod/lisp/emacs-lisp/bytecomp.el
@@ -1640,6 +1640,12 @@
   ;; Force logging of the file name for each file compiled.
   (setq byte-compile-last-logged-file nil)
   (let ((byte-compile-current-file filename)
+        ;; Prefer source files over compiled files.  This is so that when
+        ;; several files are changed and recompiled, each new file is
+        ;; properly recompiled with the new macros in the other new files.
+        (load-suffixes (sort (copy-sequence load-suffixes)
+                             (lambda (s1 s2) (and (string-match "\\.elc\\b" s2)
+                                             (string-match "\\.el\\b" s1)))))
 	(set-auto-coding-for-load t)
 	target-file input-buffer output-buffer
 	byte-compile-dest-file)

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Drew Adams <drew.adams@oracle.com> wrote:

>     > The origianl `require' returns FEATURE even if it is already
>     > provided, but the adviced `require' returns nil in such a
>     > case.  So, once mh-acros.el is loaded, a code depending on
>     > the return value of `require' stops working.
>     >
>     > How about this change?
> 
>     What do you (and others) think about this?
> 
> Besides the fact that we shouldn't use defadvice in vanilla Emacs code, I
> don't think we should change the return value of `require' at all. It has
> acted this way for a very long time, and there is undoubtedly code that
> depends on this behavior (at least some external libraries). Besides, we
> certainly should not make such a change just before a release.

Unless you load MH-E, require does not change. If I understand
correctly, Kanichi's patch addresses your concern (about the return
value of require changing). Here's the ChangeLog for that change:

    2004-08-23  Satyaki Das  <satyaki@theforce.stanford.edu>

	* mh-acros.el (mh-require-cl): Remove unneeded autoloads.
	(require): Add an advice to the function so that at compile time
	the uncompiled file is loaded. This avoids compilation problems
	when built in the Emacs tree.

Satyaki, just out of curiosity, do you remember what the problems were?
Would we be able to remove the require defadvice if we removed the
circular dependencies?

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: mh-e/mh-acros.el advices `require' incorrectly

[Satyaki Das]

On 1/13/06, Bill Wohler <wohler@newt.com> wrote:
> Drew Adams <drew.adams@oracle.com> wrote:
>
> >     > The origianl `require' returns FEATURE even if it is already
> >     > provided, but the adviced `require' returns nil in such a
> >     > case.  So, once mh-acros.el is loaded, a code depending on
> >     > the return value of `require' stops working.
> >     >
> >     > How about this change?
> >
> >     What do you (and others) think about this?
> >
> > Besides the fact that we shouldn't use defadvice in vanilla Emacs code, I
> > don't think we should change the return value of `require' at all. It has
> > acted this way for a very long time, and there is undoubtedly code that
> > depends on this behavior (at least some external libraries). Besides, we
> > certainly should not make such a change just before a release.
>
> Unless you load MH-E, require does not change.

Even if you load MH-E, require isn't adviced since mh-acros.el is
never loaded at run  time unless you load it yourself.

>                                                                         If I understand
> correctly, Kanichi's patch addresses your concern (about the return
> value of require changing). Here's the ChangeLog for that change:

Kenichi's patch won't cause any problems for MH-E.

>
>     2004-08-23  Satyaki Das  <satyaki@theforce.stanford.edu>
>
>         * mh-acros.el (mh-require-cl): Remove unneeded autoloads.
>         (require): Add an advice to the function so that at compile time
>         the uncompiled file is loaded. This avoids compilation problems
>         when built in the Emacs tree.
>
> Satyaki, just out of curiosity, do you remember what the problems were?
> Would we be able to remove the require defadvice if we removed the
> circular dependencies?

That advice forces the load of src .el file.  Suppose you have three
files a.el, b.el and c.el and the compiled files a.elc, b.elc and
c.elc.  Suppose that there is a macro foo in c.el that is used in
a.el.  Now let us make a change to macro foo.  Then if we recompile
all the files in the directory then they are compiled in the lexical
order.  So when a.el is getting compiled, require will load the c.elc
and get the old definition of the macro.  The advice above avoids
this.

A better solution would be to use Stefan's change in bytecomp.el.  If
it were checked in, we can drop the advice to require and it will make
things nicer elsewhere too.

Satyaki


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_idv37&alloc_id\x16865&op=click

Re: mh-e/mh-acros.el advices `require' incorrectly

[Kenichi Handa]

In article <837.1137190309@olgas.newt.com>, Bill Wohler <wohler@newt.com> writes:

>>  Besides the fact that we shouldn't use defadvice in vanilla Emacs code, I
>>  don't think we should change the return value of `require' at all. It has
>>  acted this way for a very long time, and there is undoubtedly code that
>>  depends on this behavior (at least some external libraries). Besides, we
>>  certainly should not make such a change just before a release.

> Unless you load MH-E, require does not change. 

Unfortunately, even if we don't load MH-E explicitly, now,
just compiling some file in emacs/lisp/mh-e/*.el change the
behaviour of `require'.

> If I understand correctly, Kanichi's patch addresses your
> concern (about the return value of require changing).

Yes.

---
Kenichi Handa
handa@m17n.org

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Satyaki Das <satyakid@gmail.com> wrote:

> On 1/13/06, Bill Wohler <wohler@newt.com> wrote:
> > Drew Adams <drew.adams@oracle.com> wrote:
> >
> > >     > The origianl `require' returns FEATURE even if it is already
> > >     > provided, but the adviced `require' returns nil in such a
> > >     > case.  So, once mh-acros.el is loaded, a code depending on
> > >     > the return value of `require' stops working.
> > >     >
> > >     > How about this change?
> > >
> > >     What do you (and others) think about this?
> > >
> > > Besides the fact that we shouldn't use defadvice in vanilla Emacs code, I
> > > don't think we should change the return value of `require' at all. It has
> > > acted this way for a very long time, and there is undoubtedly code that
> > > depends on this behavior (at least some external libraries). Besides, we
> > > certainly should not make such a change just before a release.
> >
> > Unless you load MH-E, require does not change.
> 
> Even if you load MH-E, require isn't adviced since mh-acros.el is
> never loaded at run  time unless you load it yourself.
> 
> >                                                                         If I understand
> > correctly, Kanichi's patch addresses your concern (about the return
> > value of require changing). Here's the ChangeLog for that change:
> 
> Kenichi's patch won't cause any problems for MH-E.

This means you can check in your patch, Kenichi ;-).

Thanks very much for the explanation, Satyaki.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> *** mh-acros.el	13 Jan 2006 10:27:43 +0900	1.13
>> --- mh-acros.el	13 Jan 2006 16:45:04 +0900	
>> ***************
>> *** 152,161 ****
>   
>>   (defadvice require (around mh-prefer-el activate)
>>     "Modify `require' to load uncompiled MH-E files."
>> !   (or (featurep (ad-get-arg 0))
>> !       (and (string-match "^mh-" (symbol-name (ad-get-arg 0)))
>> !            (load (format "%s.el" (ad-get-arg 0)) t t))
>> !       ad-do-it))
>   
> For what it's worth:
> We usually try to avoid defadvice within Emacs and I think for
> good reasons.  Now, some elisp packages do you use defadvice, but they only
> only do it when it's absolutely necessary.  The above advice seem to be just
> a minor convenience hack to making recompiling occasionally easier.
>
> I'd rather try and come up with a generic fix for the problem.
> I personally use the patch below for now.
>
>
>         Stefan
>
>
> --- orig/lisp/emacs-lisp/bytecomp.el
> +++ mod/lisp/emacs-lisp/bytecomp.el
> @@ -1640,6 +1640,12 @@
>    ;; Force logging of the file name for each file compiled.
>    (setq byte-compile-last-logged-file nil)
>    (let ((byte-compile-current-file filename)
> +        ;; Prefer source files over compiled files.  This is so that when
> +        ;; several files are changed and recompiled, each new file is
> +        ;; properly recompiled with the new macros in the other new files.
> +        (load-suffixes (sort (copy-sequence load-suffixes)
> +                             (lambda (s1 s2) (and (string-match "\\.elc\\b" s2)
> +                                             (string-match "\\.el\\b" s1)))))
>  	(set-auto-coding-for-load t)
>  	target-file input-buffer output-buffer
>  	byte-compile-dest-file)

Is there a problem with your committing this patch? Then we can get
rid of the defadvice and make everyone happy ;-).

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: mh-e/mh-acros.el advices `require' incorrectly

[Richard M. Stallman]

    The origianl `require' returns FEATURE even if it is already
    provided, but the adviced `require' returns nil in such a
    case.

Packages in Emacs are not supposed to advise other parts of Emacs.
So really the right thing to do is get rid of this advising.
It is not clean for `require' to work differently in a certain
part of Emacs.

Bill, what was the reason for this?

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Richard M. Stallman <rms@gnu.org> wrote:

>     The origianl `require' returns FEATURE even if it is already
>     provided, but the adviced `require' returns nil in such a
>     case.
> 
> Packages in Emacs are not supposed to advise other parts of Emacs.
> So really the right thing to do is get rid of this advising.
> It is not clean for `require' to work differently in a certain
> part of Emacs.
> 
> Bill, what was the reason for this?

I trust you've seen Satyaki's and Stefan's messages by now. I'm happy to
remove the defadvice as soon as the problem it works around has been
solved. Perhaps we can install Stefan's patch or one like it.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: mh-e/mh-acros.el advices `require' incorrectly

[Richard M. Stallman]

    --- orig/lisp/emacs-lisp/bytecomp.el
    +++ mod/lisp/emacs-lisp/bytecomp.el
    @@ -1640,6 +1640,12 @@
       ;; Force logging of the file name for each file compiled.
       (setq byte-compile-last-logged-file nil)
       (let ((byte-compile-current-file filename)
    +        ;; Prefer source files over compiled files.  This is so that when
    +        ;; several files are changed and recompiled, each new file is
    +        ;; properly recompiled with the new macros in the other new files.
    +        (load-suffixes (sort (copy-sequence load-suffixes)
    +                             (lambda (s1 s2) (and (string-match "\\.elc\\b" s2)
    +                                             (string-match "\\.el\\b" s1)))))
	    (set-auto-coding-for-load t)
	    target-file input-buffer output-buffer
	    byte-compile-dest-file)

That creates an inconsistency.  If it were a reliable solution to the
problem, that could justify the inconsistency.  But it isn't reliable,
because if the file require'd was already loaded in bytecode outside
the compiler, this won't reload it.

Thus, if we want a reliable solution, it has to be something else.

For the same reason, the advice in mh-e.el is not a good solution
(even disregarding that it is a bad thing for parts of Emacs to
advise other parts).

Re: mh-e/mh-acros.el advices `require' incorrectly

[Richard M. Stallman]

    Unless you load MH-E, require does not change. If I understand
    correctly, Kanichi's patch addresses your concern (about the return
    value of require changing).

MH-E should not advise `require'; nothing should.
Another solution is necessary.

Which files are trying to force loading in source form?

Re: mh-e/mh-acros.el advices `require' incorrectly

[Stefan Monnier]

>     --- orig/lisp/emacs-lisp/bytecomp.el
>     +++ mod/lisp/emacs-lisp/bytecomp.el
>     @@ -1640,6 +1640,12 @@
>        ;; Force logging of the file name for each file compiled.
>        (setq byte-compile-last-logged-file nil)
>        (let ((byte-compile-current-file filename)
>     +        ;; Prefer source files over compiled files.  This is so that when
>     +        ;; several files are changed and recompiled, each new file is
>     +        ;; properly recompiled with the new macros in the other new files.
>     +        (load-suffixes (sort (copy-sequence load-suffixes)
>     +                             (lambda (s1 s2) (and (string-match "\\.elc\\b" s2)
>     +                                             (string-match "\\.el\\b" s1)))))
> 	    (set-auto-coding-for-load t)
> 	    target-file input-buffer output-buffer
> 	    byte-compile-dest-file)

> That creates an inconsistency.  If it were a reliable solution to the
> problem, that could justify the inconsistency.  But it isn't reliable,
> because if the file require'd was already loaded in bytecode outside
> the compiler, this won't reload it.

Indeed, it's just a minor convenience hack.

> For the same reason, the advice in mh-e.el is not a good solution
> (even disregarding that it is a bad thing for parts of Emacs to
> advise other parts).

100% agreement.


        Stefan

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

"Richard M. Stallman" <rms@gnu.org> writes:

> MH-E should not advise `require'; nothing should.
> Another solution is necessary.

We only had the problem our workaround addresses when compiling in the
Emacs tree. We do not have any problems when compiling in our own
environment because we remove all MH-E .elc files before compiling.

What do people think about my updating lisp/Makefile.in to remove
lisp/mh-e/*.elc if and only if lisp/mh-e/*.el changes?

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: mh-e/mh-acros.el advices `require' incorrectly

[Andreas Schwab]

Bill Wohler <wohler@newt.com> writes:

> "Richard M. Stallman" <rms@gnu.org> writes:
>
>> MH-E should not advise `require'; nothing should.
>> Another solution is necessary.
>
> We only had the problem our workaround addresses when compiling in the
> Emacs tree. We do not have any problems when compiling in our own
> environment because we remove all MH-E .elc files before compiling.

That's what `make bootstrap' does.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Andreas Schwab <schwab@suse.de> wrote:

> Bill Wohler <wohler@newt.com> writes:
> 
> > "Richard M. Stallman" <rms@gnu.org> writes:
> >
> >> MH-E should not advise `require'; nothing should.
> >> Another solution is necessary.
> >
> > We only had the problem our workaround addresses when compiling in the
> > Emacs tree. We do not have any problems when compiling in our own
> > environment because we remove all MH-E .elc files before compiling.
> 
> That's what `make bootstrap' does.

You can't expect everyone to run make bootstrap when they change one
lisp file.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: mh-e/mh-acros.el advices `require' incorrectly

[Stefan Monnier]

> You can't expect everyone to run make bootstrap when they change one
> lisp file.

Yet, that's how we've worked for many years now.
In practice the problems you describe are infrequent and managed by hand or
by `make bootstrap'.


        Stefan


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: mh-e/mh-acros.el advices `require' incorrectly

[Richard M. Stallman]

    What do people think about my updating lisp/Makefile.in to remove
    lisp/mh-e/*.elc if and only if lisp/mh-e/*.el changes?

That won't help with M-x byte-recompile.  The right solution is to
make the graph of requires acyclic, and put the macro file at the
start of the alphabet.  Given the way the software in fact works,
that should be totally reliable.

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Richard M. Stallman <rms@gnu.org> wrote:

>     What do people think about my updating lisp/Makefile.in to remove
>     lisp/mh-e/*.elc if and only if lisp/mh-e/*.el changes?
> 
> That won't help with M-x byte-recompile.  The right solution is to
> make the graph of requires acyclic, and put the macro file at the
> start of the alphabet.  Given the way the software in fact works,
> that should be totally reliable.

Thank you. I think I'm going to go ahead and bite the bullet and do just
that. I've appended a proposal I sent to the MH-E developers today.
While I don't expect you to understand MH-E, perhaps you can let me know
whether your gut says "good idea" or "bad idea" and why.

(mh-customize.el holds all of our defcustom and defface calls.
mh-loaddefs.el is the internal MH-E analog of loaddefs.el.)

1. Remove all (require 'mh-*) lines, move the provides in
   mh-customize.el and mh-e.el to the end and start from scratch ;-).

2. Make mh-utils.el a pure "utility" package. I'm thinking chunks could
   go into mh-xface.el and mh-scan.el (taking stuff from mh-e.el too).
   We would then create files for each of the modes. We now already have
   mh-search.el, so I'd propose mh-show.el and mh-folder.el, maybe
   renaming mh-comp.el to mh-letter.el for consistency. Move
   mh-modify to mh-funcs.el where it belongs.

3. Make mh-e.el a lean, mean entry point. Make it require mh-acros,
   mh-customize, *and nothing else* within MH-E. Have it define pretty
   much only mh-rmail, mh-smail, and mh-version (functions with the
   ;;;###autoload cookie) as well as a few other generic globals such as
   mh-xemacs-flag. The frequently-used commands would go into
   mh-folder.el and the rest would go into mh-funcs.el.

4. Make mh-customize require mh-loaddefs to get the function definitions
   it needs *and nothing else* within MH-E.

5. Everything else then requires mh-e.el (implicitly getting
   mh-customize and mh-loaddefs) *and nothing else* within MH-E (except
   for perhaps variable-only files like mh-buffers.el and mh-scan.el).

6. Limit scope of variables to just the file in which it is defined.
   If necessary, provide access to variables via functions that are
   found in mh-loaddefs.el.

7. Move all macros into mh-acros.el so that we can remove the defadvice
   of require in mh-acros.el.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: mh-e/mh-acros.el advices `require' incorrectly

[Richard M. Stallman]

I don't understand 100% of the plan, but it looks like a good plan
overall.

    2. Make mh-utils.el a pure "utility" package. I'm thinking chunks could
       go into mh-xface.el and mh-scan.el (taking stuff from mh-e.el too).
       We would then create files for each of the modes. We now already have
       mh-search.el, so I'd propose mh-show.el and mh-folder.el, maybe
       renaming mh-comp.el to mh-letter.el for consistency. Move
       mh-modify to mh-funcs.el where it belongs.

    6. Limit scope of variables to just the file in which it is defined.
       If necessary, provide access to variables via functions that are
       found in mh-loaddefs.el.

I think these two are be necessary to solve the present problem, and
could substantially increase the amount of change.  It might be better
not to do these changes now.

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

Richard M. Stallman <rms@gnu.org> wrote:

> I don't understand 100% of the plan, but it looks like a good plan
> overall.

Thanks for the feedback.

>     2. Make mh-utils.el a pure "utility" package. I'm thinking chunks could
>        go into mh-xface.el and mh-scan.el (taking stuff from mh-e.el too).
>        We would then create files for each of the modes. We now already have
>        mh-search.el, so I'd propose mh-show.el and mh-folder.el, maybe
>        renaming mh-comp.el to mh-letter.el for consistency. Move
>        mh-modify to mh-funcs.el where it belongs.
> 
>     6. Limit scope of variables to just the file in which it is defined.
>        If necessary, provide access to variables via functions that are
>        found in mh-loaddefs.el.
> 
> I think these two are be necessary to solve the present problem, and
> could substantially increase the amount of change.  It might be better
> not to do these changes now.

Maybe. I'm investigating it right now since it seems that every day we
uncover a new problem associated with the current mess. It's extremely
brittle. The changes will either work or they won't. If they do, I think
it would actually add to the stability of the release to check in the
changes. I'll let you know how it goes.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: mh-e/mh-acros.el advices `require' incorrectly

[David Kastrup]

Bill Wohler <wohler@newt.com> writes:

> Maybe. I'm investigating it right now since it seems that every day
> we uncover a new problem associated with the current mess. It's
> extremely brittle. The changes will either work or they won't. If
> they do, I think it would actually add to the stability of the
> release to check in the changes. I'll let you know how it goes.

You are aware that this sounds like smoldering duct tape all over?
I am not at all acquainted with the involved code, but if you feel it
is best described with those words...

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: mh-e/mh-acros.el advices `require' incorrectly

[Bill Wohler]

David Kastrup <dak@gnu.org> wrote:

> Bill Wohler <wohler@newt.com> writes:
> 
> > Maybe. I'm investigating it right now since it seems that every day
> > we uncover a new problem associated with the current mess. It's
> > extremely brittle. The changes will either work or they won't. If
> > they do, I think it would actually add to the stability of the
> > release to check in the changes. I'll let you know how it goes.
> 
> You are aware that this sounds like smoldering duct tape all over?
> I am not at all acquainted with the involved code, but if you feel it
> is best described with those words...

The code has roots from the early 80's so it's long past the smoldering
point ;-). But the bones are strong; it just needs a renovation ;-).

I usually describe it in these words: http://mh-e.sourceforge.net/

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

MH-E reorg (was: mh-e/mh-acros.el advices `require' incorrectly)

[Bill Wohler]

Bill Wohler <wohler@olgas.newt.com> wrote:

> Richard M. Stallman <rms@gnu.org> wrote:
> 
> > I don't understand 100% of the plan, but it looks like a good plan
> > overall.
> 
> Thanks for the feedback.
> 
> >     2. Make mh-utils.el a pure "utility" package. I'm thinking chunks could
> >        go into mh-xface.el and mh-scan.el (taking stuff from mh-e.el too).
> >        We would then create files for each of the modes. We now already have
> >        mh-search.el, so I'd propose mh-show.el and mh-folder.el, maybe
> >        renaming mh-comp.el to mh-letter.el for consistency. Move
> >        mh-modify to mh-funcs.el where it belongs.
> > 
> >     6. Limit scope of variables to just the file in which it is defined.
> >        If necessary, provide access to variables via functions that are
> >        found in mh-loaddefs.el.
> > 
> > I think these two are be necessary to solve the present problem, and
> > could substantially increase the amount of change.  It might be better
> > not to do these changes now.
> 
> Maybe. I'm investigating it right now since it seems that every day we
> uncover a new problem associated with the current mess. It's extremely
> brittle. The changes will either work or they won't. If they do, I think
> it would actually add to the stability of the release to check in the
> changes. I'll let you know how it goes.

Hi Richard,

I finished the reorganization of MH-E over a week ago, and I'm extremely
pleased with the results. The developers and a few users have been smoke
testing a tarball of my work directory and there are no known issues
(the only issue revealed by the smoke test was a small Emacs 21 issue
which was quickly fixed).

I'm sorry this didn't come up sooner in the release cycle, but I'll go
ahead and check in the changes since I'm definitely going to use them
for the MH-E 8.0 release, the MH-E package isn't a critical part of
Emacs, and I think the Emacs 22 release will be more stable with few, if
any, risks.

Highlights:

1. Circular dependencies are gone.

2. defadvice of require is gone; shared macros have been moved to
   mh-acros.el. This means that you'll never get stale macro definitions
   making the defadvice unnecessary.

3. I was able to delete this code *within*
   mh-gnus-article-highlight-citation:

  ;; Requiring gnus-cite should have been sufficient. However for Emacs21.1,
  ;; recursive-load-depth-limit is only 10, so an error occurs. Also it may be
  ;; better to have an autoload at top-level (though that won't work because
  ;; of recursive-load-depth-limit). That gets rid of a compiler warning as
  ;; well.
  (unless mh-xemacs-flag
    (require 'gnus-art)
    (require 'gnus-cite))

4. I was able to delete this code in mh-utils.el:

 (eval-and-compile
   (defvar recursive-load-depth-limit)
   (if (and (boundp 'recursive-load-depth-limit)
            (integerp recursive-load-depth-limit)
            (< recursive-load-depth-limit 50))
       (setq recursive-load-depth-limit 50)))

5. I was able to delete eval-and-compile tricks as in the following
   around mh-show-font-lock-keywords:

    (eval-and-compile
      ;; Otherwise byte-compilation fails on
      ;; `mh-show-font-lock-keywords-with-cite'

6. XEmacs now compiles with just three warnings (from XEmacs macros
   which I can't do anything about). It used to compile with *hundreds*
   of warnings and a handful of errors too. (Note that MH-E has always
   compiled clean in GNU Emacs--it is a developer requirement.)

While the diffs will be large, they mostly represent functions moving
from one file to another. The functional groupings are now much more
cohesive and consistent and will be easier to maintain.

Developers should be aware that three files will be deleted so it might
be a good idea to remove lisp/mh-e/*.elc. The entry points are in
different files too so users of CVS MH-E will have to run "cd lisp; make
autoloads". You can accomplish both, of course, with "make bootstrap".
Nine new files were added.


If you're curious, here is the original plan with commentary:

> 1. Remove all (require 'mh-*) lines, move the provides in
>    mh-customize.el and mh-e.el to the end and start from scratch ;-).

Done.

> 2. Make mh-utils.el a pure "utility" package. I'm thinking chunks could
>    go into mh-xface.el and mh-scan.el (taking stuff from mh-e.el too).
>    We would then create files for each of the modes. We now already have
>    mh-search.el, so I'd propose mh-show.el and mh-folder.el, maybe
>    renaming mh-comp.el to mh-letter.el for consistency. Move
>    mh-modify to mh-funcs.el where it belongs.

Done. While I moved mh-letter-mode into mh-letter.el, It still made
sense to keep mh-comp.el. I also pulled the limiting code out of
mh-seq.el into mh-limit.el and the threading code out of mh-seq.el into
mh-thread.el. In addition, I created mh-compat.el (for compatibility
defsubsts) and mh-tool-bar.el for the tool bar creation code.

> 3. Make mh-e.el a lean, mean entry point. Make it require mh-acros,
>    mh-customize, *and nothing else* within MH-E. Have it define pretty
>    much only mh-rmail, mh-smail, and mh-version (functions with the
>    ;;;###autoload cookie) as well as a few other generic globals such as
>    mh-xemacs-flag. The frequently-used commands would go into
>    mh-folder.el and the rest would go into mh-funcs.el.

I made mh-e.el lean and mean, but not exactly as I described. I moved
nearly all of the code into other more logical files, pulled in the
defcustoms from mh-customize.el, and then just enough code to get it to
compile, including mh-init.el and mh-exec.el (removing those three files
as a result). The only entry point left in mh-e.el is mh-version. I
moved the other entry points into their corresponding files (for
example, mh-smail into mh-comp.el and mh-rmail into mh-folder.el). The
result eliminates dependencies in mh-e.el, and reduces the amount of
code that needs to be loaded when running, say, just mh-smail.

> 4. Make mh-customize require mh-loaddefs to get the function definitions
>    it needs *and nothing else* within MH-E.

The content of mh-customize.el was incorporated into mh-e.el and
the file was removed.

> 5. Everything else then requires mh-e.el (implicitly getting
>    mh-customize and mh-loaddefs) *and nothing else* within MH-E (except
>    for perhaps variable-only files like mh-buffers.el and mh-scan.el).

Done.

> 6. Limit scope of variables to just the file in which it is defined.
>    If necessary, provide access to variables via functions that are
>    found in mh-loaddefs.el.

I did this in a couple of places where it was very clearly the right
thing to do, but left the vast majority of globals there were previously
spread across mh-e.el and mh-utils.el in mh-e.el.

> 7. Move all macros into mh-acros.el so that we can remove the defadvice
>    of require in mh-acros.el.

Done, except that I didn't move all macros, just those that were used
in more than one file.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: MH-E reorg

[Bill Wohler]

Bill Wohler <wohler@olgas.newt.com> wrote:

> Developers should be aware that three files will be deleted so it might
> be a good idea to remove lisp/mh-e/*.elc. The entry points are in
> different files too so users of CVS MH-E will have to run "cd lisp; make
> autoloads". You can accomplish both, of course, with "make bootstrap".
> Nine new files were added.

I forgot to point out that because lisp/Makefile.in changed, you first
have to run ./configure.

Apologies for the inconvenience.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.

Re: MH-E reorg (was: mh-e/mh-acros.el advices `require' incorrectly)

[Richard M. Stallman]

Thanks for making this clean.  Certainly please install it.

Re: MH-E reorg (was: mh-e/mh-acros.el advices `require' incorrectly)

[Bill Wohler]

Richard M. Stallman <rms@gnu.org> wrote:

> Thanks for making this clean.  Certainly please install it.

Thank you. Done.

-- 
Bill Wohler <wohler@newt.com>  http://www.newt.com/wohler/  GnuPG ID:610BD9AD
Maintainer of comp.mail.mh FAQ and MH-E. Vote Libertarian!
If you're passed on the right, you're in the wrong lane.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642