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

Nomanifest #37

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Nomanifest #37

wants to merge 7 commits into from

Conversation

chriselrod
Copy link
Contributor

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

@non-Jedi
Copy link
Owner

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

chriselrod commented Jan 18, 2023

My PR is currently incompatible with straight.el due to a Pkg bug, explained in JuliaLang/Pkg.jl#3326.

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.

3 participants