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

straight not building magit-version.el #520

Closed
CeleritasCelery opened this issue May 5, 2020 · 18 comments
Closed

straight not building magit-version.el #520

CeleritasCelery opened this issue May 5, 2020 · 18 comments

Comments

@CeleritasCelery
Copy link
Contributor

What's wrong

as described in magit/magit#2070, the package manager should be building magit-version.el but straight is not. This is leading to the error

Cannot determine Magit’s version (error "c:/Users/tjhinckl/AppData/Roaming/.emacs.d/straight/build/magit/magit.el" repo static elpa 

Directions to reproduce

run (magit-version) after loading the package with straight.

Version information

  • Emacs version: 26.3
  • Operating system: Windows
@progfolio
Copy link
Contributor

related: #72

@raxod502
Copy link
Member

That doesn't sound correct to me. I installed Magit using package.el and I don't see any magit-version.el file.

@raxod502
Copy link
Member

Also, #72 isn't supported by package.el either. So that's definitely not it.

@progfolio
Copy link
Contributor

progfolio commented May 10, 2020

Perhaps I was mistaken. I didn't get to look into it too deeply, but I did notice that the file only exists as part of the make process and is removed if it still exists afterward by the magit-version function:

https://github.com/magit/magit/blob/16dd29206f678c79c02674d346ff727eb968ccc2/lisp/magit.el#L411-L423

Apologies if this is just noise.

@CeleritasCelery
Copy link
Contributor Author

Maybe @tarsius can explain how the package manager is supposed to build magit-version.el. Especially if package.el does not build it.

@tarsius
Copy link

tarsius commented May 10, 2020

Duplicate of #480.

That is labeled as external bug, which I don't really agree with.

In the ideal case we can determine the exact version in use:

$ git describe --tags --dirty 
v2.90.1-971-g9a388a61d

We cannot do that when not running Magit from its Git repository.

When we create a release tarball then we bundle a magit-version.el with that.
When we install using make in the Git repository then we also install a magit-version.el.

In a shallow clone Git isn't of any help:

$ git clone --depth 1 git@github.com:magit/magit.git
$ cd magit
$ git describe --tags --dirty 
fatal: No names found, cannot describe anything.

When installing using package.el then we cannot make it remember the precise version that it installs. So we just ask package.el what version string it has assigned to the version it has installed. That will be less precise than what we get with the methods above but its better than nothing.

It appears we have to do the same for straight.el (because it uses a shallow clone?). Please propose a patch to magit-version that gets this information from straight.el when that's actually what is being used.

@CeleritasCelery
Copy link
Contributor Author

It appears we have to do the same for straight.el (because it uses a shallow clone?)

Straight does not normally use a shallow clone. I think the issue is that the load path for magit points to the build directory which is not a git repo and only contains the byte-compiled code. Therefore getting the version from the tag doesn’t work.

If we could point the magit-version function to the the magit directory in straight/repos that would work.

@raxod502
Copy link
Member

Okay, I just tested and with the following init-file

(defvar bootstrap-version)
(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))

(straight-use-package 'magit)

the command M-x magit-version works just fine, with the following output:

Magit v2.90.1-972-g26da7efb, Git 2.26.1, Emacs 28.0.50, gnu/linux

So I suspect there is something else going on in your configuration @CeleritasCelery.

Now for shallow clones, I agree there's not much can be done on the side of Magit. However, I'm not quite sure @tarsius why the lack of version information should produce a warning at startup/byte-compilation. Why not just report "version info unavailable" or generate a warning at the time that magit-version is run?

I think the reason I marked #480 as external bug was because Magit was producing a warning while being byte-compiled, which I considered to be a bug. Not because it was failing to magically produce a version number out of nothing :)

@CeleritasCelery
Copy link
Contributor Author

the command M-x magit-version works just fine, with the following output:

Your minimal setup works on linux, but not on windows. I still get

Cannot determine Magit’s version (error "c:/Users/cel/AppData/Roaming/.emacs.d/straight/build/magit/magit.el" repo static elpa dirname)

@dinkonin
Copy link

Had the same problem with magit and (setq straight-vc-git-default-clone-depth 1) . Fixed it by specifying :straight (:depth full) in my use-package magit daclaration.

Same problem (and solution) with org, without a full clone I kept getting:

Error running timer ‘require’: (error "Invalid version syntax: ‘fatal: No names found, cannot describe anything.’ (must start with a number)")

@tarsius
Copy link

tarsius commented May 12, 2020

However, I'm not quite sure @tarsius why the lack of version information should produce a warning at startup/byte-compilation.

Well to be honest, I sometimes wonder too. The idea is that when a user is experiencing some (other) issue and it happens to be relevant what version they are using, then it would be nice not to have to first figure out why the version cannot be determined.

On the other hand we get quite a few issues about this harmless little warning, which is unfortunate considering it's supposed to reduce work.

Then again, every time we add some additional code to handle yet another method to install magit, we got that method going forward. So maybe it is a win after all.

@raxod502
Copy link
Member

Would you be okay with magit-version reporting shallow clone at revision <sha> in this case? If so, I could create a pull request.

@tarsius
Copy link

tarsius commented May 13, 2020

Just the hash would be better but make sure we only do that when straight was actually used to install magit.

@hlissner
Copy link

hlissner commented May 20, 2020

magit/magit@b1b2683 fixes the issue for macOS and Linux users, but not Windows users it seems. Presumably because straight doesn't (can't) use symlinks on Windows, so magit can't chase links in its load-file-name to find the root of its repo.

hlissner added a commit to doomemacs/doomemacs that referenced this issue May 20, 2020
Magit complains loudly (but harmlessly) when it can't determine its own
version in a sparse clone. This was fixed upstream in
magit/magit@b1b2683 for unix OSes, but not for Windows where symlinks
aren't supported, and so `magit-version` can't resolve its own repo's
root (see radian-software/straight.el#520).
@raxod502
Copy link
Member

That's really tragic.

@tarsius:

For context, straight.el normally symlinks package files from the repo into its build directory before compiling and adding to the load path. On Windows, there's no such thing as symlinks (for all practical purposes), so instead I just have it copy the files. Then for each file copied I also have it create a file in a parallel directory that contains the text of the path the symlink would be pointing to if we could use symlinks. Finally there is a straight-symlink-emulation-mode which adds to find-file-hook code which looks into this parallel directory and redirects you to the "target" of the "symlink" if you are looking at a file in the build directory. As a result of this weird arrangement, Magit can't find the actual repo.

Here is one way to solve the problem. I could add another case to magit-version that does the following:

  • check if straight-symlink-emulation-mode is defined and enabled
  • if so, check if the Magit library is inside straight.el's build directory (with guards in case the relevant variables are not defined)
  • if so, check the parallel file inside the symlink bookkeeping directory to see if it exists and contains a path
  • if so, resolve to that target before doing the rest of the logic

@tarsius
Copy link

tarsius commented May 25, 2020

I could add another case to magit-version that does the following:

I would merge that. 😁

@raxod502
Copy link
Member

Okay, I created the relevant pull request. This should solve the last remaining case for this ticket.

tarsius pushed a commit to raxod502/magit that referenced this issue Jun 11, 2020
Add a new function `magit--straight-chase-links' to do so.
Closes radian-software/straight.el#520.
tarsius pushed a commit to raxod502/magit that referenced this issue Jun 11, 2020
Add a new function `magit--straight-chase-links' to do so.
Closes radian-software/straight.el#520.
@tarsius
Copy link

tarsius commented Jul 22, 2020

Thanks for the pr.
I think you can close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants