Page MenuHomeFreeBSD

Emacs ports: no-op to conform to var order and improve consistency
ClosedPublic

Authored by jrm on Wed, Sep 4, 7:35 PM.

Details

Summary

PR:
Submitted by:
Reported by:

Test Plan

poudriere testport looks good

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jrm created this revision.Wed, Sep 4, 7:35 PM
jrm updated this revision to Diff 61657.Wed, Sep 4, 7:38 PM

Also remove EMACS_VER since it's only used once

ashish added a comment.Thu, Sep 5, 9:52 PM

Hi,

In D21524#468847, @jrm wrote:

Also remove EMACS_VER since it's only used once

I think you meant EMACS_REV here ?

editors/emacs-devel/Makefile
200 ↗(On Diff #61657)

Also, as I see that you have replaced all occurences EMACS_VER with DISTVERSION:R, which IMHO looks ugly. If you'd rather see Emacs version at the top of the file (easy to locate which sounds fair to me), then maybe initialize EMACS_VER as ${DISTVERSION:R}, while keeping everything that includes EMACS_VER as is ?

ashish requested changes to this revision.Thu, Sep 5, 9:53 PM
This revision now requires changes to proceed.Thu, Sep 5, 9:53 PM
jrm added a comment.Thu, Sep 5, 10:25 PM

Hi,

In D21524#468847, @jrm wrote:

Also remove EMACS_VER since it's only used once

I think you meant EMACS_REV here ?

Yup.

As an aside, the garbage collection bug was not completely fixed and Paul E. submitted another patch. See messages #72 and later at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37006

editors/emacs-devel/Makefile
200 ↗(On Diff #61657)

My motivation was to reduce the diff between editors/emacs and editors/emacs-devel. In editors/emacs DISTVERSION is used because it would be the same as EMACS_VER. In editors/emacs-devel DISTVERSION is the same as EMACS_VER, except without the revision date, which is what ${DISTVERSION:R} sort of means. Even with these changes, a diff would still flag these lines as different, just less different, plus there would be one less line in the diff for defining EMACS_VER.

Putting my silly obsessions aside, it's not so important to me either way, so if this motivation does not seem useful, I will follow your suggestion.

ashish added a comment.Fri, Sep 6, 3:39 PM
In D21524#469200, @jrm wrote:

Hi,

In D21524#468847, @jrm wrote:

Also remove EMACS_VER since it's only used once

I think you meant EMACS_REV here ?

Yup.
As an aside, the garbage collection bug was not completely fixed and Paul E. submitted another patch. See messages #72 and later at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37006

Thanks for working on it.

editors/emacs-devel/Makefile
200 ↗(On Diff #61657)

I haven't looked at these changes from the that perspective, and do not think it's a silly obsession. I think from maintenance point of view, your initiative is a good one, less chances of human errors.

I made this diff implying what I think should happen, w.r.t. EMACS_VER. And also created a diff to illustrate differences between emacs, and emacs-devel ports.

Let me know what you think.

ashish added inline comments.Fri, Sep 6, 3:44 PM
editors/emacs-devel/Makefile
200 ↗(On Diff #61657)

Sorry, ignore the diffs in previous comment, right diffs are below:

jrm added a comment.EditedFri, Sep 6, 8:41 PM

This is a reasonable approach, however I don't see the value in defining EMACS_REV since we are essentially doing VAR1=some_value then the only reference to VAR1 is VAR2=VAR1. I sort of feel the same about EMACS_VER, since it holds the same value as DISTVERSION in editors/emacs and almost the same value in editors/emacs-devel. It seems to add an extra layer (What's the value of EMACS_VER? It's DISTVERSION. What's the value of DISTVERSION? It's 26.3).

Having said this, I don't think it makes that much difference either way, and I yield to your seniority as Emacs maintainer here. :)

EDIT: Fix spelling of EMACS_REV.

ashish accepted this revision.Sat, Sep 7, 2:07 AM

As discussed on Slack, your changes are okay.

Thanks!

This revision is now accepted and ready to land.Sat, Sep 7, 2:07 AM
This revision was automatically updated to reflect the committed changes.