Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sayid-version for native-comp #59

Closed
wants to merge 3 commits into from

Conversation

nivekuil
Copy link

@nivekuil nivekuil commented Sep 7, 2020

My load-file-name documentation says:

In case a .eln file is being loaded this is unreliable and ‘load-true-file-name’
should be used instead.

Indeed this seems to be always nil with native comp enabled.

https://github.com/emacs-mirror/emacs/blob/feature/native-comp/src/lread.c#L5037

load-true-file-name doesn't exist in non-native comp emacs so there needs to be some special logic for both unfortunately. If boundp is hacky comp-native-compiling might work too.

My `load-file-name` documentation says:

In case a .eln file is being loaded this is unreliable and ‘load-true-file-name’
should be used instead.

Indeed this seems to be always nil with native comp enabled.
`lm-version` already does this:

"Return the version listed in file FILE, or current buffer if FILE is nil."
@@ -49,8 +49,9 @@

(defconst sayid-version
(eval-when-compile
(lm-version (or load-file-name buffer-file-name)))
"The current version of `clojure-mode'.")
(lm-version (if (boundp 'load-true-file-name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add some comment here explain the need for this, as it's very far from obvious. :-)

@nivekuil nivekuil marked this pull request as draft September 9, 2020 13:40
@nivekuil
Copy link
Author

nivekuil commented Sep 9, 2020

turns out this isn't the problem; sayid-version is only set every time the current emacs session compiled sayid.el, so if it's restarted it will just load the .eln and sayid-version will be nil in the new session.

@bbatsov
Copy link
Member

bbatsov commented Sep 9, 2020

Hmm, that sucks. We can add some fallback on a hardcoded version, but this will kind of defeat the purpose of retrieving the version dynamically.

@anonimitoraf
Copy link

Any other workarounds for this?

@carrete
Copy link

carrete commented Jan 4, 2021

diff --git a/src/el/sayid.el b/src/el/sayid.el
index 9806c92..6daea46 100644
--- a/src/el/sayid.el
+++ b/src/el/sayid.el
@@ -47,11 +47,15 @@
   :group 'sayid
   :type 'boolean)
 
-(defconst sayid-version
-  (eval-when-compile
-    (lm-version (or load-file-name buffer-file-name)))
-  "The current version of `clojure-mode'.")
+(defun sayid-version-read ()
+  "The current version of `sayid.el'."
+  (lm-version (or load-file-name buffer-file-name)))
 
+(defconst sayid-version (eval-when-compile (sayid-version-read)))
+
+(defun sayid-version-fn ()
+  "Return cached version or re-read version."
+  (or sayid-version (sayid-version-read)))
 
 (defface sayid-int-face '((t :inherit default))
   "Sayid integer face"
@@ -152,7 +156,7 @@ If injecting the dependencies is not preferred set `sayid-inject-dependencies-at
   (when (and sayid-inject-dependencies-at-jack-in
              (boundp 'cider-jack-in-lein-plugins)
              (boundp 'cider-jack-in-nrepl-middlewares))
-    (add-to-list 'cider-jack-in-lein-plugins `("com.billpiel/sayid" ,sayid-version))
+    (add-to-list 'cider-jack-in-lein-plugins `("com.billpiel/sayid" ,(sayid-version-fn)))
     (add-to-list 'cider-jack-in-nrepl-middlewares "com.billpiel.sayid.nrepl-middleware/wrap-sayid")))
 
 ;;;###autoload
@@ -166,7 +170,7 @@ If injecting the dependencies is not preferred set `sayid-inject-dependencies-at
   (message "clj=%s el=%s"
            (sayid-req-get-value
             (list "op" "sayid-version"))
-           sayid-version))
+           (sayid-version-fn)))
 
 (defun sayid-select-default-buf ()
   "Select sayid default buffer."

I'm not using native compilation. I'm not sure why I have this problem, but sayid-version is always nil when I run cider-jack-in. If relevant, I've used both use-package.el and straight.el (current) to install sayid, always same behavior. I don't know that this is the best solution. This is just a patch I quickly came up with that doesn't use a hard-coded version string.

@anonimitoraf
Copy link

@carrete What do those changes do exactly?

@carrete
Copy link

carrete commented Jan 5, 2021

@anonimitoraf I've added a function that will recompute the version when the version constant is nil at runtime. I suspect the problem is with eval-when-compile. These changes address the symptom, not the cause. Perhaps lexical binding would help?

@bbatsov
Copy link
Member

bbatsov commented Jan 5, 2021

I'm starting to think we should hardcode it again. :D I never thought such a simple change could have such ramifications.

@bbatsov
Copy link
Member

bbatsov commented Dec 29, 2021

Turns out the problem is that if load-file-name is not the .el file it's not possible for lm-version to find anything. I'll just go back to how things were for simplicity's sake. Thanks to everyone who tried to help with this!

@bbatsov bbatsov closed this Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants