search

non-Jedi / eglot-jl

Wrapper for using Julia LanguageServer.jl with emacs eglot
Creative Commons Zero v1.0 Universal
62 stars 11 forks source link

Nomanifest #37

Open chriselrod opened 1 year ago

chriselrod commented 1 year ago

straight keeps asking about keeping/discarding changes when I update. Adding the manifest to .gitignore should solve this problem.

non-Jedi commented 1 year ago

Including the Manifest was an explicit design decision for this package. The idea was that if somebody reported an issue, I'd know exactly which package versions they were running which would hopefully make reproduction easier. In practice because the Manifest encodes the Julia version, I agree that this isn't really working.

I think I agree with this PR in principle, but we should simplify the stuff put into eglot-jl.jl at the same time if we're doing this. Could you comment on why your commits went back and forth between resolve and instantiate? It's been long enough since I touched anything even semi-complex involving Pkg that I can't remember the intricacies of the differences.

ffevotte commented 1 year ago

I think this has to do with #20. We used to simply Pkg.instantiate the project, but this caused various problems when the Manifest format changed around Julia 1.5-1.6. So #21 started to Pkg.resolve the project in order to make sure the manifest was compatible with the julia version used (but still use the shipped version numbers if possible).

@chriselrod What's exactly the problem with Manifest.toml being versioned?

If we don't ship any manifest, I think lines 9-20 of eglot-jl.jl could be removed. @non-Jedi is that what you had in mind? As mentioned in the discussion on #21, incompatibilities between eglot-jl and LanguageServer.jl could then be handled via tight compat bounds in Project.toml (but that could reduce the usefulness of #33).

chriselrod commented 1 year ago

What's exactly the problem with Manifest.toml being versioned?

The instantiate modifies the Manifest.toml, so every time I update packages, straight asks about these changes.

Could you comment on why your commits went back and forth between resolve and instantiate? It's been long enough since I touched anything even semi-complex involving Pkg that I can't remember the intricacies of the differences.

I am not sure why that ended up getting committed these changes. But this PR is/was fairly broken, at least for me.

Pkg.instantiate() would work, and I added Pkg.status(io=stderr) which would show LanguageServer and SymbolServer. Yet, the using LanguageServer, SymbolServer would error. I believe I also tried commenting out all the extra path modifications, but that didn't work either. Running or including the script manually, it would work. Just not when started by eglot-jl.el.

As a lazy workaround, I added LanguageServer and SymbolServer to my Julia version environment (i.e., v1.10). The LOAD_PATH handling here was meant to make it so that it only looks at this Project and not at the version environment. So I thought something was going wrong there, if it was only looking at v1.10 instead of this project.

So I (or someone else) would have to go back and look at the load path to make sure it's as expected/confirm we don't see these errors before we consider merging this.

chriselrod commented 1 year ago

My PR is currently incompatible with straight.el due to a Pkg bug, explained in https://github.com/JuliaLang/Pkg.jl/issues/3326.