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

File management and module mapping improvement #289

Merged
merged 20 commits into from
Mar 20, 2020

Conversation

rlepigre
Copy link
Contributor

@rlepigre rlepigre commented Nov 25, 2019

This PR adresses #243 and several other previous limitations.

Here is the rough idea of the new system (though the work is not finished):

  • There is a fixed "library root" directory that default to some reasonable place under the ~/.opam directory (or some system-wide directory if opam is not used).
  • The root directory can be altered using the --lib-root flag. This is important because the root directory should always be a valid directory, and that is typically not the case prior to installation. So we must now use something like dune exec -- lambdapi check --lib-root lib so that the lib directory in the source tree is considered the library root. Note that to recover the previous behavior we must run with . as the root.
  • There is the --map-dir option that can be used to map a module path to a specific directory (that may or may not be under the library root). For instance, we might do --map-dir matita:libraries/matita so that the files under the libraries/matita folder correspond to module paths starting with matita and then continuing with the underlying directory structure.
  • There is no more limitation to relative paths. There is now a piece of C code that calls realpath to get an absolute file name. Note that symbolic links are removed, so we cannot use a symbolic link to include a sub-tree under some mapped module path.
  • This PR subsumes Merge the LSP server into Lambdapi. #269, this change is useful for several good reasons. The main advantage is that the initialisation code is forced to be consistent, and so the server now knows about the path for the installed libraries.

Things that remain to be done:

  • More testing with corner cases.
  • Make sure that the libraries still compile, possibly cleaning up the scripts.
  • Create a minimal standard library under lib/std, lib being intended as the library root. As a consequence, the modules of the standard library would be accessed with std.bool for example (file lib/std/bool.lp). The lib directory should then be installed as the library root of the system.
  • Make sure that the LSP server still works (it is probably broken now since some initialization code is missing).
  • Fix the documentation and ensure that the editors work with the right binary. Now it needs to be something like lambdapi lsp --standard-lsp.
  • Write documentation for the module system and the intended installation procedure for third-party libraries.

@fblanqui
Copy link
Member

Thanks @rlepigre for your PR. I don't understand why you need a --lib-root option. It seems to me that the --map option subsumes it.

@rlepigre
Copy link
Contributor Author

rlepigre commented Nov 25, 2019

No, it is not subsumed because it would mean being able to map an "empty" module path, which is not possible. Plus, the library root has a very particular role: every installed library should be installed under it on the system.

src/files.ml Outdated Show resolved Hide resolved
src/files.ml Outdated Show resolved Hide resolved
@fblanqui
Copy link
Member

It's not clear to me. Please provide an example.

@rlepigre
Copy link
Contributor Author

It's not clear to me. Please provide an example.

So, let us assume that we have some library root directory <root>, and that we also have the following mappings:

  • foo.bar:<dir1> (i.e., module path foo.bar is mapped to directory <dir1>),
  • baz:<dir2> (i.e., module path baz is mapped to directory <dir2>).

Globally, this is represented by the following tree:

Some(<root>)
├─foo── None ──bar── Some(<dir1>)
└─baz── Some(<dir2>)

The search for files corresponding to different module paths would then yield:

  • foo.bar.baz.mod mapped to <dir1>/baz/mod,
  • a.b.c mapped to <root>/a/b/c,
  • foo.baz.mod mapped to <root>/foo/baz/mod.

There are several important things here:

  • This process does not fail as long as the root of the tree contains Some(_) (this requirement is explicitly stated in the actual lookup code). This is enforced when calling init_lib_root, which registers the root.
  • We fall back on the last Some(_) encountered during the tree traversal to pick a base directory, and we then complete it by taking the structure of the module path as the directory sub-structure. This is what happens with the foo.baz.mod: there is no branch with the baz label under first branch, so we fall back to the <root> directory.

@fblanqui
Copy link
Member

Why not taking the current directory as the root? I was personally thinking in something different:

lambdapi --map r1:d1 .. --map rk:dk file.lp

where ri is a non-qualified ident called a root and di is an arbitrary path

some roots could also be given through an environment variable $DK_PATH

there is also an implicit root r0 mapped to the current directory

then in file.lp we can have:

require a.b // refers to ./a/b.lp
require ri:a.b // refers to ri/a/b.lp

@rlepigre
Copy link
Contributor Author

The root is an actual root: it is the entry point of the module system. So it does not make sense to have the current directory be the root since installed libraries are not installed under the current directory. Of course, the current directory is special, and it should be mapped under a module path that is chosen for the project (maybe automatically by looking at the name of the current directory name, or using a meta data file). I insist that what I proposed here is still work in progress, so everything is not optimal for usability yet.

What you propose could maybe work, but I don't see that as a standard approach. (I did not really think your proposal through as much as I did mine, so it's hard for me to understand its limitations.) With what I propose, we will be prepared for having proper tooling (in the line of what Rust has with Cargo), and it also accounts for installation of libraries. Moreover, I don't really like adding yet another syntactic element (your roots). These are not useful since they can just be represented using a normal module path. The location of the corresponding files alone can tell whether a module path corresponds to a library or the a local module.

@fblanqui
Copy link
Member

Why do you want to map module paths to directories and not just roots?

@fblanqui
Copy link
Member

fblanqui commented Nov 26, 2019

How to handle manually installed libraries with your system, that is, not installed through a system like opam?

@rlepigre
Copy link
Contributor Author

Why do you want to map module paths to directories and not just roots?

I don't understand that question: what you call "roots" are directories right?

How to handle locally installed libraries with your system?

What do you mean by "locally installed libraries"?

@fblanqui
Copy link
Member

In a previous example, you proposed to map foo.bar to some directory. Why would you like to map foo.bar and not just foo?

@fblanqui
Copy link
Member

By locally installed, I mean "manually" installed, that is, not installed using a system like opam.

@rlepigre
Copy link
Contributor Author

rlepigre commented Nov 26, 2019

In a previous example, you proposed to map foo.bar to some directory. Why would you like to map foo.bar and not just foo?

Oh, I see. The reason is simple: we may want to have a more complex structure in our libraries that just a flat one. For example, we may want to have the following structure in our installed libraries:

  • std is the entry point of the standard library, with e.g., std.bool, std.nat and std.list modules.
  • std.list could contain some modules for algorithms on lists. For example, we could have modules std.list.sorting.qsort and std.list.sorting.msort for quick- and merge-sort.
  • imported containing imported libraries like imported.matita or imported.verine.

It just gives more options for placing things in relevant directories, and they don't have to be provided by the same package.

By locally installed, I mean "manually" installed, that is, not installed using a system like opam.

Ideally, it should be impossible to install a library "manually", otherwise you get the same mess as with OCaml. But in any case, the approach I propose enforces that libraries are installed under the system's library root. And there is no difference whether they are installed "manually" or automatically. It is just a convention that everyone must respect.

@fblanqui
Copy link
Member

Do you have an example of use of mapping std to dir1 and std.list to a directory that is not a sub-directory of dir1 (even transitively)?

@rlepigre
Copy link
Contributor Author

Do you have an example of use of mapping std to dir1 and std.list to a directory that is not a sub-directory of dir1 (even transitively)?

For std no, because I guess that the standard library would be a single package. But for imported, the example that I gave above makes sense.

@fblanqui
Copy link
Member

Isn't it a problem to enforce the use of a system like opam? What if the user wants to experiment with some libraries he modified and compiled somewhere in its home directory, for instance, in some git repos?

src/files.ml Outdated Show resolved Hide resolved
@rlepigre
Copy link
Contributor Author

Isn't it a problem to enforce the use of a system like opam? What if the user wants to experiment with some libraries he modified and compiled somewhere in its home directory, for instance, in some git repos?

Well, that's what the --map option is for!

In any case, enforcing the use of a proper tool (nothing like opam, which is crap) is great. It makes everything simpler and more uniform, and the user does not have to care about completely useless details. And also, everything is predictable.

src/console.ml Outdated Show resolved Hide resolved
src/files.ml Outdated Show resolved Hide resolved
src/files.ml Outdated Show resolved Hide resolved
@fblanqui
Copy link
Member

The update of doc/ is missing.

src/extra.ml Show resolved Hide resolved
src/files.ml Outdated Show resolved Hide resolved
@rlepigre rlepigre force-pushed the filepath branch 3 times, most recently from abbab7c to 2627025 Compare January 31, 2020 10:02
lib/std/bool.lp Outdated Show resolved Hide resolved
@fblanqui
Copy link
Member

Hi. Emacs doesn't seem to work with this PR. It would be good to check vscode as well.

@rlepigre
Copy link
Contributor Author

rlepigre commented Mar 20, 2020

Hi. Emacs doesn't seem to work with this PR. It would be good to check vscode as well.

It is supposed to work, @gabrielhdt tried it. Did you reinstall the Emacs mode?

For vscode I have no idea how to make the necessary adaptation. There is no documentation and I don't want to spend my time on that, especially since I do not use vscode.

@Rehan-MALAK
Copy link
Contributor

Rehan-MALAK commented Mar 20, 2020

Hi. Emacs doesn't seem to work with this PR. It would be good to check vscode as well.

It is supposed to work, @gabrielhdt tried it. Did you reinstall the Emacs mode?

For vscode I have no idea how to make the necessary adaptation. There is no documentation and I don't want to spend my time on that, especially since I do not use vscode.

It seems that pretty much every dedukti/lambdapi's users use emacs in one form or another (evil, dooms, spacemacs, vanilla, ...). I havn't use vscode for months (since I saw the lack of interest of the coq people for vscoq)
Both from a technical point of view and a strategical point of view, it's more interesting to concentrate our efforts on :

  • emacs > vscode
  • LSP server > LSP client

@gabrielhdt
Copy link
Member

I tested back the emacs mode, with a minimal configuration, and it seems that the proof mode does not work. The rest does.

To see it yourself,

wget https://sanemacs.com/sanemacs.el
echo '(use-package eglot)' >> sanemacs.el
emacs -q --no-site-file -l sanemacs.el -l <path/to/lambdapi.el>

and edit for instance natproofs.lp.

@fblanqui
Copy link
Member

I can now run emacs (I didn't install LP properly) but it doesn't print whether some thing is well type-checked or not.

@fblanqui
Copy link
Member

No, my fault. lambdapi.pkg is now mandatory even for a single file... Why not but, then, the error message is not very clear:

15:37 ~/lambdapi ((HEAD détachée sur rodolphe/filepath)) lambdapi check 303.lp 
[/home/blanqui/src/lambdapi/303.lp] cannot be placed under the library mapping.

Why not saying that there is no lambdapi.pkg file? Do you test this btw?

@rlepigre
Copy link
Contributor Author

Why not saying that there is no lambdapi.pkg file? Do you test this btw?

Yes, the lambdapi.pkg file is mandatory now. At least when you use the LSP server. Otherwise there is no way for the server where files should be placed under the library root.

I have given some thoughts as to what we could do to make things simpler, but there is no easy solution. Indeed, the error message is not great, I'll improve it.

@fblanqui
Copy link
Member

It's not just when you use the LSP server. It's all the time, even for checking a single file with no dependencies. It's not very convenient. Couldn't we use a default mapping in this case?

@rlepigre
Copy link
Contributor Author

It's not just when you use the LSP server. It's all the time, even for checking a single file with no dependencies. It's not very convenient. Couldn't we use a default mapping in this case?

I don't know, I don't have time to take care of that right now. Open an issue about that once this PR is merged and I'll try and find a way. However, I don't think this is going to be a big problem since the only thing you would need is to put a lambdapi.pkg file in your working directories. In fact, you could very well put one into your home directory, and this way all your "single files" would be type-checked no problem (assuming your directories have reasonable names, with no spaces for instance).

@fblanqui fblanqui merged commit 4da1632 into Deducteam:master Mar 20, 2020
@rlepigre rlepigre deleted the filepath branch March 20, 2020 22:45
ejgallego added a commit to ejgallego/lambdapi that referenced this pull request May 20, 2020
PR Deducteam#289 silently removed important unit tests in the Pure module.

We try to recover them, but indeed, they don't work, as it seems the
library handling API introduced in Deducteam#289 is lacking in terms of how it
can be used. There seem to be many problems with the new system in
terms of what a language server needs, in particular regarding
assumptions about the underlying file system.
fblanqui pushed a commit that referenced this pull request May 20, 2020
The PR #289 broke the vscode plugin.

We do a hotfix for now, but the mode still remains broken if using
from the dev tree [for example with `dune exec -- code`].

This is quite inconvenient, for now lambdapi must be fully installed
as to be used from vscode.
ejgallego added a commit to ejgallego/lambdapi that referenced this pull request May 27, 2020
PR Deducteam#289 silently removed important unit tests in the Pure module.

We try to recover them, but indeed, they don't work, as it seems the
library handling API introduced in Deducteam#289 is lacking in terms of how it
can be used. There seem to be many problems with the new system in
terms of what a language server needs, in particular regarding
assumptions about the underlying file system.
@maximedenes
Copy link

since I saw the lack of interest of the coq people for vscoq

FTR, I wonder on what kind of data this observation is based. Or maybe I misunderstood the claim.

@Rehan-MALAK
Copy link
Contributor

FTR, I wonder on what kind of data this observation is based. Or maybe I misunderstood the claim.

Hi @maximedenes
A bit of time spent on gitter vscoq some months ago and private conversations with coq intensive users (not especially coq developers).
But we could talk about this somewhere else. (I ping you privately on gitter vscoq)

ejgallego added a commit to ejgallego/lambdapi that referenced this pull request Nov 11, 2020
Fixes Deducteam#432 , introduced in Deducteam#289

This whole code needs more investigation as the logic seems quite
strange, in particular I think the core of LP should not mess with
filesystem low-level details, but hoping for a quick fix.
fblanqui pushed a commit that referenced this pull request Nov 12, 2020
Fixes #432 , introduced in #289

This whole code needs more investigation as the logic seems quite
strange, in particular I think the core of LP should not mess with
filesystem low-level details, but hoping for a quick fix.
ejgallego added a commit to ejgallego/lambdapi that referenced this pull request Oct 18, 2022
PR Deducteam#289 silently removed important unit tests in the Pure module.

We try to recover them, but indeed, they don't work, as it seems the
library handling API introduced in Deducteam#289 is lacking in terms of how it
can be used. There seem to be many problems with the new system in
terms of what a language server needs, in particular regarding
assumptions about the underlying file system.
ejgallego added a commit to ejgallego/lambdapi that referenced this pull request Oct 18, 2022
PR Deducteam#289 silently removed important unit tests in the Pure module.

We try to recover them, but indeed, they don't work, as it seems the
library handling API introduced in Deducteam#289 is lacking in terms of how it
can be used. There seem to be many problems with the new system in
terms of what a language server needs, in particular regarding
assumptions about the underlying file system.
ejgallego added a commit to ejgallego/lambdapi that referenced this pull request Oct 18, 2022
PR Deducteam#289 silently removed important unit tests in the Pure module.

We try to recover them, but indeed, they don't work, as it seems the
library handling API introduced in Deducteam#289 is lacking in terms of how it
can be used. There seem to be many problems with the new system in
terms of what a language server needs, in particular regarding
assumptions about the underlying file system.
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.

Too many warnings with libraries option --version is buggy expected relative path
6 participants