Fix dependency resolution for path dependencies

[Hackder]

Closes #2278
If a project foo had a path dependency of project bar, and the developer adds a new dependency to bar, foo will become unbuildable, until the manual deletion of the manifest.toml file.

During dependency change detection, also check recursively all path dependencies. This will observe any new dependencies added to any of the path deps.

The LSP problem:

• The fix proposed here will not cause the LSP to update automatically. It will need to be restarted after the dependency addition, to see the changes. I could have it watch gleam.toml for every path dependency, but I wanted some feedback before implementing it.

Also, I am not sure if there is an existing setup for tests, which could test this behavior. (Or even if we want to test it at all)

[lpil]

Thank you!

Could you expand on what the issue is with the language server in this case? Why is that it continues not to work after this change and what is the path to making it work?

[Hackder]

Of course. Suppose you have this project structure:

.
├── local_package
│   └── gleam.toml
└── main_project
    └── gleam.toml

The language server watches main_project/gleam.toml file (in supported editors). But if I add a package to local_package with gleam add gleam_erlang, the language server won't notice, and a restart of the language server is required.

A solution would be to also watch for changes to local_package/gleam.toml. Basically, watch all gleam.toml from all path dependencies recursively.

But I am also fine with restarting the LSP. I have never seen a language that does this, without also having proper support for monorepos/workspaces.

[lpil]

and a restart of the language server is required.

Why is a restart required? What happens if the language server is not required?

The language server uses the build tool, so doesn't it pick up the changes on the next compilation?

But I am also fine with restarting the LSP. I have never seen a language that does this, without also having proper support for monorepos/workspaces.

This is not an option, the LS supports monorepos and multiple packages.

[Hackder]

I just tested it with VSCode and Neovim.

I have three packages that depend on each other in a chain of first -> second -> third.
Each package imports a function from it's dependency. first.hello() calls second.hello() calls third.hello() returns 1.
I compile the first project and make sure the lsp works as expected.

Then I add gleam_erlang to third and make the hello function return process.ExitMessage(process.self(), process.Normal)

When I open the first.gleam file in neovim and VSCode, no changes are detected (LSP still reports fn hello() -> Int). After I build the first project (with my PR included it builds successfully) Neovim still doesn't pick up the changes. VSCode seems to be confused for a few seconds but then it works as expected.

However, when working in a single package, there is no need to perform a compilation. When I add a package, the LSP picks up the change and works as expected in both Neovim and VSCode.

I am not sure how exactly it works, I didn't explore the LSP code fully, but these were my observations.

[lpil]

That sounds like it does work. The language server doesn't automatically pick up changes, it only does anything when the editor tells it that there is a change and compilation is needed. VS Code tells the LS about changes to gleam.toml, while neovim does not.

If first.hello() is edited does it pick up the changes in the path dependencies and perform analysis as needed?

[Hackder]

Hmm. Yes, it does. I didn't think of testing this. When I edit first.hello() in Neovim (the editor was open the whole time) the changes are picked up.

So, if this is considered "correct behavior" is there anything more needed for this PR to be ready to merge? After rebasing ofcourse. Do we want to test this? And if so, could you give me directions because I didn't see a test infra setup for something like this. Thanks

[lpil]

Yup, that's the right behaviour!

I've just looked at the code and it looks like it is recording dependencies of path dependencies as being direct dependencies of the root package? Unless I'm misreading the code.

Instead you'll need to determine if any path dependencies have changes their requirements.

[Hackder]

The code should be doing this:
The compiler already checks if the manifest is up to date and present. When it does that, it checks if all the dependencies from gleam.toml are present in manifest.toml. If that is not the case, update the dependencies and manifest.

I simply collect all the dependencies from all path deps recursively, and check if all those are present in the manifest. That's it.

It doesn't store it anywhere. It is only used for that one check and then discarded. And because the manifest should contain all dependencies of all dependences recursively, this seemed to be a simple solution to the problem.

Please correct me if my assumptions were wrong.

[Hackder]

Could you please explain what problem my approach would cause? Or is it only about making it less "hacky" and making the code clearer? I can't seem to understand why doing this only in the if check (not storing this modified list anywhere) may cause issues.

[lpil]

I simply collect all the dependencies from all path deps recursively, and check if all those are present in the manifest. That's it.

It doesn't store it anywhere. It is only used for that one check and then discarded.

It's being added to the manifest in the section that lists the direct dependencies.

And because the manifest should contain all dependencies of all dependences recursively, this seemed to be a simple solution to the problem.

The manifest records them all in the packages section. You have added them to the requirements section, which the requirements of the root package. It is not a list of all the dependencies in the application.

Instead you'll need to determine if any path dependencies have changes their requirements. This could be done by storing the mtime and the checksum of their gleam.toml in the build directory.

[Hackder]

I have just ran through my "three package test" again (the one I mentioned earlier) and the packages are being added to the packages section of the manifest.toml. Could you please point out where I added them to the requirements?

This is the maniest:

# This file was generated by Gleam
# You typically do not need to edit this file

packages = [
  { name = "gleam_erlang", version = "0.25.0", build_tools = ["gleam"], requirements = ["gleam_stdlib"], otp_app = "gleam_erlang", source = "hex", outer_checksum = "054D571A7092D2A9727B3E5D183B7507DAB0DA41556EC9133606F09C15497373" },
  { name = "gleam_stdlib", version = "0.39.0", build_tools = ["gleam"], requirements = [], otp_app = "gleam_stdlib", source = "hex", outer_checksum = "2D7DE885A6EA7F1D5015D1698920C9BAF7241102836CE0C3837A4F160128A9C4" },
  { name = "gleeunit", version = "1.2.0", build_tools = ["gleam"], requirements = ["gleam_stdlib"], otp_app = "gleeunit", source = "hex", outer_checksum = "F7A7228925D3EE7D0813C922E062BFD6D7E9310F0BEE585D3A42F3307E3CFD13" },
  { name = "second", version = "1.0.0", build_tools = ["gleam"], requirements = ["gleam_stdlib", "third"], source = "local", path = "../second" },
  { name = "third", version = "1.0.0", build_tools = ["gleam"], requirements = ["gleam_erlang", "gleam_stdlib"], source = "local", path = "../third" },
]

[requirements]
gleam_stdlib = { version = ">= 0.34.0 and < 2.0.0" }
gleeunit = { version = ">= 1.0.0 and < 2.0.0" }
second = { path = "../second" }

I think this manifest is correct... I wouldn't want to overcomplicate the approach if this minor change is enough. But if you would prefer checking changes in all path deps, i could implement that as well. But I'm wary of proceeding if I don't understand where the issue is here.