core: contain side effects when loading packages for SPC h SPC

Fixes some edge cases like SPC f e R performed after SPC h SPC which
could wrongly install or uninstall packages.

Side effects is contained using the variable
configuration-layer--package-properties-read-onlyp, if non nil then
properties value of a package cannot be overwritten.

core/core-configuration-layer.el

@@ -200,6 +200,13 @@ LAYER has to be installed for this method to work properly."
     (when (configuration-layer/layer-usedp (car layers))
       (car layers))))
 
+(defmethod cfgl-package-set-property ((pkg cfgl-package) slot value)
+  "Set SLOT to the given VALUE for the package PKG.
+If `configuration-layer--package-properties-read-onlyp' is non-nil then VALUE
+is not set for the given SLOT."
+  (unless configuration-layer--package-properties-read-onlyp
+    (eval `(oset pkg ,slot value))))
+
 (defvar configuration-layer--elpa-archives
   '(("melpa" . "melpa.org/packages/")
     ("org"   . "orgmode.org/elpa/")
@@ -245,6 +252,10 @@ LAYER has to be installed for this method to work properly."
 (defvar configuration-layer--inhibit-warnings nil
   "If non-nil then warning message emitted by the layer system are ignored.")
 
+(defvar configuration-layer--package-properties-read-onlyp nil
+  "If non-nil then package properties are read only and cannot be overriden by
+`configuration-layer/make-package'.")
+
 (defvar configuration-layer--declared-layers-usedp nil
   "If non-nil then declared layers are considered to be used.")
 
@@ -549,10 +560,6 @@ If TOGGLEP is nil then `:toggle' parameter is ignored."
          (min-version (when (listp pkg) (plist-get (cdr pkg) :min-version)))
          (step (when (listp pkg) (plist-get (cdr pkg) :step)))
          (toggle (when (listp pkg) (plist-get (cdr pkg) :toggle)))
-         ;; (excluded (when (listp pkg)
-         ;;             (if (memq :excluded (cdr pkg))
-         ;;                 (plist-get (cdr pkg) :excluded)
-         ;;               'unspecified)))
          (excluded (when (listp pkg) (plist-get (cdr pkg) :excluded)))
          (location (when (listp pkg) (plist-get (cdr pkg) :location)))
          (protected (when (listp pkg) (plist-get (cdr pkg) :protected)))
@@ -567,11 +574,11 @@ If TOGGLEP is nil then `:toggle' parameter is ignored."
          (ownerp (or (and (eq 'dotfile layer-name)
                           (null (oref obj :owners)))
                      (fboundp init-func))))
-    (when min-version (oset obj :min-version (version-to-list min-version)))
-    (when step (oset obj :step step))
-    (when toggle (oset obj :toggle toggle))
-    (oset obj :excluded (or excluded (oref obj :excluded)))
-    ;; (unless (eq 'unspecified excluded) (oset obj :excluded excluded))
+    (when min-version
+      (cfgl-package-set-property obj :min-version (version-to-list min-version)))
+    (when step (cfgl-package-set-property obj :step step))
+    (when toggle (cfgl-package-set-property obj :toggle toggle))
+    (cfgl-package-set-property obj :excluded (or excluded (oref obj :excluded)))
     (when location
       (if (and (listp location)
                (eq (car location) 'recipe)
@@ -582,15 +589,17 @@ If TOGGLEP is nil then `:toggle' parameter is ignored."
                                        (configuration-layer/get-layer-local-dir
                                         layer-name)
                                        pkg-name-str pkg-name-str))))
-                    (oset obj :location `(recipe :fetcher file :path ,path))))
+                    (cfgl-package-set-property
+                     obj :location `(recipe :fetcher file :path ,path))))
            ((eq 'dotfile layer-name)
             ;; TODO what is the local path for a packages owned by the dotfile?
             nil))
-        (oset obj :location location)))
+        (cfgl-package-set-property obj :location location)))
     ;; cannot override protected packages
     (unless copyp
       ;; a bootstrap package is protected
-      (oset obj :protected (or protected (eq 'bootstrap step)))
+      (cfgl-package-set-property
+       obj :protected (or protected (eq 'bootstrap step)))
       (when protected
         (push pkg-name configuration-layer--protected-packages)))
     (when ownerp
@@ -891,8 +900,8 @@ variable as well."
           (if obj
               (setq obj (configuration-layer/make-package pkg layer-name obj))
             (setq obj (configuration-layer/make-package pkg layer-name)))
-          (configuration-layer//add-package obj (and (oref obj :owners)
-                                                     usedp)))))))
+          (configuration-layer//add-package
+           obj (and (cfgl-package-get-safe-owner obj) usedp)))))))
 
 (defun configuration-layer/make-packages-from-dotfile (&optional usedp)
   "Read the additonal packages declared in the dotfile and create packages.
@@ -909,7 +918,7 @@ USEDP if non-nil indicates that made packages are used packages."
       (unless obj
         (setq obj (configuration-layer/make-package xpkg 'dotfile)))
       (configuration-layer//add-package obj usedp)
-      (oset obj :excluded t))))
+      (cfgl-package-set-property obj :excluded t))))
 
 (defun configuration-layer/lazy-install (layer-name &rest props)
   "Configure auto-installation of layer with name LAYER-NAME."
@@ -932,7 +941,7 @@ USEDP if non-nil indicates that made packages are used packages."
                        :initial-value t)))
             (oset layer :lazy-install lazy)
             (dolist (pkg packages)
-              (oset pkg :lazy-install lazy)))))
+              (cfgl-package-set-property pkg :lazy-install lazy)))))
       (dolist (x extensions)
         (let ((ext (car x))
               (mode (cadr x)))
@@ -1252,10 +1261,10 @@ wether the declared layer is an used one or not."
           (cond
            ((or (null pkg) (eq 'elpa location))
             (configuration-layer//install-from-elpa pkg-name)
-            (when pkg (oset pkg :lazy-install nil)))
+            (when pkg (cfgl-package-set-property pkg :lazy-install nil)))
            ((and (listp location) (eq 'recipe (car location)))
             (configuration-layer//install-from-recipe pkg)
-            (oset pkg :lazy-install nil))
+            (cfgl-package-set-property pkg :lazy-install nil))
            (t (configuration-layer//warning "Cannot install package %S."
                                         pkg-name)))
         ('error
@@ -1292,7 +1301,7 @@ wether the declared layer is an used one or not."
             (delq nil (mapcar
                        (lambda (x)
                          (let ((pkg (configuration-layer/get-package x)))
-                           (oset pkg :lazy-install nil)
+                           (cfgl-package-set-property pkg :lazy-install nil)
                            pkg))
                        (oref layer :packages)))))
       (let ((last-buffer (current-buffer))

layers/+completion/helm/local/helm-spacemacs-help/helm-spacemacs-help.el

@@ -45,6 +45,7 @@
 (defun helm-spacemacs-help//init (&optional arg)
   (when (or arg (null helm-spacemacs--initialized))
     (let ((configuration-layer--load-packages-files t)
+          (configuration-layer--package-properties-read-onlyp t)
           (configuration-layer--inhibit-warnings t))
       (configuration-layer/discover-layers)
       (configuration-layer/declare-layers (configuration-layer/get-layers-list))

layers/+completion/ivy/local/ivy-spacemacs-help/ivy-spacemacs-help.el

@@ -43,6 +43,7 @@
 (defun ivy-spacemacs-help//init (&optional arg)
   (when (or arg (null ivy-spacemacs--initialized))
     (let ((configuration-layer--load-packages-files t)
+          (configuration-layer--package-properties-read-onlyp t)
           (configuration-layer--inhibit-warnings t))
       (configuration-layer/discover-layers)
       (configuration-layer/declare-layers (configuration-layer/get-layers-list))

tests/core/core-configuration-layer-utest.el

@@ -559,16 +559,16 @@
         (pkg '(testpkg :toggle bar))
         (expected (cfgl-package "testpkg"
                                 :name 'testpkg
-                                :owners '(layer-override-toggle-1)
+                                :owners '(layer-make-pkg-6)
                                 :toggle 'bar)))
-    (defun layer-override-toggle-1/init-testpkg nil)
+    (defun layer-make-pkg-6/init-testpkg nil)
     (helper--set-layers
-     `(,(cfgl-layer "layer-override-toggle-1" :name 'layer-override-toggle-1))
+     `(,(cfgl-layer "layer-make-pkg-6" :name 'layer-make-pkg-6))
      t)
     (should
      (equal
       expected
-      (configuration-layer/make-package pkg 'layer-override-toggle-1 obj)))))
+      (configuration-layer/make-package pkg 'layer-make-pkg-6 obj)))))
 
 (ert-deftest test-make-package--can-override-location ()
   (let* (configuration-layer--used-layers
@@ -579,14 +579,14 @@
          (pkg '(testpkg :location local))
          (expected (cfgl-package "testpkg"
                                  :name 'testpkg
-                                 :owners '(layer-override-loc-1)
+                                 :owners '(layer-make-pkg-7)
                                  :location 'local)))
-    (defun layer-override-loc-1/init-testpkg nil)
+    (defun layer-make-pkg-7/init-testpkg nil)
     (helper--set-layers
-     `(,(cfgl-layer "layer-override-loc-1" :name 'layer-override-loc-1)) t)
+     `(,(cfgl-layer "layer-make-pkg-7" :name 'layer-make-pkg-7)) t)
     (should
      (equal expected
-            (configuration-layer/make-package pkg 'layer-override-loc-1 obj)))))
+            (configuration-layer/make-package pkg 'layer-make-pkg-7 obj)))))
 
 (ert-deftest test-make-package--can-override-step ()
   (let* (configuration-layer--used-layers
@@ -597,15 +597,15 @@
          (pkg '(testpkg :step pre))
          (expected (cfgl-package "testpkg"
                                  :name 'testpkg
-                                 :owners '(layer-override-step-1)
+                                 :owners '(layer-make-pkg-8)
                                  :step 'pre)))
-    (defun layer-override-step-1/init-testpkg nil)
+    (defun layer-make-pkg-8/init-testpkg nil)
     (helper--set-layers
-     `(,(cfgl-layer "layer-override-step-1" :name 'layer-override-step-1)) t)
+     `(,(cfgl-layer "layer-make-pkg-8" :name 'layer-make-pkg-8)) t)
     (should
      (equal
       expected
-      (configuration-layer/make-package pkg 'layer-override-step-1 obj)))))
+      (configuration-layer/make-package pkg 'layer-make-pkg-8 obj)))))
 
 (ert-deftest test-make-package--cannot-override-protected ()
   (let* (configuration-layer--used-layers
@@ -616,16 +616,16 @@
          (pkg '(testpkg :protected nil))
          (expected (cfgl-package "testpkg"
                                  :name 'testpkg
-                                 :owners '(layer-override-protected-1)
+                                 :owners '(layer-make-pkg-9)
                                  :protected t)))
-    (defun layer-override-protected-1/init-testpkg nil)
+    (defun layer-make-pkg-9/init-testpkg nil)
     (helper--set-layers
-     `(,(cfgl-layer "layer-override-protected-1"
-                    :name 'layer-override-protected-1)) t)
+     `(,(cfgl-layer "layer-make-pkg-9"
+                    :name 'layer-make-pkg-9)) t)
     (should
      (equal
       expected
-      (configuration-layer/make-package pkg 'layer-override-protected-1 obj)))))
+      (configuration-layer/make-package pkg 'layer-make-pkg-9 obj)))))
 
 (ert-deftest test-make-package--cannot-unexclude-excluded-package ()
   (let* (configuration-layer--used-layers
@@ -637,9 +637,11 @@
          (expected (cfgl-package "testpkg"
                                  :name 'testpkg
                                  :excluded t)))
-    (helper--set-layers `(,(cfgl-layer "layer1" :name 'layer1)) t)
-    (should (equal expected
-                   (configuration-layer/make-package pkg 'layer1 obj)))))
+    (helper--set-layers
+     `(,(cfgl-layer "layer-make-pkg-10" :name 'layer-make-pkg-10)) t)
+    (should
+     (equal expected
+            (configuration-layer/make-package pkg 'layer-make-pkg-10 obj)))))
 
 (ert-deftest test-make-package--bootstrap-package-are-protected ()
   (let* (configuration-layer--used-layers
@@ -647,17 +649,16 @@
          (pkg '(testpkg :step bootstrap))
          (expected (cfgl-package "testpkg"
                                  :name 'testpkg
-                                 :owners '(layer-bootstrap-protected-1)
+                                 :owners '(layer-make-pkg-11)
                                  :step 'bootstrap
                                  :protected t)))
-    (defun layer-bootstrap-protected-1/init-testpkg nil)
+    (defun layer-make-pkg-11/init-testpkg nil)
     (helper--set-layers
-     `(,(cfgl-layer "layer-bootstrap-protected-1"
-                    :name 'layer-bootstrap-protected-1)) t)
+     `(,(cfgl-layer "layer-make-pkg-11" :name 'layer-make-pkg-11)) t)
     (should
      (equal
       expected
-      (configuration-layer/make-package pkg 'layer-bootstrap-protected-1)))))
+      (configuration-layer/make-package pkg 'layer-make-pkg-11)))))
 
 ;; ---------------------------------------------------------------------------
 ;; configuration-layer//get-distant-packages
@@ -1185,6 +1186,32 @@
                                :location 'local)
                  (ht-get configuration-layer--indexed-packages 'pkg2))))))
 
+(ert-deftest test-make-packages-from-layers--package-properties-read-only ()
+  ;; we expect that :excluded is still nil
+  (let* (configuration-layer--used-layers
+         (configuration-layer--indexed-layers (make-hash-table :size 1024))
+         (layer28 (cfgl-layer "layer28"
+                              :name 'layer28
+                              :packages '((pkg1 :excluded nil))))
+         (layer29 (cfgl-layer "layer29"
+                              :name 'layer29
+                              :packages '((pkg1 :excluded t))))
+         (expected (cfgl-package "pkg1"
+                                 :name 'pkg1
+                                 :owners '(layer28)
+                                 :excluded nil))
+         (mocker-mock-default-record-cls 'mocker-stub-record))
+    (defun layer28/init-pkg1 nil)
+    (helper--set-layers (list layer28) t)
+    (helper--set-layers (list layer29))
+    (mocker-let
+     ((configuration-layer//warning (msg &rest args) ((:output nil :occur 1))))
+     (configuration-layer/make-packages-from-layers '(layer28) t)
+     (let ((configuration-layer--package-properties-read-onlyp t))
+       (configuration-layer/make-packages-from-layers '(layer28 layer29)))
+     (should
+      (equal expected (ht-get configuration-layer--indexed-packages 'pkg1))))))
+
 ;; ---------------------------------------------------------------------------
 ;; configuration-layer/make-packages-from-dotfile
 ;; ---------------------------------------------------------------------------