syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.64k stars 4.89k forks source link

org-roam-setup cause race condition with user config file #15724

Closed emacs18 closed 1 week ago

emacs18 commented 2 years ago

Following is part of org-roam setup within spacemacs:

(defun org/init-org-roam ()
  (use-package org-roam
    :defer t
    :hook (after-init . org-roam-setup)
    :init
    ...))

org-roam-setup is an obsolete function as indicated by these lines from org-roam:

(define-obsolete-function-alias
  'org-roam-setup
  'org-roam-db-autosync-enable "org-roam 2.0")

The damage done by adding org-roam-setup is that automatic database update mode is enabled. This cause org-roam-db-sync function to be called which will then start to update sqlite database of all my org files. The problem is that org-roam-db-sync is called before my init file is loaded! Thus org-roam-directory has not yet been setup properly. org-roam-db-sync thus uses incorrect directories and happily updates the sqlite db.

After emacs comes up if I now execute org-roam-db-sync manually, then it thinks that all files are out of date, because org-roam-directory has now changed! Thus sqlite db is updated for all my files again. At this point if I restart emacs, then the cycle repeats all over again where sqlite db for all my org files are updated again. This can add up to 8 minute delay in my emacs start up time! For some unknown reason sqlite db update for some files take very long time like two minutes for some files.

What I would like to ask is that we not enable automatic update of org-roam database and let the user choose when this is enabled, i.e., do not add org-roam-setup to after-init-hook.

lebensterben commented 2 years ago

should we use

org-roam-db-autosync-mode instead?

this seems to be indicated in

https://www.orgroam.com/manual.html#Setting-up-Org_002droam

emacs18 commented 2 years ago

org-roam-db-autosync-mode should be used instead of obsolete org-roam-setup if one were to enable this minor mode.

However this should not be used until after user configuration is done, i.e., dotspacemacs/user-config function has been executed.

lebensterben commented 2 years ago

I do not use org-roam (except for debugging..). Can you show me what user might need to config in Spacemacs/user-config?

emacs18 commented 2 years ago

(org-roam-db-autosync-mode) enables the minor mode which updates sqlite db of org files as they are modified. This is mentioned in the link that you mentioned above.

lebensterben commented 2 years ago

one solution is to tell users to call org-roam-db-autosync-mode AFTER any user configuration.

another solution is to tell users to add all user configurations in

(spacemacs|use-package-add-hook org-roam
  :pre-config
  ;; user configuration
  )

and we call org-roam-db-autosync-mode inside the regular :config.

What do you think?

emacs18 commented 2 years ago

My preference would be to simply tell to call the function in question to enable the minor mode. This is the documented method in org-roam manual.

I think the alternative seems much more complicated. I don't know what spacemacs|use-package-add-hook does. Even if I were to figure out what it does once, I'm sure I'll forget it soon. So I would rather not deal with things like this.

breadncup commented 2 years ago

So, should we not use post-config for org-roam at all?

It looks like post-config is never being called.

lebensterben commented 2 years ago

@breadncup

I've no idea why you bring up post-config... It's not used.

breadncup commented 2 years ago
(spacemacs|use-package-add-hook org-roam
  :post-config
  ;; user configuration
  )

I used the above format, and it works well to me before this change.

lebensterben commented 2 years ago

@breadncup

what do you have in :post-config

breadncup commented 2 years ago

I have a private layer for my purpose and use this:

(defun bnc_personal/pre-init-org-roam ()
  "Pre-initialize org-roam for personal configuration."
  (spacemacs|use-package-add-hook org-roam
    :post-init
    (progn
      (setq org-roam-directory (expand-file-name "~/roamfiles/")
            org-roam-db-location (concat bnc:dir:base "org-roam.db")
            )
      )
    :post-config
    ;; Journal Template
    (let (
          (mylist
           `(
             ("d" "default" entry "* %?"
              :target (file+head "%<%Y-%m-%d>.org.gpg" "#+setupfile: ../personal/personal_org_setup.org\n#+title: %<%Y-%m-%d>")
              :jump-to-captured t
              )
             ))
          )
      (setq org-roam-dailies-capture-templates mylist)
      )
    ))

And, it was working before this change, and now org-roam-dailies-capture-templates is not even shown.

Excluding this change, the above works fine.

lebensterben commented 2 years ago

Just adding org-roam-db-autosync-mode in your :post-config should work,

breadncup commented 2 years ago

It doesn't seem to work.

(defun bnc_personal/pre-init-org-roam ()
  "Pre-initialize org-roam for personal configuration."
  (spacemacs|use-package-add-hook org-roam
    :post-init
    (progn
      (setq org-roam-directory (expand-file-name "~/roamfiles/")
            org-roam-db-location (concat bnc:dir:base "org-roam.db")
            )
      )
    :post-config
    (org-roam-db-autosync-mode)
    ;; Journal Template
    (let* (
           (mylist
            `(
              ("d" "default" entry "* %?"
               :target (file+head "%<%Y-%m-%d>.org.gpg" "#+setupfile: ../personal/personal_org_setup.org\n#+title: %<%Y-%m-%d>")
               :jump-to-captured t
               )
              ))
           )
      (setq org-roam-dailies-capture-templates mylist)
      )
    ))

It seems that post-config section is not running at all. I put a test code like (setq mydummy t) but mydummy is not seen at all. Without this change, I'm able to see the test variable of mydummy is seen.

lebensterben commented 2 years ago

C-h v use-package--org-roam--post-config-hook

Is it empty?

lebensterben commented 2 years ago

you may also try to move it from :post-config to :post-init.

breadncup commented 2 years ago

@lebensterben

Thanks,

Moving (org-roam-db-autosync-mode) to :post-init, then it works now.

emacs18 commented 2 years ago

This is what I have in my init file which is loaded by dotspacemacs/user-config:

(use-package org-roam
  :init
  (setq org-roam-directory ...)
  :config
  (org-roam-db-autosync-enable)
  )

This seems to work for me. Isn't this much simpler?

emacs18 commented 2 years ago

My init file is setup to use vanilla code that is not spacemacs specific, because my init file may be loaded by spacemacs, doom, scimax, etc.

Since this more general (and simpler) code seems to work fine, I don't see why I should resort to spacemacs specific code which is also more complicated.

lebensterben commented 2 years ago

@emacs18

before your PR, org-roam syncs the database in the after-init hook.


after your PR, with your new configuration, org-roam syncs the database after it's loaded. (because :config is evaluated after the package is loaded)

This works for you because spacemacs/user-config is evaluated after layers, but before packages are loaded.

I've no issue with adopting this. If you want you can make a PR.

albertfgu commented 1 year ago

Are there any updates to this? What's the proper way to call org-roam-db-autosync-mode? Especially if, like most users, I don't have a private layer.

Is there a way to do it when loading the layer, like

   dotspacemacs-configuration-layers
   '(
     (org :variables
          org-enable-roam-support t
          :post-config
          org-roam-db-autosync-mode)
    )

or is this not valid syntax?

Otherwise, do I need to use this type of block?

(spacemacs|use-package-add-hook org-roam
  :post-config
  ;; user configuration
  )

Should it be :post-config or :post-init or something else? Also, where does this entire block go? Does it go in the standard dotspacemacs/user-config where the rest of my config is, or somewhere else?

albertfgu commented 1 year ago

I've tried using

(spacemacs|use-package-add-hook org-roam
  :post-config
  ;; user configuration
  )

with both :post-config and :post-init, placing it in either the dotspacemacs/user-config or dotspacemacs/user-init functions.

I think it's supposed to be recommended that org-roam-db-autosync-mode is turned on, so having it be part of the org-roam layer by default or making it easier to configure would be great.

emacs18 commented 1 year ago

It has been too long so I don't remember the details. I just looked up my current setup which calls org-roam-db-autosync-mode within dotspacemacs/user-config function.

albertfgu commented 1 year ago

So you're no longer loading a separate init file that has use-package?

How do you call org-roam-db-autosync-mode within dotspacemacs/user-config? I tried just literally adding

(org-roam-db-autosync-mode)

as a line inside the function, but that broke a lot of stuff.

emacs18 commented 1 year ago

My init code is about 7,000 thousand lines in a file which is loaded within dotspacemacs/user-config. I don't have any explicit use-config, load, or require of org-roam. Also I use straight.el rather than package.el. I don't know whether this could change init code loading order.

emacs18 commented 1 year ago

I just tested a vanilla spacemacs with the following change to enable org layer and org-roam. Also (org-roam-db-autosync-mode) is called in dotspacemacs/user-config. This worked fine in that emacs started up without error and org-roam-db-autosync-mode was t.

diff --git a/core/templates/.spacemacs.template b/core/templates/.spacemacs.template
index 8abd9c0d9..00a240225 100644
--- a/core/templates/.spacemacs.template
+++ b/core/templates/.spacemacs.template
@@ -46,7 +46,10 @@ This function should only modify configuration layer settings."
      ;; lsp
      ;; markdown
      multiple-cursors
-     ;; org
+     (org :variables
+          org-directory dotspacemacs-directory
+          org-roam-directory dotspacemacs-directory
+          org-enable-roam-support t)
      ;; (shell :variables
      ;;        shell-default-height 30
      ;;        shell-default-position 'bottom)
@@ -568,6 +571,7 @@ This function is called at the very end of Spacemacs startup, after layer
 configuration.
 Put your configuration code here, except for variables that should be set
 before packages are loaded."
+  (org-roam-db-autosync-mode)
 )

In order to prevent emacs from loading any file outside of the spacemacs directory, I first applied the following patch which also used core/template/.spacemacs.template file as the init file.

commit 74d361e6727e4d3cd1a574536595447f2dbfc5a1 (spacemacsdir)
Author: Richard Kim <emacs18@gmail.com>
Date:   Sat Aug 26 16:28:08 2023 -0700

    set SPACEMACSDIR to use .spacemacs.template

diff --git a/.spacemacs.d/init.el b/.spacemacs.d/init.el
new file mode 120000
index 000000000..2813af572
--- /dev/null
+++ b/.spacemacs.d/init.el
@@ -0,0 +1 @@
+../core/templates/.spacemacs.template
\ No newline at end of file
diff --git a/early-init.el b/early-init.el
index 218328397..598bca7a0 100644
--- a/early-init.el
+++ b/early-init.el
@@ -35,6 +35,8 @@
 ;; needed nor loaded on those versions.
 (setq package-enable-at-startup nil)

+(setenv "SPACEMACSDIR" (expand-file-name ".spacemacs.d" (file-name-directory load-file-name)))
+
 (load (concat (file-name-directory load-file-name)
               "core/core-early-funcs")
       nil (not init-file-debug))
albertfgu commented 1 year ago

Thanks a lot for this. It works now, but only if I include these lines that you had:

(org :variables
         org-directory dotspacemacs-directory
         org-roam-directory dotspacemacs-directory
         org-enable-roam-support t)

Previously I was setting org-roam-directory using a setq inside dotspacemacs/user-config, which worked fine, but broke once (org-roam-db-autosync-mode) was called. I don't completely understand the internals of Spacemacs so I'm not really sure why it broke, but it makes sense.

I'm also curious how you found this solution. The Org layer documentation doesn't seem to mention the org-roam-directory variable. I don't think I could have figured this out by myself without a lot of pain.

emacs18 commented 1 year ago

spacemacs does not document every package. Instead it refers to the documentation for each package. For example org layer documentation simply refers to org-roam package documentation. org-roam documentation at https://www.orgroam.com/manual.html#Getting-Started says to set org-roam-directory very early on.