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

[WIP] Support multi-ipkg setup; Rework how we build the workspace project and when we compute metadata (TTM) #231

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Russoul
Copy link
Member

@Russoul Russoul commented Nov 27, 2024

TODO: Update the description

The main change is around loadURI. First we figure out which ipkg file to use (either one provided in LSP configuration by the client or we try to find one via idris2 api). The first option takes priority. Next we call an api function that builds the main module (including all dependencies) specified in the ipkg file. Rest is unchanged.

Another small change is addition of a new LSP configuration option, called "ipkgPath", which can be sent to the server from the client on initialisation and/or changed any time later. It represents optional relative path to the ipkg file. The root path is that where LSP server has been started.

Functionally, the PR makes it possible to switch/specify the ipkg file mid-air. Next time loadURI gets triggered the project will get rebuilt against the last specified ipkg file. Moreover, due to TTC cache, the switch may be close to instantaneous in case no single file needs a fresh build or a rebuild.

@Russoul
Copy link
Member Author

Russoul commented Nov 30, 2024

Currently there is one regression that needs to be attended to: when loading a file, the whole project (defined by ipkg) gets built (or loaded from cache). That is slower and less ergonomic, as error messages appearing in independent files will block syntax highlighting and interactive commands for the target file.

Copy link
Member

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't fully digested the changes but since my time is so sporadic right now I'll leave these comments now instead of holding them for later.

(Left (InternalError "Cannot find the .ipkg file"))
Right
<$>
[| gets LSPConf ipkgPath <+> coreLift (findIpkgFile <&> (<&> (\(dir, name, _) => dir </> name))) |]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line gets a bit terse IMO right around the time your anonymous function begins with <&>. I suppose this isn't a requested change unless you also are on the fence about it. I can follow, but I'm spending more effort reading the syntax than I usually like to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll fix

src/Server/ProcessMessage.idr Outdated Show resolved Hide resolved
@@ -212,43 +220,74 @@ loadURI conf uri version = do
let Just (startFolder, startFile) = splitParent fpath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startFolder and startFile are not used anymore. did you forget to remove their calculation from here or did you intend to use them below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, just forgot to remove them. If only we had a linter...

-- NOTE on catch: It seems the compiler sometimes throws errors instead
-- of accumulating them in the buildDeps.
unless (null errs) $ do
update LSPConf ({ errorFiles $= insert uri })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your code never sets errorFiles does it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, right. Probably removed unintentionally.

@mattpolzin mattpolzin self-requested a review November 30, 2024 23:54
| Left err => do let msg = "Cannot read file at \{show uri}"
logE Server msg
pure $ Left msg
update LSPConf ({ virtualDocuments $= insert uri (fromMaybe 0 version, res ++ "\n") })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code doesn't add the current file to the virtual documents.

@Russoul Russoul changed the title Support multi-ipkg setup [WIP] Support multi-ipkg setup; Rework how we build the workspace project and when we compute metadata (TTM) Dec 17, 2024
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.

2 participants