search

nwolverson / purescript-language-server

MIT License
184 stars 41 forks source link

Create outputDirectory if not existing #86

Closed andys8 closed 4 years ago

andys8 commented 4 years ago

If the outputDirectory is not existing on disk, it won't be created. It would be neat, if the the directory would be created and the build command would be triggered.

[Info  - 12:12:59 AM] Non-fatal error loading modules: Couldn't locate your output directory at: /home/andreas/dev/project/ls-output
[Info  - 12:12:59 AM] Output directory does not exist at '/home/andreas/dev/project/ls-output'
[Error  - 12:13:21 AM] Error message from IDE server reloading modules: Couldn't locate your output directory at: /home/andreas/dev/project/ls-output
kritzcreek commented 4 years ago

Just creating an empty output directory would start the server with no module information. A missing output directory means that either the server is started in the wrong directory or no full build has taken place yet, both of which mean the server can't be functional after it's started.

andys8 commented 4 years ago

I think what purs-loader does, if you configure an output directory, is creating the directory, and build its content.

It would be nice if purescript-language-server would behave similar.

kritzcreek commented 4 years ago

I think what purs-loader does, if you configure an output directory, is creating the directory, and build its content

How does it do that? Does it own the build process? The LS doesn't. It works on projects built with spago, pulp, nix, or your own custom solution so building the whole project is just not something it knowns how to do.

andys8 commented 4 years ago

I'm with you that it might not be the responsibility of the language server. But I'm somewhat confused, because code looks like building the project is something it can do.

building the whole project is just not something it knowns how to do.

image

https://github.com/nwolverson/purescript-language-server/blob/5b77635ac9dfbeced72c779d14628674395a23df/src/LanguageServer/IdePurescript/Main.purs#L335-L339

https://github.com/nwolverson/purescript-language-server/blob/63d2c9160da2f6c71f7cf3a061df720b46a21a49/src/IdePurescript/Build.purs#L56-L89

kritzcreek commented 4 years ago

But I'm somewhat confused, because code looks like building the project is something it can do.

Oh! I thought that would be in the editor specific plugins. Allright let's think about this again then... I fear for the UX here, but let's think about a couple of scenarios

  1. Running the build command might fail because there's an actual error in the project

In that situation you sometimes still want to start the LS because it can use the information of all the modules that did compile to help you fix these errors. But sometimes this is an indication of a bigger problem like version mismatches that you really want the user to know about and fix before continuing. So we'd need to be very smart or very chatty to not lead users astray here.

  1. Running the build without prompting the user on startup might fail because the LS guessed the build command wrong

How do we tell his apart from 1.? Horror scenario would be to guess the build command wrong but still run to completion and get the wrong output, but that seems very unlikely at least.

  1. Running the build command takes a long time.

It's not uncommon for a completely fresh clone of a large PS project to take a minute or two to compile from scratch (especially if you're also downloading the packages). Will this make editors assume the language server timed out? This would probably need a progress bar/indicator, but I don't think spago makes it easy to extract that progress (doesn't have an API for programs calling it).

This is what I can come up with right now... the problems all seem solvable but sound like some thought and care is needed to get this right.

nwolverson commented 4 years ago

Unless I'm mistaken, @andys8 you've just indicated that when you start without output directory existing, the option to build project is presented?

That seems like an ideal solution, for the reasons that @kritzcreek gave I wouldn't want to trigger this automatically - some of the time this will be the situation for setup reasons rather than just needing to build, and the build command may not be correctly configured - but here we give the user information, and make it easy to build and proceed. I'm concerned that firing this off automatically will just lead to less obvious support issues if anything.

Is the issue that this functionality didn't work for you? It sounds that you were unaware of it in the original issue description. I certainly wouldn't be surprised many LSP clients do not implement the action button functionality on warning messages.

andys8 commented 4 years ago

I try to improve the quality of my comments regarding this issue. Initially, I tried to set the outputDirectory and noted, it's just not working. I went with a different way to solve this for my project, but wanted to create an issue, so this is tracked. The initial issue was missing details. I also think it's less critical than e.g. #85 where code actions are not working in Vim.

Starting with more clarification:

I initially thought, the build is not triggered/working at all. But (see 2.) it is. The language server / my configuration wasn't using the correct outputDirectory in the build command. I guess I didn't notice, because the output (wrong directory) was still existing in my past tests.

Tests

1. No language server config

Clean repo with no output folder

[Info  - 6:15:07 PM] Starting IDE server on port 15721 with cwd /home/andreas/dev/ebot7/conversation-ide-poc
Started IDE server (port 15721)
[Info  - 6:15:07 PM] Your output directory didn't exist. This usually means you didn't compile your project yet.
psc-ide needs you to compile your project (for example by running pulp build)
[Info  - 6:15:07 PM] Non-fatal error loading modules: Couldn't locate your output directory at: /home/andreas/dev/ebot7/conversation-ide-poc/output
[Info  - 6:15:07 PM] Output directory does not exist at '/home/andreas/dev/ebot7/conversation-ide-poc/output'
[Info  - 6:15:14 PM] Resolved build command (1st is used): 
[Info  - 6:15:14 PM] /home/andreas/.npm-global/bin/spago: 0.16.0
[Info  - 6:15:14 PM] Running build command: spago build --purs-args --json-errors
...
Compiling Main
...
[Info  - 6:15:39 PM] [info] Build succeeded.
[Info  - 6:15:39 PM] Build complete

2. Configured outputDirectory

Clean repo with no output folder

"outputDirectory": "other-directory/"

3. Trying to configure buildCommand

[WIP] With the results of 2, I think the cause is likely that the buildCommand needs to be adapted.

"outputDirectory": "other-directory/",
"buildCommand": "spago build --purs-args \"-o other-directory --json-errors\""

I must be doing something wrong with escaping here.

[Info  - 6:34:33 PM] Running build command: spago build --purs-args ""-o other-directory --json-errors""
[Info  - 6:34:33 PM] Invalid argument `other-directory'

So my current understanding is, that the language server implementation would work, if the buildCommand would be adapted correctly.

nwolverson commented 4 years ago

See #64, the build command parsing is stupid

andys8 commented 4 years ago

I gave this another shot today. Based on this workaround, it is actually possible to configure the language server to use a different directory, and prompt and build are working absolutely fine!

The configuration has to look like this to avoid spaces at all costs:

"outputDirectory": "other-directory/",
"buildCommand": "spago build --purs-args=\"--json-errors\" --purs-args=\"-oother-directory\""

64 already addresses improving parsing purs-args.

Regarding this issue, a possible improvement could be: If we're using spago, and an outputDirectory is configured, but no buildCommand is configured (or in both cases), the argument to change the output folder (-o<folder or --output <folder>) is added to the build command by the language server.

With that change, this configuration would work out of the box.

"outputDirectory": "other-directory/",
nwolverson commented 4 years ago

I'm fairly opposed to mangling the configured build command - I'm keen that the default setup "just works", but otherwise when you stray from that (ie custom output directory) I think we start down a path that ends at removing the custom build command, and directly calling purs to build with appropriate args, source globs, etc.

There's definitely a documentation issue, probably the docs on purescript.outputDirectory should note the build command should be altered appropriately.

andys8 commented 4 years ago

Makes sense.

The description of the option does actually hint in that direction.

https://github.com/nwolverson/vscode-ide-purescript/blob/21269fbfaa3fd377db54db39ef0c8c81600658a4/package.json#L220

I think the actual issue was #64 that made it hard to configure. Maybe the docs for configuration could be improved by having examples in the README and maybe a table with options in the README itself. It could be generated from the JSON definition.

Other than that I'm going to close the issue.