unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: [elpa] master 872014e: Prevent accidental deletion of .git
       [not found] ` <E1ZvbIq-0004cL-VV@vcs.savannah.gnu.org>
@ 2015-11-09  1:58   ` Stefan Monnier
  2015-11-09  4:49     ` Thomas Fitzsimmons
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Monnier @ 2015-11-09  1:58 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: Fabián E. Gallina, emacs-devel

> +# Use && after the cd commands, not ;, to ensure the build fails
> +# immediately if the directory $(ARCHIVE_TMP)/packages does not exist.
> +# For process-archive this is crucial; otherwise batch-make-archive in
> +# archive-contents.el will interpret directories in the current
> +# directory as unreleased packages, and recursively delete them,
> +# including .git.  Prior to using &&, running "make process-archive"
> +# could silently delete all local git history!

Actually, I think the problem is in the code which does the deletion: it
should only do the delete files it positively knows should be deleted,
rather than deleting any files which seem to be out of place.

Once we change the handling of :core in elpa/admin/archive-contents.el
so that it sets up symlinks rather than performing copies, we should at
least be able to make the deletion less dangerous, e.g. by only deleting
symlinks and empty directories (so even if the deletion was erroneous,
no actual file contents was destroyed along the way).

Fabián, any news on this?


        Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-09  1:58   ` [elpa] master 872014e: Prevent accidental deletion of .git Stefan Monnier
@ 2015-11-09  4:49     ` Thomas Fitzsimmons
  2015-11-09  5:03       ` Stefan Monnier
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Fitzsimmons @ 2015-11-09  4:49 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Fabián E. Gallina, emacs-devel

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

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

>> +# Use && after the cd commands, not ;, to ensure the build fails
>> +# immediately if the directory $(ARCHIVE_TMP)/packages does not exist.
>> +# For process-archive this is crucial; otherwise batch-make-archive in
>> +# archive-contents.el will interpret directories in the current
>> +# directory as unreleased packages, and recursively delete them,
>> +# including .git.  Prior to using &&, running "make process-archive"
>> +# could silently delete all local git history!
>
> Actually, I think the problem is in the code which does the deletion: it
> should only do the delete files it positively knows should be deleted,
> rather than deleting any files which seem to be out of place.

Agreed that the deletion code is questionable, though I guess it's
operating under the assumption that it's within a throwaway directory
and is erring on the side of not publishing something not intended for
publication.

I think using && after the cd is just good practice, and I wanted to
push at least a partial fix for this right away since it bit me today.

> Once we change the handling of :core in elpa/admin/archive-contents.el
> so that it sets up symlinks rather than performing copies, we should at
> least be able to make the deletion less dangerous, e.g. by only deleting
> symlinks and empty directories (so even if the deletion was erroneous,
> no actual file contents was destroyed along the way).
>
> Fabián, any news on this?

Arthur pointed out in another thread that this code is at least
partially there already (I haven't heard from Fabián about its status).
The attached patch seems to work with a locally-generated archive, but
it introduces the first external to make use of :core.  Should I push it
and see what happens?

Thomas

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-ntlm-to-externals-list.patch --]
[-- Type: text/x-patch, Size: 878 bytes --]

From 4058dd04d88120b914a65f756356ac1aad42f224 Mon Sep 17 00:00:00 2001
From: Thomas Fitzsimmons <fitzsim@fitzsim.org>
Date: Sun, 8 Nov 2015 23:33:49 -0500
Subject: [PATCH] Add ntlm to externals-list

* externals-list ("ntlm"): New core entry.
---
 externals-list | 1 +
 1 file changed, 1 insertion(+)

diff --git a/externals-list b/externals-list
index ea0a69c..dd9ea61 100644
--- a/externals-list
+++ b/externals-list
@@ -59,6 +59,7 @@
  ("math-symbol-lists" 	:subtree "https://github.com/vspinu/math-symbol-lists.git")
  ("nameless" :subtree "https://github.com/Malabarba/Nameless")
  ("names" :subtree "http://github.com/Bruce-Connor/names")
+ ("ntlm"		:core "lisp/net/ntlm.el")
  ("omn-mode"            :external nil)
  ("pabbrev"             :external "https://github.com/phillord/pabbrev.git")
  ("pinentry"		:subtree "https://github.com/ueno/pinentry-el.git")
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-09  4:49     ` Thomas Fitzsimmons
@ 2015-11-09  5:03       ` Stefan Monnier
  2015-11-09 16:07         ` Eli Zaretskii
  2015-11-18 14:39         ` Thomas Fitzsimmons
  0 siblings, 2 replies; 17+ messages in thread
From: Stefan Monnier @ 2015-11-09  5:03 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: Fabián E. Gallina, emacs-devel

>> Once we change the handling of :core in elpa/admin/archive-contents.el
>> so that it sets up symlinks rather than performing copies, we should at
>> least be able to make the deletion less dangerous, e.g. by only deleting
>> symlinks and empty directories (so even if the deletion was erroneous,
>> no actual file contents was destroyed along the way).
>> Fabián, any news on this?

> Arthur pointed out in another thread that this code is at least
> partially there already (I haven't heard from Fabián about its status).

Right, Fabian installed its code already, but I found out at that point
that the way it works (i.e copying files from emacs.git) is problematic
in various ways: it should instead setup symlinks.

> The attached patch seems to work

Yes, it kinda works but it suffers from various warts (e.g. "make
externals; make" will always recompile all the :core packages).

> with a locally-generated archive, but it introduces the first external
> to make use of :core.  Should I push it and see what happens?

No, we should change to code to use symlinks instead of
file-copies first (and at the same time change it as I suggested,
i.e. to only delete symlinks and empty directories).


        Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-09  5:03       ` Stefan Monnier
@ 2015-11-09 16:07         ` Eli Zaretskii
  2015-11-09 16:38           ` Artur Malabarba
  2015-11-18 14:39         ` Thomas Fitzsimmons
  1 sibling, 1 reply; 17+ messages in thread
From: Eli Zaretskii @ 2015-11-09 16:07 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: fgallina, fitzsim, emacs-devel

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 09 Nov 2015 00:03:34 -0500
> Cc: "Fabián E. Gallina" <fgallina@gnu.org>,
> 	emacs-devel@gnu.org
> 
> Right, Fabian installed its code already, but I found out at that point
> that the way it works (i.e copying files from emacs.git) is problematic
> in various ways: it should instead setup symlinks.

Symlinks will cause problems on MS-Windows.




^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-09 16:07         ` Eli Zaretskii
@ 2015-11-09 16:38           ` Artur Malabarba
  2015-11-09 16:45             ` Eli Zaretskii
  0 siblings, 1 reply; 17+ messages in thread
From: Artur Malabarba @ 2015-11-09 16:38 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: fgallina, fitzsim, Stefan Monnier, emacs-devel

Eli Zaretskii <eliz@gnu.org> writes:

>> Right, Fabian installed its code already, but I found out at that point
>> that the way it works (i.e copying files from emacs.git) is problematic
>> in various ways: it should instead setup symlinks.
>
> Symlinks will cause problems on MS-Windows.

IIUC, this would only be a problem if you want to build a mirror of the
Gelpa repository on a Windows machine. It wouldn't affect Windows users
trying to install packages from elpa.gnu.org.



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-09 16:38           ` Artur Malabarba
@ 2015-11-09 16:45             ` Eli Zaretskii
  2015-11-09 20:20               ` Stefan Monnier
  0 siblings, 1 reply; 17+ messages in thread
From: Eli Zaretskii @ 2015-11-09 16:45 UTC (permalink / raw)
  To: Artur Malabarba; +Cc: fgallina, fitzsim, monnier, emacs-devel

> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  fgallina@gnu.org,  fitzsim@fitzsim.org,  emacs-devel@gnu.org
> Date: Mon, 09 Nov 2015 16:38:45 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Right, Fabian installed its code already, but I found out at that point
> >> that the way it works (i.e copying files from emacs.git) is problematic
> >> in various ways: it should instead setup symlinks.
> >
> > Symlinks will cause problems on MS-Windows.
> 
> IIUC, this would only be a problem if you want to build a mirror of the
> Gelpa repository on a Windows machine. It wouldn't affect Windows users
> trying to install packages from elpa.gnu.org.

Maybe I've misunderstood, but I thought it will affect everyone who
has a clone of Emacs and a clone of ELPA.



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-09 16:45             ` Eli Zaretskii
@ 2015-11-09 20:20               ` Stefan Monnier
  0 siblings, 0 replies; 17+ messages in thread
From: Stefan Monnier @ 2015-11-09 20:20 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: fgallina, fitzsim, Artur Malabarba, emacs-devel

> Maybe I've misunderstood, but I thought it will affect everyone who
> has a clone of Emacs and a clone of ELPA.

That will affect Windows users who make to use "make externals" in
elpa.git, yes.  We'll have to fall back to copying in that case.


        Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-09  5:03       ` Stefan Monnier
  2015-11-09 16:07         ` Eli Zaretskii
@ 2015-11-18 14:39         ` Thomas Fitzsimmons
  2015-11-18 15:10           ` Stefan Monnier
  1 sibling, 1 reply; 17+ messages in thread
From: Thomas Fitzsimmons @ 2015-11-18 14:39 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Fabián E. Gallina, emacs-devel

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

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

[...]

> No, we should change to code to use symlinks instead of
> file-copies first (and at the same time change it as I suggested,
> i.e. to only delete symlinks and empty directories).

How about the attached?  I've only tested it on a single-file package
(ntlm.el).  I could extend it to directories if the approach is what you
want.

Thomas


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-symbolic-links-for-core-packages.patch --]
[-- Type: text/x-patch, Size: 3099 bytes --]

From 73978a3d5c49c59e02b42000de6c8d9337e6c83b Mon Sep 17 00:00:00 2001
From: Thomas Fitzsimmons <fitzsim@fitzsim.org>
Date: Sun, 15 Nov 2015 21:25:04 -0500
Subject: [PATCH] Use symbolic links for core packages

* admin/archive-contents.el (archive--process-simple-package):
Copy file if it is a symlink to prevent deletion of target source
file.
(archive--core-package-copy-file): Create symbolic links for core
files.
(archive--core-package-sync): Change message to indicate linking.
---
 admin/archive-contents.el | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/admin/archive-contents.el b/admin/archive-contents.el
index 74e473e..4f3aee9 100755
--- a/admin/archive-contents.el
+++ b/admin/archive-contents.el
@@ -207,8 +207,11 @@ (defun archive--process-simple-package (dir pkg vers desc req extras)
   "Deploy the contents of DIR into the archive as a simple package.
 Rename DIR/PKG.el to PKG-VERS.el, delete DIR, and return the descriptor."
   ;; Write DIR/foo.el to foo-VERS.el and delete DIR
-  (rename-file (expand-file-name (concat pkg ".el") dir)
-	       (concat pkg "-" vers ".el"))
+  (if (file-symlink-p (expand-file-name (concat pkg ".el") dir))
+      (copy-file (expand-file-name (concat pkg ".el") dir)
+		 (concat pkg "-" vers ".el"))
+    (rename-file (expand-file-name (concat pkg ".el") dir)
+		 (concat pkg "-" vers ".el")))
   ;; Add the content of the ChangeLog.
   (let ((cl (expand-file-name "ChangeLog" dir)))
     (with-current-buffer (find-file-noselect (concat pkg "-" vers ".el"))
@@ -647,7 +650,7 @@ (defun archive--core-package-empty-dest-p (dest)
 
 (defun archive--core-package-copy-file
     (source dest emacs-repo-root package-root exclude-regexp)
-  "Copy file from SOURCE to DEST ensuring subdirectories."
+  "Link file from SOURCE to DEST ensuring subdirectories."
   (unless (string-match-p exclude-regexp source)
     (let* ((absolute-package-file-name
             (expand-file-name dest package-root))
@@ -656,7 +659,10 @@ (defun archive--core-package-copy-file
            (directory (file-name-directory absolute-package-file-name)))
       (unless (file-directory-p directory)
         (make-directory directory t))
-      (copy-file absolute-core-file-name absolute-package-file-name))
+      (if (memq system-type '(windows-nt ms-dos))
+	  (copy-file absolute-core-file-name absolute-package-file-name)
+	(make-symbolic-link absolute-core-file-name
+			    absolute-package-file-name t)))
     (message "  %s -> %s" source (if (archive--core-package-empty-dest-p dest)
                                      (file-name-nondirectory source)
                                    dest))))
@@ -711,7 +717,7 @@ (defun archive--core-package-sync (definition)
              ;; Files may be just a string, normalize.
              (list file-patterns)
            file-patterns))))
-    (message "Copying files for package: %s" name)
+    (message "Linking files for package: %s" name)
     (when (file-directory-p package-root)
       (delete-directory package-root t))
     (make-directory package-root t)
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-18 14:39         ` Thomas Fitzsimmons
@ 2015-11-18 15:10           ` Stefan Monnier
  2015-11-18 16:44             ` Stephen Leake
                               ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Stefan Monnier @ 2015-11-18 15:10 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: Fabián E. Gallina, emacs-devel

> How about the attached?

Looks like a good start, thank you.

> @@ -207,8 +207,11 @@ (defun archive--process-simple-package (dir pkg vers desc req extras)
>    "Deploy the contents of DIR into the archive as a simple package.
>  Rename DIR/PKG.el to PKG-VERS.el, delete DIR, and return the descriptor."
>    ;; Write DIR/foo.el to foo-VERS.el and delete DIR
> -  (rename-file (expand-file-name (concat pkg ".el") dir)
> -	       (concat pkg "-" vers ".el"))
> +  (if (file-symlink-p (expand-file-name (concat pkg ".el") dir))
> +      (copy-file (expand-file-name (concat pkg ".el") dir)
> +		 (concat pkg "-" vers ".el"))
> +    (rename-file (expand-file-name (concat pkg ".el") dir)
> +		 (concat pkg "-" vers ".el")))

I'd use

   (let ((src (expand-file-name (concat pkg ".el") dir)))
     (funcall (if (file-symlink-p src) #'copy-file #'rename-file)
              src (concat pkg "-" vers ".el")))

to avoid redundancy.

>  (defun archive--core-package-copy-file
>      (source dest emacs-repo-root package-root exclude-regexp)
> -  "Copy file from SOURCE to DEST ensuring subdirectories."
> +  "Link file from SOURCE to DEST ensuring subdirectories."

The function name would need to be correspondingly adjusted ;-)

> -      (copy-file absolute-core-file-name absolute-package-file-name))
> +      (if (memq system-type '(windows-nt ms-dos))
> +	  (copy-file absolute-core-file-name absolute-package-file-name)
> +	(make-symbolic-link absolute-core-file-name +
> absolute-package-file-name t)))

I don't have a Windows system to test it, but if possible I'd rather
have something like

   (if (fboundp 'make-symbolic-link) ...)

or

   (condition-case nil
       (make-symbolic-link absolute-core-file-name
                           absolute-package-file-name t)
     (<the-error-signalled-by-make-symbolic-link>
      (copy-file absolute-core-file-name absolute-package-file-name)))

We'd also want to try and fix the directory-deletion code accordingly,
so it's less trigger happy.


        Stefan "who also got caught by this directory deletion code"



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-18 15:10           ` Stefan Monnier
@ 2015-11-18 16:44             ` Stephen Leake
  2015-11-18 17:35             ` Eli Zaretskii
  2015-11-26  3:08             ` Thomas Fitzsimmons
  2 siblings, 0 replies; 17+ messages in thread
From: Stephen Leake @ 2015-11-18 16:44 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Fabián E. Gallina, Thomas Fitzsimmons, emacs-devel

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> I don't have a Windows system to test it, but if possible I'd rather
> have something like
>
>    (if (fboundp 'make-symbolic-link) ...)

make-symbolic-link is bound on Windows.

> or
>
>    (condition-case nil
>        (make-symbolic-link absolute-core-file-name
>                            absolute-package-file-name t)
>      (<the-error-signalled-by-make-symbolic-link>
>       (copy-file absolute-core-file-name absolute-package-file-name)))

The error is 'file-error

-- 
-- Stephe



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-18 15:10           ` Stefan Monnier
  2015-11-18 16:44             ` Stephen Leake
@ 2015-11-18 17:35             ` Eli Zaretskii
  2015-11-26  3:08             ` Thomas Fitzsimmons
  2 siblings, 0 replies; 17+ messages in thread
From: Eli Zaretskii @ 2015-11-18 17:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: fgallina, fitzsim, emacs-devel

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Wed, 18 Nov 2015 10:10:26 -0500
> Cc: "Fabián E. Gallina" <fgallina@gnu.org>,
> 	emacs-devel@gnu.org
> 
> > -      (copy-file absolute-core-file-name absolute-package-file-name))
> > +      (if (memq system-type '(windows-nt ms-dos))
> > +	  (copy-file absolute-core-file-name absolute-package-file-name)
> > +	(make-symbolic-link absolute-core-file-name +
> > absolute-package-file-name t)))
> 
> I don't have a Windows system to test it, but if possible I'd rather
> have something like
> 
>    (if (fboundp 'make-symbolic-link) ...)

That'd be okay for the MS-DOS case, but not for Windows: we do have
this function implemented there.




^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-18 15:10           ` Stefan Monnier
  2015-11-18 16:44             ` Stephen Leake
  2015-11-18 17:35             ` Eli Zaretskii
@ 2015-11-26  3:08             ` Thomas Fitzsimmons
  2015-11-26  3:55               ` Stefan Monnier
  2 siblings, 1 reply; 17+ messages in thread
From: Thomas Fitzsimmons @ 2015-11-26  3:08 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Fabián E. Gallina, emacs-devel

Hi Stefan,

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

[...]

> We'd also want to try and fix the directory-deletion code accordingly,
> so it's less trigger happy.

I've been looking at how to do this, but I'm not sure exactly what
you're after.  The following is from the perspective of "in place"
development.  For the deployment case, deletions etc. make perfect sense
since those scripts are operating on committed code.

There are lots of points where directory deletion happens in GNUmakefile
and archive-contents.el, during both "make externals" and "make
archive".

For "make archive", directory deletions all happen under "archive-tmp"
-- this is now enforced by my change to use cd ... && ... in the make
recipe.  The specific directory deletion that bit me is in
batch-make-achive:

    (progn ;; Negative version: don't publish this package yet!
      (message "Package %s not released yet!" dir)
      (delete-directory dir 'recursive))

To avoid `delete-directory' calls entirely, we could create archive-tmp
additively by only copying over releasable directories.  But then the
first step will still be "rm -rf archive-tmp" in the Makefile.  Do you
think it's worth the effort to rewrite this to do selective copying,
rather than just ensuring directory deletions only happen in
archive-tmp?

As for "make externals", I don't really like the concept of it, because
it mixes the build process with source configuration management.  For
example, this directory deletion could remove in-progress work from my
source directory, under elpa/packages:

   ;; Check if `dir' is under version control.
   ((not (zerop (call-process "git" nil nil nil
                              "ls-files" "--error-unmatch" dir)))
    (message "Deleted untracked package %s" dir)
    (delete-directory dir 'recursive t))

I would never expect that when invoking make.  I would rather "git
status" tell me that I have untracked changes.

I didn't closely follow the discussions while this was being designed.
Was there some reason to avoid git submodules?  It seems possible to
replace "make externals" with submodules that track "remote" externals/
branches of elpa.git itself, with git >= 1.8.2.  Then pure git
operations could be used to e.g. ensure a clean working tree, instead of
directory deletion Elisp.

Thomas



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-26  3:08             ` Thomas Fitzsimmons
@ 2015-11-26  3:55               ` Stefan Monnier
  2015-11-26 14:34                 ` Thomas Fitzsimmons
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Monnier @ 2015-11-26  3:55 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: Fabián E. Gallina, emacs-devel

>> We'd also want to try and fix the directory-deletion code accordingly,
>> so it's less trigger happy.
> I've been looking at how to do this, but I'm not sure exactly what
> you're after.  The following is from the perspective of "in place"
> development.

Yes, that's the important case.

> There are lots of points where directory deletion happens in GNUmakefile
> and archive-contents.el, during both "make externals" and "make
> archive".

Deletion in "make archive" is not a problem, AFAICT.

> As for "make externals", I don't really like the concept of it, because
> it mixes the build process with source configuration management.  For
> example, this directory deletion could remove in-progress work from my
> source directory, under elpa/packages:

Exactly.

>    ;; Check if `dir' is under version control.
>    ((not (zerop (call-process "git" nil nil nil
>                               "ls-files" "--error-unmatch" dir)))
>     (message "Deleted untracked package %s" dir)
>     (delete-directory dir 'recursive t))
>
> I would never expect that when invoking make.  I would rather "git
> status" tell me that I have untracked changes.

Yes, this is the problematic case.  This case is the one that intends to
handle the situation where we removed a :core package from
externals-list.

Presumably the call-process check should make sure this directory is not
under Git control at all, so no amount of "git status" will help.

So here, instead of delete-directory we should use a new function which
is a lot more careful, so it only deletes data which it has earlier
created for a :core package.  If the :core packages are handled via
symlinks, then this can be done by checking that the directory only
contains symlinks (or subdirs with the same property).  Of course there
are other ways, such as keeping track somewhere of the files we've
installed as :core, and then check this info before we delete files.

> I didn't closely follow the discussions while this was being designed.
> Was there some reason to avoid git submodules?

"git submodules" could make sense (and I did consider them) for
the :external packages, but these aren't affected by this code (since
the call-process should indicate that they are under Git).

> It seems possible to replace "make externals" with submodules that
> track "remote" externals/ branches of elpa.git itself, with git >=
> 1.8.2.

IIRC the issue with git submodules is that they have to refer to
a particular revision rather than to a branch, so additionally to
committing new code on the external branch, you'd then have to go and
commit a change to master that updates the submodule reference to point
to the new revision at the head of the external branch.


        Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-26  3:55               ` Stefan Monnier
@ 2015-11-26 14:34                 ` Thomas Fitzsimmons
  2015-11-26 15:42                   ` Stefan Monnier
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Fitzsimmons @ 2015-11-26 14:34 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Fabián E. Gallina, emacs-devel

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

>>> We'd also want to try and fix the directory-deletion code accordingly,
>>> so it's less trigger happy.
>> I've been looking at how to do this, but I'm not sure exactly what
>> you're after.  The following is from the perspective of "in place"
>> development.
>
> Yes, that's the important case.

BTW, I found myself being more focused on the "in place archive" use
case.  That's how I ended up invoking the process-archive target
directly which indirectly deleted .git (this is the one my patch now
prevents).

I think the default "make" target should also generate the archive
directory so that one can always test "what will be published" with:

rm -rf test-home && mkdir test-home && HOME=`pwd`/test-home $TARGET -Q \
    --eval \
    "(setq package-archives '((\"local\" . \".../elpa/archive/packages\")))"

Then one can test dependency resolution, byte-compilation and
compatibility on the "target Emacs" -- I test Emacs 24.1 through 24.5
and master.  Testing just against the default emacs on PATH (what "make"
does right now) is OK for development, but GNU ELPA should encourage
backward and forward compatibility.

>> There are lots of points where directory deletion happens in GNUmakefile
>> and archive-contents.el, during both "make externals" and "make
>> archive".
>
> Deletion in "make archive" is not a problem, AFAICT.
>
>> As for "make externals", I don't really like the concept of it, because
>> it mixes the build process with source configuration management.  For
>> example, this directory deletion could remove in-progress work from my
>> source directory, under elpa/packages:
>
> Exactly.
>
>>    ;; Check if `dir' is under version control.
>>    ((not (zerop (call-process "git" nil nil nil
>>                               "ls-files" "--error-unmatch" dir)))
>>     (message "Deleted untracked package %s" dir)
>>     (delete-directory dir 'recursive t))
>>
>> I would never expect that when invoking make.  I would rather "git
>> status" tell me that I have untracked changes.
>
> Yes, this is the problematic case.  This case is the one that intends to
> handle the situation where we removed a :core package from
> externals-list.
>
> Presumably the call-process check should make sure this directory is not
> under Git control at all, so no amount of "git status" will help.
>
> So here, instead of delete-directory we should use a new function which
> is a lot more careful, so it only deletes data which it has earlier
> created for a :core package.  If the :core packages are handled via
> symlinks, then this can be done by checking that the directory only
> contains symlinks (or subdirs with the same property).  Of course there
> are other ways, such as keeping track somewhere of the files we've
> installed as :core, and then check this info before we delete files.

OK, I'll work on this.

>> I didn't closely follow the discussions while this was being designed.
>> Was there some reason to avoid git submodules?
>
> "git submodules" could make sense (and I did consider them) for
> the :external packages, but these aren't affected by this code (since
> the call-process should indicate that they are under Git).
>
>> It seems possible to replace "make externals" with submodules that
>> track "remote" externals/ branches of elpa.git itself, with git >=
>> 1.8.2.
>
> IIRC the issue with git submodules is that they have to refer to
> a particular revision rather than to a branch, so additionally to
> committing new code on the external branch, you'd then have to go and
> commit a change to master that updates the submodule reference to point
> to the new revision at the head of the external branch.

That was true at one point but it looks like git 1.8.2 introduced the
notion of "remote-tracking branch submodules".  Maybe I'll experiment
with how that could work for elpa.git.

Thomas



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-26 14:34                 ` Thomas Fitzsimmons
@ 2015-11-26 15:42                   ` Stefan Monnier
  2015-11-27  6:58                     ` Thomas Fitzsimmons
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Monnier @ 2015-11-26 15:42 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: Fabián E. Gallina, emacs-devel

> BTW, I found myself being more focused on the "in place archive" use
> case.

I have no idea what "in-place archive" means here.  AFAIK none of the
code lets you generate an archive without making copies of the files
(i.e. it's not "in-place").

> That's how I ended up invoking the process-archive target
> directly which indirectly deleted .git (this is the one my patch now
> prevents).
[...]
> Then one can test dependency resolution, byte-compilation and
> compatibility on the "target Emacs" -- I test Emacs 24.1 through 24.5
> and master.  Testing just against the default emacs on PATH (what "make"
> does right now) is OK for development, but GNU ELPA should encourage
> backward and forward compatibility.

AFAIK "process-archive" won't byte-compile anything, so it's not very
useful for that.  It just shuffles things around so they're in the place
and form expected for package-install.

The purpose of the "in-place installation" method is so that those
packages behave kind of like Emacs's bundled packages (except that you
need to add the "packages" dir to your package-directory-list, so they
won't appear in "emacs -Q" and after "cd .../emacs; make" you also have
to do "cd .../elpa; make").  Just like Emacs's bundled packages, you
don't need to "package-install" them, or even to choose which ones
you want.


        Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-26 15:42                   ` Stefan Monnier
@ 2015-11-27  6:58                     ` Thomas Fitzsimmons
  2015-11-27 15:27                       ` Stefan Monnier
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Fitzsimmons @ 2015-11-27  6:58 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Fabián E. Gallina, emacs-devel

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

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> BTW, I found myself being more focused on the "in place archive" use
>> case.
>
> I have no idea what "in-place archive" means here.  AFAIK none of the
> code lets you generate an archive without making copies of the files
> (i.e. it's not "in-place").

OK, wasn't sure what to call it.  I meant invoking "make archive", which
creates the archive in the elpa working directory, including local
changes.  Basically, I was doing:

   make externals
   make
   make archive

>> That's how I ended up invoking the process-archive target
>> directly which indirectly deleted .git (this is the one my patch now
>> prevents).
> [...]
>> Then one can test dependency resolution, byte-compilation and
>> compatibility on the "target Emacs" -- I test Emacs 24.1 through 24.5
>> and master.  Testing just against the default emacs on PATH (what "make"
>> does right now) is OK for development, but GNU ELPA should encourage
>> backward and forward compatibility.
>
> AFAIK "process-archive" won't byte-compile anything, so it's not very
> useful for that.  It just shuffles things around so they're in the place
> and form expected for package-install.

Agreed, didn't mean to confuse things with the reference to
"process-archive".  I only called it while I was experimenting with how
the build process works.  I don't call it directly normally.

But for byte-compilation testing, I do a package-install in each target
emacs, and check the byte-compilation results reported by the target
emacs.  For that step I point at the result of "make archive".

I consider this the "package compatibility testing" use case, where I
want to make local changes that, after a make invocation, are made
available as installable packages.  Running admin/update-archive.sh
after each change isn't what I want, because it does things like trying
to pull elpa.git.

> The purpose of the "in-place installation" method is so that those
> packages behave kind of like Emacs's bundled packages (except that you
> need to add the "packages" dir to your package-directory-list, so they
> won't appear in "emacs -Q" and after "cd .../emacs; make" you also have
> to do "cd .../elpa; make").  Just like Emacs's bundled packages, you
> don't need to "package-install" them, or even to choose which ones
> you want.

OK.  I haven't been using that (other than as a test of byte compilation
against the default Emacs) since I've been doing Excorporate feature
development in a standalone git repository.  But I may use this work
flow once I start doing feature development in elpa.git.  For now I'm
only doing packaging/compatibility work.

Speaking of which, I'd like to push the attached patch set, which makes
externals deletion safer and adds the first two core packages to GNU
ELPA (ntlm and soap-client).  Look OK?

Thanks,
Thomas


[-- Attachment #2: add-two-core-packages-1.tar.gz --]
[-- Type: application/gzip, Size: 3988 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [elpa] master 872014e: Prevent accidental deletion of .git
  2015-11-27  6:58                     ` Thomas Fitzsimmons
@ 2015-11-27 15:27                       ` Stefan Monnier
  0 siblings, 0 replies; 17+ messages in thread
From: Stefan Monnier @ 2015-11-27 15:27 UTC (permalink / raw)
  To: Thomas Fitzsimmons; +Cc: Fabián E. Gallina, emacs-devel

> But for byte-compilation testing, I do a package-install in each target
> emacs, and check the byte-compilation results reported by the target
> emacs.  For that step I point at the result of "make archive".

That sounds excruciating.  Why not do:

   cd .../elpa
   rm **/*.elc
   make EMACS="emacs-23.1 --batch"

Admittedly, your system is slightly closer to what the end-user will do,
so it may give slightly more precise results in some cases.

> Speaking of which, I'd like to push the attached patch set, which makes
> externals deletion safer and adds the first two core packages to GNU
> ELPA (ntlm and soap-client).  Look OK?

[ Better just attach them so they're easier to view in the MUA.  ]

Looks fine to me, thanks,


        Stefan



^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-11-27 15:27 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20151109013124.17711.29422@vcs.savannah.gnu.org>
     [not found] ` <E1ZvbIq-0004cL-VV@vcs.savannah.gnu.org>
2015-11-09  1:58   ` [elpa] master 872014e: Prevent accidental deletion of .git Stefan Monnier
2015-11-09  4:49     ` Thomas Fitzsimmons
2015-11-09  5:03       ` Stefan Monnier
2015-11-09 16:07         ` Eli Zaretskii
2015-11-09 16:38           ` Artur Malabarba
2015-11-09 16:45             ` Eli Zaretskii
2015-11-09 20:20               ` Stefan Monnier
2015-11-18 14:39         ` Thomas Fitzsimmons
2015-11-18 15:10           ` Stefan Monnier
2015-11-18 16:44             ` Stephen Leake
2015-11-18 17:35             ` Eli Zaretskii
2015-11-26  3:08             ` Thomas Fitzsimmons
2015-11-26  3:55               ` Stefan Monnier
2015-11-26 14:34                 ` Thomas Fitzsimmons
2015-11-26 15:42                   ` Stefan Monnier
2015-11-27  6:58                     ` Thomas Fitzsimmons
2015-11-27 15:27                       ` Stefan Monnier

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).