Re: [PATCH v2] Add compat.el support to Org (was: [POLL] Use compat.el in Org? (was: Useful package? Compat.el))

[Ihor Radchenko <yantar92@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 4757 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

>> +;; Package-Requires: ((emacs "26.1") (compat "29.1.4.1"))
>
> Is there a way to express (or (compat "29.1.4.1") (emacs "28.1")) to 
> avoid installing compat in the case of sufficiently new emacs? E.g. 
> dpkg/apt allows such alternatives.

No, AFAIK:

D.8 Conventional Headers for Emacs Libraries

‘Package-Requires’
     If this exists, it names packages on which the current package
     depends for proper operation.  *Note Packaging Basics::.  This is
     used by the package manager both at download time (to ensure that a
     complete set of packages is downloaded) and at activation time (to
     ensure that a package is only activated if all its dependencies
     have been).

     Its format is a list of lists on a single line.  The ‘car’ of each
     sub-list is the name of a package, as a symbol.  The ‘cadr’ of each
     sub-list is the minimum acceptable version number, as a string that
     can be parsed by ‘version-to-list’.  An entry that lacks a version
     (i.e., an entry which is just a symbol, or a sub-list of one
     element) is equivalent to entry with version "0".  For instance:

          ;; Package-Requires: ((gnus "1.0") (bubbles "2.7.2") cl-lib (seq))

     The package code automatically defines a package named ‘emacs’ with
     the version number of the currently running Emacs.  This can be
     used to require a minimal version of Emacs for a package.

> Early I asked concerning compat-29.1.3. I would prefer to install 
> elpa-compat system package to avoid network activity in response to 
> make. Required version matters for those who builds packages for 
> backport repositories for various linux distributions. It allows to 
> decide if it is necessary to provide newer elpa-compat or it is enough 
> to package elpa-org.

We can put older version require now, indeed. However, later we should
not constrain ourselves about bumping compat version as necessary.

Considering Debian lifecycle, it is pretty much guaranteed that we will
require the compat version that is not installed by Debian, eventually.

In any case, see the attached additional patch.

>> compat.el is required for "compile" target. Compilation will simply fail
>> with older Emacs. And "test" triggers "compile".
>
> There are different types of build systems. Some of them allows to 
> specify which dependencies should be pulled, some do not. My expectation 
> is that make does not attempt to manage dependencies. For me it is OK to 
> type an additional command to install them and to fail otherwise.

Sure. And you will have such option (EFLAGS).
However, I decided to enable auto-downloading by default to not break
the previous working compilation instructions.

It may, however, be worth documenting EFLAGS in WORG. See the attached
patch.

> In my opinion
>
>> +	@$(FIND) $(pkgdir) \(  -name \*.elc \) -exec $(RM) {} +
>
> command tells that package management capabilities in Emacs are rather 
> rudimentary (in comparison to e.g. python toolchain). That is why I am 
> in favor to more manual dependency management. Notice that I am not 
> against an option to do it from make.

May you elaborate what you mean by "more manual"? What concrete change
in the patch do you have in mind?

> Untested:
>
> $(FIND) $(pkgdir) -name \*.elc -delete
>
> "@" for silencing is intentionally dropped, parenthesis are unnecessary 
> for single condition.

Looks fine, except that now we have part of the targets using $RM
variable and part of the targets ignoring it. I am not sure if it is a
problem. I am slightly in favour of keeping the existing approach with
$RM.

>>> default.mk is included from top level Makefile only.
>> 
>> Not sure here. I just noticed that GITVERSION got re-calculated many
>> times when looking at debug output of make. (It was slowing things down
>> significantly). GITVERSION is in targets.mk though.
>
> GITVERSION is defined as ?=, so it is a variable with deferred 
> evaluation. Immediately evaluated variable may defined using :=

That's what I did in one of the patches (use :=).

>>> pkgdir = $(top_builddir)/pkg-deps
>> 
>> How to define top_builddir? If also via `pwd`, I see not much difference.
>
> I consider it as self-documenting code. Intermediate variable makes it 
> apparent for readers that the directory is relative to the top of the 
> package file tree.

Agree. Will do.
See the attached.

> Since out of source tree builds are not supported, I would consider 
> adding emacs version to path. The idea is to allow keeping installed 
> packages when switching between several emacs versions.

Done.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-mk-default.mk-pkgdir-Make-it-more-clear-that-we-inde.patch --]
[-- Type: text/x-patch, Size: 904 bytes --]

From c5ba5773e69d16930fc12db6fbe0d907fae926cd Mon Sep 17 00:00:00 2001
Message-Id: <c5ba5773e69d16930fc12db6fbe0d907fae926cd.1681389559.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Sat, 8 Apr 2023 19:05:37 +0200
Subject: [PATCH 1/4] mk/default.mk (pkgdir): Make it more clear that we indent
 to use top git dir

* mk/default.mk (top_builddir): New variable storing top-level directory.
(pkgdir): Use the new variable.
---
 mk/default.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mk/default.mk b/mk/default.mk
index 67bf96c2e..92a3942da 100644
--- a/mk/default.mk
+++ b/mk/default.mk
@@ -32,7 +32,8 @@ TMPDIR ?= /tmp
 testdir = $(TMPDIR)/tmp-orgtest
 
 # Where to store Org dependencies
-pkgdir := $(shell pwd)/pkg-deps
+top_builddir := $(shell pwd)
+pkgdir := $(top_builddir)/pkg-deps
 
 # Extra flags to be passed to Emacs
 EFLAGS ?=
-- 
2.40.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-mk-Separate-third-party-package-dirs-for-different-E.patch --]
[-- Type: text/x-patch, Size: 2126 bytes --]

From ddebc0e86b43e655b057927ca9869143232c3ee5 Mon Sep 17 00:00:00 2001
Message-Id: <ddebc0e86b43e655b057927ca9869143232c3ee5.1681389559.git.yantar92@posteo.net>
In-Reply-To: <c5ba5773e69d16930fc12db6fbe0d907fae926cd.1681389559.git.yantar92@posteo.net>
References: <c5ba5773e69d16930fc12db6fbe0d907fae926cd.1681389559.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 10 Apr 2023 19:57:24 +0200
Subject: [PATCH 2/4] mk: Separate third-party package dirs for different Emacs
 versions

* mk/default.mk (EMACS_VERSION): New variable holding current Emacs
version, according to EMACS.
(pkgdir_top): New variable holding top pkgdir, parent of per-Emacs
version directories.
(pkgdir): Set as a sub-directory of pkgdir_top named as Emacs version.
* mk/targets.mk (uppkg): Do not clear .elc files.  This is no longer
needed as we do not need to worry about .elc being used by different
Emacs version.
(cleanpkg): Update, deleting the whole top pkgdir.
---
 mk/default.mk | 4 +++-
 mk/targets.mk | 3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mk/default.mk b/mk/default.mk
index 92a3942da..bd6efeb6f 100644
--- a/mk/default.mk
+++ b/mk/default.mk
@@ -6,6 +6,7 @@
 
 # Name of your emacs binary
 EMACS	= emacs
+EMACS_VERSION := $(shell $(EMACS) -Q --batch --eval '(message "%s" emacs-version)' 2>&1)
 
 # Where local software is found
 prefix	= /usr/share
@@ -33,7 +34,8 @@ testdir = $(TMPDIR)/tmp-orgtest
 
 # Where to store Org dependencies
 top_builddir := $(shell pwd)
-pkgdir := $(top_builddir)/pkg-deps
+pkgdir_top := $(top_builddir)/pkg-deps
+pkgdir := $(pkgdir_top)/$(EMACS_VERSION)
 
 # Extra flags to be passed to Emacs
 EFLAGS ?=
diff --git a/mk/targets.mk b/mk/targets.mk
index 072106237..2171159ac 100644
--- a/mk/targets.mk
+++ b/mk/targets.mk
@@ -105,7 +105,6 @@ endif
 uppkg::
 	$(info ========= Installing required third-party packages)
 	@$(MKDIR) -p $(pkgdir)
-	@$(FIND) $(pkgdir) \(  -name \*.elc \) -exec $(RM) {} +
 	-@$(INSTALL_PACKAGES)
 
 up0 up1 up2::
@@ -167,4 +166,4 @@ cleantest:
 	}
 
 cleanpkg:
-	-$(RMR) $(pkgdir)
+	-$(RMR) $(pkgdir_top)
-- 
2.40.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Makefile-Document-new-targets-in-make-helpall.patch --]
[-- Type: text/x-patch, Size: 1518 bytes --]

From a96ccb3b4d8b534e3d5a7bcccada368b0a220b69 Mon Sep 17 00:00:00 2001
Message-Id: <a96ccb3b4d8b534e3d5a7bcccada368b0a220b69.1681389559.git.yantar92@posteo.net>
In-Reply-To: <c5ba5773e69d16930fc12db6fbe0d907fae926cd.1681389559.git.yantar92@posteo.net>
References: <c5ba5773e69d16930fc12db6fbe0d907fae926cd.1681389559.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Thu, 13 Apr 2023 14:33:16 +0200
Subject: [PATCH 3/4] * Makefile: Document new targets in make helpall

Document the newly added cleanpkg and uppkg targets.
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index f476a3ea7..5e0e0fdff 100644
--- a/Makefile
+++ b/Makefile
@@ -41,6 +41,7 @@ helpall::
 	$(info Cleaning)
 	$(info ========)
 	$(info make clean          - remove built Org ELisp files and documentation)
+	$(info make cleanpkg      - remove third-party packages downloaded via make uppkg)
 	$(info make cleanall       - remove everything that can be built and all remnants)
 	$(info make clean-install  - remove previous Org installation)
 	$(info )
@@ -81,6 +82,7 @@ helpall::
 	$(info Convenience)
 	$(info ===========)
 	$(info make up0            - pull from upstream)
+	$(info make uppkg          - download third-party packages required for compilation)
 	$(info make up1            - pull from upstream, build and check)
 	$(info make up2            - pull from upstream, build, check and install)
 	$(info make update         - pull from upstream and build)
-- 
2.40.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-lisp-org.el-Require-lower-Compat-version.patch --]
[-- Type: text/x-patch, Size: 1327 bytes --]

From 0f5b5b445b649d6dda0a2637cc52ef6b6fed90e1 Mon Sep 17 00:00:00 2001
Message-Id: <0f5b5b445b649d6dda0a2637cc52ef6b6fed90e1.1681389559.git.yantar92@posteo.net>
In-Reply-To: <c5ba5773e69d16930fc12db6fbe0d907fae926cd.1681389559.git.yantar92@posteo.net>
References: <c5ba5773e69d16930fc12db6fbe0d907fae926cd.1681389559.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Thu, 13 Apr 2023 14:36:42 +0200
Subject: [PATCH 4/4] * lisp/org.el: Require lower Compat version

Compat 29.1.3.2 is pre-installed on current Ubuntu 23.04.
Do not require later version as it is (1) not yet necessary, (2) will
make life slightly easier for people who do not want to download staff
unnecessarily.

Link: https://list.orgmode.org/orgmode/u09ejk$vi1$1@ciao.gmane.io/
---
 lisp/org.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index 065813acf..26128815f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -7,7 +7,7 @@ ;;; org.el --- Outline-based notes management and organizer -*- lexical-binding:
 ;; Maintainer: Bastien Guerry <bzg@gnu.org>
 ;; Keywords: outlines, hypermedia, calendar, wp
 ;; URL: https://orgmode.org
-;; Package-Requires: ((emacs "26.1") (compat "29.1.4.1"))
+;; Package-Requires: ((emacs "26.1") (compat "29.1.3.2"))
 
 ;; Version: 9.7-pre
 
-- 
2.40.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0001-dev-org-build-system.org-Document-newly-added-target.patch --]
[-- Type: text/x-patch, Size: 2171 bytes --]

From 1d90f3459b9a51502dc40d683fafb45803335eb4 Mon Sep 17 00:00:00 2001
Message-Id: <1d90f3459b9a51502dc40d683fafb45803335eb4.1681389714.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Thu, 13 Apr 2023 14:32:20 +0200
Subject: [PATCH] * dev/org-build-system.org: Document newly added targets and
 options

(Make Targets =mk/targets.mk=): Document EGLAGS and EPACKAGES variables.
(Cleaning):
(Compatibility and Convenience): Document cleanpkg and uppkg targets.
---
 dev/org-build-system.org | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/dev/org-build-system.org b/dev/org-build-system.org
index 37d07fe5..a3a7d8e8 100644
--- a/dev/org-build-system.org
+++ b/dev/org-build-system.org
@@ -118,6 +118,13 @@ ** Make Targets =mk/targets.mk=
 be useful in some situations.  The following variables are considered
 stable:
 
+- =EFLAGS= :: Additional flags passed to Emacs executable.  Applies to
+              all targets.
+- =EPACKAGES= :: Additional packages to install before compilation.
+                 This is useful to install optional packages for
+                 test suite.  The packages will not be installed, if
+                 already available in the ~load-path~ that can be passed
+                 via =EFLAGS=.
 - =TEST_NO_AUTOCLEAN= :: Define to a non-null value to keep the test
      directory around for inspection.  This is mostly useful for
      debugging the test suite.
@@ -214,6 +221,7 @@ ** Documentation
 ** Cleaning
 
 - =clean= :: Cleans in =lisp/= and =doc/=.
+- =cleanpkg= :: Cleans in =pkg-deps/=.
 - =cleanall= :: Cleans everything that can be cleaned, including
                 several types of backup files, so do not use this when
                 you have active edit sessions!
@@ -224,6 +232,7 @@ ** Compatibility and Convenience
 
 - =up0= :: Updates the current Git branch from upstream by doing a
            =git pull=.
+- =uppkg= :: Download packages to be available for =make compile=.
 - =up1= :: Does =up0= and then builds and checks Org.
 - =up2= :: Does =up1= and installs Org if there was no test error.
 - =update= :: Does =up0= and then builds Org.  Does not test.
-- 
2.40.0


[-- Attachment #7: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>