bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Benjamin Orthen]

Recent changes in use-package (i.e., commit 
2ce279680bf9c1964e98e2aa48a03d6675c386fe) have not propagated to 
emacs-devel (https://elpa.gnu.org/devel/use-package.html) even though 
use-package is designated as a :core package in elpa.

My understanding is that changes in emacs for :core packages are 
automatically propagated to elpa(-devel). I was however not able to find 
documentation on how this propagation/sync works. I noticed that 
use-package.el does not have the disclaimer ";; This is a GNU ELPA :core 
package. Avoid adding functionality [...]" in its header. I suppose this 
is unrelated to the elpa-devel sync, but could be fixed as well.

Best regards,
Benjamin

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Philip Kaludercic]

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

Benjamin Orthen <benjamin@orthen.net> writes:

> Recent changes in use-package (i.e., commit
> 2ce279680bf9c1964e98e2aa48a03d6675c386fe) have not propagated to
> emacs-devel (https://elpa.gnu.org/devel/use-package.html) even though
> use-package is designated as a :core package in elpa.
>
> My understanding is that changes in emacs for :core packages are
> automatically propagated to elpa(-devel). I was however not able to
> find documentation on how this propagation/sync works. I noticed that
> use-package.el does not have the disclaimer ";; This is a GNU ELPA
> :core package. Avoid adding functionality [...]" in its header. I
> suppose this is unrelated to the elpa-devel sync, but could be fixed
> as well.

Unless I broke something on my local end, there seems to be a general
problem with the ELPA build system.  When trying to build use-package, I
get these error messages:

--8<---------------cut here---------------start------------->8---
$ make build/use-package
emacs --batch -Q -l admin/elpa-admin.el \
         -f elpaa-batch-pkg-spec-make-dependencies .pkg-descs.mk
emacs --batch -l /home/philip/Source/elpa/admin/elpa-admin.el	\
         -f elpaa-batch-make-one-package use-package
Linking files for package: use-package
  lisp/use-package/use-package.el -> use-package.el
  lisp/use-package/use-package-lint.el -> use-package-lint.el
  lisp/use-package/use-package-jump.el -> use-package-jump.el
  lisp/use-package/use-package-ensure.el -> use-package-ensure.el
  lisp/use-package/use-package-ensure-system-package.el -> use-package-ensure-system-package.el
  lisp/use-package/use-package-diminish.el -> use-package-diminish.el
  lisp/use-package/use-package-delight.el -> use-package-delight.el
  lisp/use-package/use-package-core.el -> use-package-core.el
  lisp/use-package/use-package-bind-key.el -> use-package-bind-key.el
  lisp/use-package/bind-key.el -> bind-key.el
  doc/emacs/doclicense.texi -> doclicense.texi
  doc/emacs/docstyle.texi -> docstyle.texi
  doc/misc/use-package.texi -> use-package.texi
======== Building tarball archive-devel/use-package-2.4.5.0.20230115.133305.tar...
Build error for archive-devel/use-package-2.4.5.0.20230115.133305.tar: (error "Error-indicating exit code in elpaa--call-sandboxed:
could not open use-package.texi: No such file or directory
")
######## Build of package archive-devel/use-package-2.4.5.0.20230115.133305.tar FAILED!!
======== Building tarball archive/use-package-2.4.5.tar...
Build error for archive/use-package-2.4.5.tar: (error "Error-indicating exit code in elpaa--call-sandboxed:
could not open use-package.texi: No such file or directory
")
######## Build of package archive/use-package-2.4.5.tar FAILED!!
--8<---------------cut here---------------end--------------->8---

But if I disable sandboxing, then it works.  The issue appears to be
that the files under package/use-package are linked in using symlinks,
but the directory (the emacs checkout) that is being linked to is not
exposed via bwarp.  If I add that to the ro-binds, then I can build the
package.  This diff might be enough to resolve the issue:


[-- Attachment #2: Type: text/plain, Size: 568 bytes --]

diff --git a/elpa-admin.el b/elpa-admin.el
index 24dd16d3cb..81467aa628 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -1262,7 +1262,8 @@ The INFILE and DISPLAY arguments are fixed as nil."
     "--tmpfs" "/tmp"))
 
 (defvar elpaa--sandbox-ro-binds
-  '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"))
+  `("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"
+    ,(file-truename (expand-file-name "emacs"))))
 
 (defun elpaa--call-sandboxed (destination &rest args)
   "Like ‘elpaa--call’ but sandboxed.

[-- Attachment #3: Type: text/plain, Size: 69 bytes --]


CC: Stefan, does this look OK to you?

>
> Best regards,
> Benjamin

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> But if I disable sandboxing, then it works.  The issue appears to be
> that the files under package/use-package are linked in using symlinks,
> but the directory (the emacs checkout) that is being linked to is not
> exposed via bwarp.  If I add that to the ro-binds, then I can build the
> package.  This diff might be enough to resolve the issue:
>
> diff --git a/elpa-admin.el b/elpa-admin.el
> index 24dd16d3cb..81467aa628 100644
> --- a/elpa-admin.el
> +++ b/elpa-admin.el
> @@ -1262,7 +1262,8 @@ The INFILE and DISPLAY arguments are fixed as nil."
>      "--tmpfs" "/tmp"))
>  
>  (defvar elpaa--sandbox-ro-binds
> -  '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"))
> +  `("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"
> +    ,(file-truename (expand-file-name "emacs"))))
[...]
> CC: Stefan, does this look OK to you?

Building of :core packages outside of `elpa.gnu.org` is indeed tested on
very lightly.  In `elpa.gnu.org`, the `emacs` subdirectory is not
a symlink so we avoid that problem.

The above patch feels a bit hackish because it depends on the
directory that happens to be current when the file is loaded.

BTW, you should be able to make the current code work without changing
it, by adding

    (sandbox-extra-ro-dirs "/where/ever/your/emacs/source/dir/is")

to the `elpa-config` file.


        Stefan

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Philip Kaludercic]

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

>> But if I disable sandboxing, then it works.  The issue appears to be
>> that the files under package/use-package are linked in using symlinks,
>> but the directory (the emacs checkout) that is being linked to is not
>> exposed via bwarp.  If I add that to the ro-binds, then I can build the
>> package.  This diff might be enough to resolve the issue:
>>
>> diff --git a/elpa-admin.el b/elpa-admin.el
>> index 24dd16d3cb..81467aa628 100644
>> --- a/elpa-admin.el
>> +++ b/elpa-admin.el
>> @@ -1262,7 +1262,8 @@ The INFILE and DISPLAY arguments are fixed as nil."
>>      "--tmpfs" "/tmp"))
>>  
>>  (defvar elpaa--sandbox-ro-binds
>> -  '("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"))
>> +  `("/lib" "/lib64" "/bin" "/usr" "/etc/alternatives" "/etc/emacs" "/gnu"
>> +    ,(file-truename (expand-file-name "emacs"))))
> [...]
>> CC: Stefan, does this look OK to you?
>
> Building of :core packages outside of `elpa.gnu.org` is indeed tested on
> very lightly.  In `elpa.gnu.org`, the `emacs` subdirectory is not
> a symlink so we avoid that problem.

OK, I forgot that I added the symlink.

> The above patch feels a bit hackish because it depends on the
> directory that happens to be current when the file is loaded.

I agree, that could be made more robust but isn't elpa-admin.el always
invoked via make?

> BTW, you should be able to make the current code work without changing
> it, by adding
>
>     (sandbox-extra-ro-dirs "/where/ever/your/emacs/source/dir/is")
>
> to the `elpa-config` file.

Of course, that was my first idea but I wanted to find a solution that
wouldn't require every user to manually configure this, since the error
message does not make it obvious what went wrong.

>
>         Stefan
>

-- 
Philip Kaludercic

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> The above patch feels a bit hackish because it depends on the
>> directory that happens to be current when the file is loaded.
> I agree, that could be made more robust but isn't elpa-admin.el always
> invoked via make?

In 99% of the cases, yes, but users are free to be more creative.

> Of course, that was my first idea but I wanted to find a solution that
> wouldn't require every user to manually configure this, since the error
> message does not make it obvious what went wrong.

I agree that these kinds of problems are annoying to debug.  It would be
nice to offer a "debug mode" where the user can see what's going on
within the sandbox.  Maybe we could do that by opening an interactive
shell with a message saying "this is the command that we want to run"
and then let the user run that command?


        Stefan

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Philip Kaludercic]

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

>>> The above patch feels a bit hackish because it depends on the
>>> directory that happens to be current when the file is loaded.
>> I agree, that could be made more robust but isn't elpa-admin.el always
>> invoked via make?
>
> In 99% of the cases, yes, but users are free to be more creative.
>
>> Of course, that was my first idea but I wanted to find a solution that
>> wouldn't require every user to manually configure this, since the error
>> message does not make it obvious what went wrong.
>
> I agree that these kinds of problems are annoying to debug.  It would be
> nice to offer a "debug mode" where the user can see what's going on
> within the sandbox.  Maybe we could do that by opening an interactive
> shell with a message saying "this is the command that we want to run"
> and then let the user run that command?

Shouldn't just invoking bash do that?

But returning to the initial issue (hoping I did not miss anything), if
the issue is not what I mentioned, then why is use-package not building?

-- 
Philip Kaludercic
 

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I agree that these kinds of problems are annoying to debug.  It would be
>> nice to offer a "debug mode" where the user can see what's going on
>> within the sandbox.  Maybe we could do that by opening an interactive
>> shell with a message saying "this is the command that we want to run"
>> and then let the user run that command?
> Shouldn't just invoking bash do that?

Yes, it should be fairly easy to do.

> But returning to the initial issue (hoping I did not miss anything), if
> the issue is not what I mentioned, then why is use-package not building?

You did not miss anything: it's not building simply because the process
run within the sandbox cannot read the target of the `use-package.texi`
symlink because the sandbox does not expose this target.


        Stefan

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Benjamin Orthen]

Hi,

I think I figured out why elpa does not build a new version of use-
package.

To determine the devel version, it looks only at the git log of `(elpa-
-main-file pkg-spec)`, which in this case turns out to be use-
package.el. However, use-package.el has not been changed since
20230115, I suppose this is why no new version is built.

Best, Benjamin


On Sun, 2023-06-11 at 12:55 -0400, Stefan Monnier wrote:
> > > I agree that these kinds of problems are annoying to debug.  It
> > > would be
> > > nice to offer a "debug mode" where the user can see what's going
> > > on
> > > within the sandbox.  Maybe we could do that by opening an
> > > interactive
> > > shell with a message saying "this is the command that we want to
> > > run"
> > > and then let the user run that command?
> > Shouldn't just invoking bash do that?
> 
> Yes, it should be fairly easy to do.
> 
> > But returning to the initial issue (hoping I did not miss
> > anything), if
> > the issue is not what I mentioned, then why is use-package not
> > building?
> 
> You did not miss anything: it's not building simply because the
> process
> run within the sandbox cannot read the target of the `use-
> package.texi`
> symlink because the sandbox does not expose this target.
> 
> 
>         Stefan
> 

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Benjamin Orthen]

[-- Attachment #1.1: Type: text/plain, Size: 1983 bytes --]

Related to this,

I have a patch for elpa-admin.el (in the elpa repository) which would
improve devel versioning for core packages:

Instead of looking only at the git log of the main package file, we
look instead at the git log of all files of the core package.
This way, a new devel version is created when any other package file is
changed, not just the main file.

Is this the right mailing list to send the patch? If not, where could I
send it to?


Best,
Benjamin

On Sun, 2023-06-11 at 19:32 +0200, Benjamin Orthen wrote:
> Hi,
> 
> I think I figured out why elpa does not build a new version of use-
> package.
> 
> To determine the devel version, it looks only at the git log of
> `(elpa-
> -main-file pkg-spec)`, which in this case turns out to be use-
> package.el. However, use-package.el has not been changed since
> 20230115, I suppose this is why no new version is built.
> 
> Best, Benjamin
> 
> 
> On Sun, 2023-06-11 at 12:55 -0400, Stefan Monnier wrote:
> > > > I agree that these kinds of problems are annoying to debug.  It
> > > > would be
> > > > nice to offer a "debug mode" where the user can see what's
> > > > going
> > > > on
> > > > within the sandbox.  Maybe we could do that by opening an
> > > > interactive
> > > > shell with a message saying "this is the command that we want
> > > > to
> > > > run"
> > > > and then let the user run that command?
> > > Shouldn't just invoking bash do that?
> > 
> > Yes, it should be fairly easy to do.
> > 
> > > But returning to the initial issue (hoping I did not miss
> > > anything), if
> > > the issue is not what I mentioned, then why is use-package not
> > > building?
> > 
> > You did not miss anything: it's not building simply because the
> > process
> > run within the sandbox cannot read the target of the `use-
> > package.texi`
> > symlink because the sandbox does not expose this target.
> > 
> > 
> >         Stefan
> > 
> 


[-- Attachment #1.2: Type: text/html, Size: 3379 bytes --]

[-- Attachment #2: 0001-Add-elpa-core-files-to-get-more-exact-devel-versions.patch --]
[-- Type: text/x-patch, Size: 3530 bytes --]

From d29163e00313690435b2baacc770a734598dd956 Mon Sep 17 00:00:00 2001
From: Benjamin Orthen <git@orthen.net>
Date: Fri, 8 Sep 2023 12:05:14 +0200
Subject: [PATCH] Add elpa--core-files to get more exact devel-versions for
 core packages

---
 elpa-admin.el | 51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/elpa-admin.el b/elpa-admin.el
index 2c2d2aeab7..57dfee8f18 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -908,20 +908,49 @@ of the current `process-environment'.  Return the modified copy."
                0)))
     (encode-time (list s mi h d mo y nil nil zs))))
 
+(defun elpaa--core-files (pkg-spec)
+  "Get a list of core files (and only files) for PKG-SPEC.
+Core folders are recursively searched, excluded files are ignored."
+  (when-let
+      ((core (elpaa--spec-get pkg-spec :core)))
+    (let*
+        ((excludes (elpaa--spec-get pkg-spec :excludes))
+         (emacs-repo-root (expand-file-name "emacs"))
+         (default-directory emacs-repo-root)
+         (file-patterns
+          (if (listp core)
+              core
+            (list core)))
+         (core-files nil))
+      ;; we look at each file or files in folder and add them
+      ;; to core-files if they are in the excludes
+      (cl-labels ((process-item (item)
+                    (unless (member item excludes)
+                      (if (file-directory-p item)
+                          (dolist (file (directory-files item nil directory-files-no-dot-files-regexp))
+                            (process-item (concat item file)))
+                        (push item core-files)))))
+        (dolist (item file-patterns)
+          (process-item item)))
+      core-files)))
+
 (defun elpaa--get-devel-version (dir pkg-spec)
   "Compute the date-based pseudo-version used for devel builds."
-  (let* ((ftn (file-truename      ;; Follow symlinks!
-              (expand-file-name (elpaa--main-file pkg-spec) dir)))
-         (default-directory (file-name-directory ftn))
-         (gitdate
+  (let* ((gitdate
           (with-temp-buffer
-           (if (plist-get (cdr pkg-spec) :core)
-               ;; For core packages, don't use the date of the last
-               ;; commit to the branch, but that of the last commit
-               ;; to the main file.
-               (elpaa--call t "git" "log" "--pretty=format:%cI" "--no-patch"
-                            "-1" "--" (file-name-nondirectory ftn))
-             (elpaa--call t "git" "show" "--pretty=format:%cI" "--no-patch"))
+            (if (plist-get (cdr pkg-spec) :core)
+                (let
+                    ((core-files (elpaa--core-files pkg-spec))
+                     (default-directory (expand-file-name "emacs")))
+                  ;; For core packages, don't use the date of the last
+                  ;; commit to the branch, but that of the last commit
+                  ;; to the core files.
+                  (apply 'elpaa--call t "git" "log" "--pretty=format:%cI" "--no-patch"
+                         "-1" "--" core-files))
+              (let* ((ftn (file-truename      ;; Follow symlinks!
+                           (expand-file-name (elpaa--main-file pkg-spec) dir)))
+                     (default-directory (file-name-directory ftn)))
+                (elpaa--call t "git" "show" "--pretty=format:%cI" "--no-patch")))
             (buffer-string)))
          (verdate
           ;; Convert Git's date into something that looks like a version number.
-- 
2.41.0

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Philip Kaludercic]

To: Benjamin Orthen <benjamin@orthen.net>
Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  63916@debbugs.gnu.org
Subject: Re: bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel
From: Philip Kaludercic <philipk@posteo.net>
Gcc: nnimap+posteo.de:Sent
--text follows this line--
Benjamin Orthen <benjamin@orthen.net> writes:

> Related to this,
>
> I have a patch for elpa-admin.el (in the elpa repository) which would
> improve devel versioning for core packages:
>
> Instead of looking only at the git log of the main package file, we
> look instead at the git log of all files of the core package.
> This way, a new devel version is created when any other package file is
> changed, not just the main file.

This seems to make sense.

> Is this the right mailing list to send the patch? If not, where could I
> send it to?

This is fine, the main thing is that someone has a patch that can be
applied.

>
> Best,
> Benjamin
>
> On Sun, 2023-06-11 at 19:32 +0200, Benjamin Orthen wrote:
>> Hi,
>> 
>> I think I figured out why elpa does not build a new version of use-
>> package.
>> 
>> To determine the devel version, it looks only at the git log of
>> `(elpa-
>> -main-file pkg-spec)`, which in this case turns out to be use-
>> package.el. However, use-package.el has not been changed since
>> 20230115, I suppose this is why no new version is built.
>> 
>> Best, Benjamin
>> 
>> 
>> On Sun, 2023-06-11 at 12:55 -0400, Stefan Monnier wrote:
>> > > > I agree that these kinds of problems are annoying to debug.  It
>> > > > would be
>> > > > nice to offer a "debug mode" where the user can see what's
>> > > > going
>> > > > on
>> > > > within the sandbox.  Maybe we could do that by opening an
>> > > > interactive
>> > > > shell with a message saying "this is the command that we want
>> > > > to
>> > > > run"
>> > > > and then let the user run that command?
>> > > Shouldn't just invoking bash do that?
>> > 
>> > Yes, it should be fairly easy to do.
>> > 
>> > > But returning to the initial issue (hoping I did not miss
>> > > anything), if
>> > > the issue is not what I mentioned, then why is use-package not
>> > > building?
>> > 
>> > You did not miss anything: it's not building simply because the
>> > process
>> > run within the sandbox cannot read the target of the `use-
>> > package.texi`
>> > symlink because the sandbox does not expose this target.
>> > 
>> > 
>> >         Stefan
>
> From d29163e00313690435b2baacc770a734598dd956 Mon Sep 17 00:00:00 2001
> From: Benjamin Orthen <git@orthen.net>
> Date: Fri, 8 Sep 2023 12:05:14 +0200
> Subject: [PATCH] Add elpa--core-files to get more exact devel-versions for
>  core packages
>
> ---
>  elpa-admin.el | 51 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/elpa-admin.el b/elpa-admin.el
> index 2c2d2aeab7..57dfee8f18 100644
> --- a/elpa-admin.el
> +++ b/elpa-admin.el
> @@ -908,20 +908,49 @@ of the current `process-environment'.  Return the modified copy."
>                 0)))
>      (encode-time (list s mi h d mo y nil nil zs))))
>  
> +(defun elpaa--core-files (pkg-spec)
> +  "Get a list of core files (and only files) for PKG-SPEC.
> +Core folders are recursively searched, excluded files are ignored."
> +  (when-let
> +      ((core (elpaa--spec-get pkg-spec :core)))

There is no need to fold this.

> +    (let*

Or this.

> +        ((excludes (elpaa--spec-get pkg-spec :excludes))
> +         (emacs-repo-root (expand-file-name "emacs"))
> +         (default-directory emacs-repo-root)
> +         (file-patterns
> +          (if (listp core)
> +              core
> +            (list core)))
> +         (core-files nil))

You can use list-ensure here.

> +      ;; we look at each file or files in folder and add them
> +      ;; to core-files if they are in the excludes
> +      (cl-labels ((process-item (item)
> +                    (unless (member item excludes)
> +                      (if (file-directory-p item)
> +                          (dolist (file (directory-files item nil directory-files-no-dot-files-regexp))
> +                            (process-item (concat item file)))
> +                        (push item core-files)))))
> +        (dolist (item file-patterns)
> +          (process-item item)))

Couldn't `directory-files-recursively' be useful here?

> +      core-files)))
> +
>  (defun elpaa--get-devel-version (dir pkg-spec)
>    "Compute the date-based pseudo-version used for devel builds."
> -  (let* ((ftn (file-truename      ;; Follow symlinks!
> -              (expand-file-name (elpaa--main-file pkg-spec) dir)))
> -         (default-directory (file-name-directory ftn))
> -         (gitdate
> +  (let* ((gitdate
>            (with-temp-buffer
> -           (if (plist-get (cdr pkg-spec) :core)
> -               ;; For core packages, don't use the date of the last
> -               ;; commit to the branch, but that of the last commit
> -               ;; to the main file.
> -               (elpaa--call t "git" "log" "--pretty=format:%cI" "--no-patch"
> -                            "-1" "--" (file-name-nondirectory ftn))
> -             (elpaa--call t "git" "show" "--pretty=format:%cI" "--no-patch"))
> +            (if (plist-get (cdr pkg-spec) :core)
> +                (let
> +                    ((core-files (elpaa--core-files pkg-spec))
> +                     (default-directory (expand-file-name "emacs")))
> +                  ;; For core packages, don't use the date of the last
> +                  ;; commit to the branch, but that of the last commit
> +                  ;; to the core files.
> +                  (apply 'elpaa--call t "git" "log" "--pretty=format:%cI" "--no-patch"

While we are at it, you might as well sharp-quote the `elpaa-call' here.

> +                         "-1" "--" core-files))
> +              (let* ((ftn (file-truename      ;; Follow symlinks!
> +                           (expand-file-name (elpaa--main-file pkg-spec) dir)))
> +                     (default-directory (file-name-directory ftn)))
> +                (elpaa--call t "git" "show" "--pretty=format:%cI" "--no-patch")))
>              (buffer-string)))
>           (verdate
>            ;; Convert Git's date into something that looks like a version number.

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

> Instead of looking only at the git log of the main package file, we
> look instead at the git log of all files of the core package.
> This way, a new devel version is created when any other package file is
> changed, not just the main file.

It has a minor downside which is that it slows down the overwhelming
cases (where there are no changes), but it's probably lost in the noise,
so it sounds like a very good idea, thank you.

> Is this the right mailing list to send the patch?

Yes.

[ BTW, Philip's `list-ensure` is called `ensure-list` :-)
  And yes, I think it's OK to use functionality from Emacs-28.  ]

>+  (when-let
>+      ((core (elpaa--spec-get pkg-spec :core)))

AFAICT this code is only used when `:core` is non-nil, so better either
not test for it redundantly, or do it via `cl-assert`.

>+                    (unless (member item excludes)

The code looks generally good, but I'm not sure about the one line
above:
- I know it won't do the right thing for ERC's "lisp/erc/ChangeLog.*"
  but it's probably harmless (those files basically never change
  anyway).
- I have the impression that it won't do the right thing for
  `use-package`'s "bind-key.el"

Also, I see you do (concat item file) but you only know that `item` is
a directory, not that it ends with a `/`.  Better use `file-name-concat`.

Last but not least: I think this doesn't quite qualify as "trivial" so
we'd need you to sign the copyright paperwork (well, maybe with some of
the suggested simplifications, it could qualify as "trivial", but it's
easier if you sign the paperwork so we don't have to worry about it,
especially if you ever submit more code, which I hope you will).
To that end, please fill the form below and email it to the FSF as
instructed so they can send you the appropriate paperwork to sign.


        Stefan


Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]


[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]


[For the copyright registration, what country are you a citizen of?]


[What year were you born?]


[Please write your email address here.]


[Please write your postal address here.]





[Which files have you changed so far, and which new files have you written
so far?]

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Benjamin Orthen]

[-- Attachment #1.1: Type: text/plain, Size: 3869 bytes --]

Hi,

I've sent the mail to assign@gnu.org and request the assignment form.

I modified the changes according to your and Philip's remarks.

On Fri, 2023-09-08 at 15:35 -0400, Stefan Monnier wrote:
> Hi,
> 
> > Instead of looking only at the git log of the main package file, we
> > look instead at the git log of all files of the core package.
> > This way, a new devel version is created when any other package
> > file is
> > changed, not just the main file.
> 
> It has a minor downside which is that it slows down the overwhelming
> cases (where there are no changes), but it's probably lost in the
> noise,
> so it sounds like a very good idea, thank you.
> 
> > Is this the right mailing list to send the patch?
> 
> Yes.
> 
> [ BTW, Philip's `list-ensure` is called `ensure-list` :-)
>   And yes, I think it's OK to use functionality from Emacs-28.  ]
> 
> > +  (when-let
> > +      ((core (elpaa--spec-get pkg-spec :core)))
> 
> AFAICT this code is only used when `:core` is non-nil, so better
> either
> not test for it redundantly, or do it via `cl-assert`.

Done
> 
> > +                    (unless (member item excludes)
> 
> The code looks generally good, but I'm not sure about the one line
> above:
> - I know it won't do the right thing for ERC's "lisp/erc/ChangeLog.*"
>   but it's probably harmless (those files basically never change
>   anyway).
> - I have the impression that it won't do the right thing for
>   `use-package`'s "bind-key.el"

I changed the code to ignore files using the wildcards in :excludes.
This still does not work for "bind-key.el", but IMO this can and should
be solved by changing it to "lisp/use-package/bind-key.el", 
similar to how erc is doing it ("lisp/erc/erc-loaddefs.el" ).

(The commentary says: ";; Exclude matches must be against the full
file-name, substring matches don't
;; work unless wildcards are used (e.g. use "etc/*" instead of
"etc/").")

> 
> Also, I see you do (concat item file) but you only know that `item`
> is
> a directory, not that it ends with a `/`.  Better use `file-name-
> concat`.

With `directory-files-recursively` this has resolved itself.
> 
> Last but not least: I think this doesn't quite qualify as "trivial"
> so
> we'd need you to sign the copyright paperwork (well, maybe with some
> of
> the suggested simplifications, it could qualify as "trivial", but
> it's
> easier if you sign the paperwork so we don't have to worry about it,
> especially if you ever submit more code, which I hope you will).
> To that end, please fill the form below and email it to the FSF as
> instructed so they can send you the appropriate paperwork to sign.
> 
> 
>         Stefan
> 
> 
> Please email the following information to assign@gnu.org, and we
> will send you the assignment form for your past and future changes.
> 
> Please use your full legal name (in ASCII characters) as the subject
> line of the message.
> ----------------------------------------------------------------------
> REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES
> 
> [What is the name of the program or package you're contributing to?]
> Emacs
> 
> [Did you copy any files or text written by someone else in these
> changes?
> Even if that material is free software, we need to know about it.]
> 
> 
> [Do you have an employer who might have a basis to claim to own
> your changes?  Do you attend a school which might make such a claim?]
> 
> 
> [For the copyright registration, what country are you a citizen of?]
> 
> 
> [What year were you born?]
> 
> 
> [Please write your email address here.]
> 
> 
> [Please write your postal address here.]
> 
> 
> 
> 
> 
> [Which files have you changed so far, and which new files have you
> written
> so far?]
> 


[-- Attachment #1.2: Type: text/html, Size: 6408 bytes --]

[-- Attachment #2: 0001-Add-elpa-core-files-to-get-more-exact-devel-versions.patch --]
[-- Type: text/x-patch, Size: 3624 bytes --]

From 0290e2f2e9bc5a3615dde54c142ae3cb54dc2274 Mon Sep 17 00:00:00 2001
From: Benjamin Orthen <git@orthen.net>
Date: Fri, 8 Sep 2023 12:05:14 +0200
Subject: [PATCH] Add elpa--core-files to get more exact devel-versions for
 core packages

---
 elpa-admin.el | 54 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/elpa-admin.el b/elpa-admin.el
index 2c2d2aeab7..d9a22b0055 100644
--- a/elpa-admin.el
+++ b/elpa-admin.el
@@ -908,20 +908,52 @@ of the current `process-environment'.  Return the modified copy."
                0)))
     (encode-time (list s mi h d mo y nil nil zs))))
 
+(defun elpaa--core-files (pkg-spec)
+  "Get a list of core files (and only files) for PKG-SPEC.
+Core folders are recursively searched, excluded files are ignored."
+  (let* ((file-patterns (ensure-list (elpaa--spec-get pkg-spec :core)))
+         (excludes (elpaa--spec-get pkg-spec :excludes))
+         (emacs-repo-root (expand-file-name "emacs"))
+         (default-directory emacs-repo-root)
+         (core-files nil))
+
+    ;; ensure we look at files from a core package
+    (cl-assert file-patterns)
+
+    ;; we look at each file or files in folder and add them
+    ;; to core-files
+    (dolist (item file-patterns)
+      (if (file-directory-p item)
+          (setq core-files (append core-files (directory-files-recursively item ".*")))
+        (push item core-files)))
+
+    ;; remove all files which match a wildcard in the excludes
+    (setq core-files (cl-remove-if
+                      (lambda (file-name)
+                        (seq-some
+                         (lambda (wildcard)
+                           (string-match-p (wildcard-to-regexp wildcard) file-name))
+                         excludes))
+                      core-files))
+    core-files))
+
 (defun elpaa--get-devel-version (dir pkg-spec)
   "Compute the date-based pseudo-version used for devel builds."
-  (let* ((ftn (file-truename      ;; Follow symlinks!
-              (expand-file-name (elpaa--main-file pkg-spec) dir)))
-         (default-directory (file-name-directory ftn))
-         (gitdate
+  (let* ((gitdate
           (with-temp-buffer
-           (if (plist-get (cdr pkg-spec) :core)
-               ;; For core packages, don't use the date of the last
-               ;; commit to the branch, but that of the last commit
-               ;; to the main file.
-               (elpaa--call t "git" "log" "--pretty=format:%cI" "--no-patch"
-                            "-1" "--" (file-name-nondirectory ftn))
-             (elpaa--call t "git" "show" "--pretty=format:%cI" "--no-patch"))
+            (if (plist-get (cdr pkg-spec) :core)
+                (let
+                    ((core-files (elpaa--core-files pkg-spec))
+                     (default-directory (expand-file-name "emacs")))
+                  ;; For core packages, don't use the date of the last
+                  ;; commit to the branch, but that of the last commit
+                  ;; to the core files.
+                  (apply #'elpaa--call t "git" "log" "--pretty=format:%cI" "--no-patch"
+                         "-1" "--" core-files))
+              (let* ((ftn (file-truename      ;; Follow symlinks!
+                           (expand-file-name (elpaa--main-file pkg-spec) dir)))
+                     (default-directory (file-name-directory ftn)))
+                (elpaa--call t "git" "show" "--pretty=format:%cI" "--no-patch")))
             (buffer-string)))
          (verdate
           ;; Convert Git's date into something that looks like a version number.
-- 
2.41.0

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Benjamin Orthen]

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

Hi,

the assignment/disclaimer process has been completed, so this is not
blocking the patch anymore.


Best,
Benjamin


On Mon, 2023-09-11 at 11:58 +0200, Benjamin Orthen wrote:
> Hi,
> 
> I've sent the mail to assign@gnu.org and request the assignment form.
> 
> I modified the changes according to your and Philip's remarks.
> 
> On Fri, 2023-09-08 at 15:35 -0400, Stefan Monnier wrote:
> > Hi,
> > 
> > > Instead of looking only at the git log of the main package file,
> > > we
> > > look instead at the git log of all files of the core package.
> > > This way, a new devel version is created when any other package
> > > file is
> > > changed, not just the main file.
> > 
> > It has a minor downside which is that it slows down the
> > overwhelming
> > cases (where there are no changes), but it's probably lost in the
> > noise,
> > so it sounds like a very good idea, thank you.
> > 
> > > Is this the right mailing list to send the patch?
> > 
> > Yes.
> > 
> > [ BTW, Philip's `list-ensure` is called `ensure-list` :-)
> >   And yes, I think it's OK to use functionality from Emacs-28.  ]
> > 
> > > +  (when-let
> > > +      ((core (elpaa--spec-get pkg-spec :core)))
> > 
> > AFAICT this code is only used when `:core` is non-nil, so better
> > either
> > not test for it redundantly, or do it via `cl-assert`.
> 
> Done
> > 
> > > +                    (unless (member item excludes)
> > 
> > The code looks generally good, but I'm not sure about the one line
> > above:
> > - I know it won't do the right thing for ERC's
> > "lisp/erc/ChangeLog.*"
> >   but it's probably harmless (those files basically never change
> >   anyway).
> > - I have the impression that it won't do the right thing for
> >   `use-package`'s "bind-key.el"
> 
> I changed the code to ignore files using the wildcards in :excludes.
> This still does not work for "bind-key.el", but IMO this can and
> should be solved by changing it to "lisp/use-package/bind-key.el", 
> similar to how erc is doing it ("lisp/erc/erc-loaddefs.el" ).
> 
> (The commentary says: ";; Exclude matches must be against the full
> file-name, substring matches don't
> ;; work unless wildcards are used (e.g. use "etc/*" instead of
> "etc/").")
> 
> > 
> > Also, I see you do (concat item file) but you only know that `item`
> > is
> > a directory, not that it ends with a `/`.  Better use `file-name-
> > concat`.
> 
> With `directory-files-recursively` this has resolved itself.
> > 
> > Last but not least: I think this doesn't quite qualify as "trivial"
> > so
> > we'd need you to sign the copyright paperwork (well, maybe with
> > some of
> > the suggested simplifications, it could qualify as "trivial", but
> > it's
> > easier if you sign the paperwork so we don't have to worry about
> > it,
> > especially if you ever submit more code, which I hope you will).
> > To that end, please fill the form below and email it to the FSF as
> > instructed so they can send you the appropriate paperwork to sign.
> > 
> > 
> >         Stefan
> > 
> > 
> > Please email the following information to assign@gnu.org, and we
> > will send you the assignment form for your past and future changes.
> > 
> > Please use your full legal name (in ASCII characters) as the
> > subject
> > line of the message.
> > -------------------------------------------------------------------
> > ---
> > REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES
> > 
> > [What is the name of the program or package you're contributing
> > to?]
> > Emacs
> > 
> > [Did you copy any files or text written by someone else in these
> > changes?
> > Even if that material is free software, we need to know about it.]
> > 
> > 
> > [Do you have an employer who might have a basis to claim to own
> > your changes?  Do you attend a school which might make such a
> > claim?]
> > 
> > 
> > [For the copyright registration, what country are you a citizen
> > of?]
> > 
> > 
> > [What year were you born?]
> > 
> > 
> > [Please write your email address here.]
> > 
> > 
> > [Please write your postal address here.]
> > 
> > 
> > 
> > 
> > 
> > [Which files have you changed so far, and which new files have you
> > written
> > so far?]
> > 
> 


[-- Attachment #2: Type: text/html, Size: 7097 bytes --]

bug#63916: 30.0.50; use-package: changes do not propagate to elpa-devel

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I modified the changes according to your and Philip's remarks.

Thank you Benjamin.
I just pushed it to `elpa-admin` and installed it into elpa.gnu.org.


        Stefan