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

Always resolve files using Coq #684

Merged
merged 4 commits into from
Sep 24, 2024
Merged

Always resolve files using Coq #684

merged 4 commits into from
Sep 24, 2024

Conversation

rlepigre
Copy link
Contributor

@rlepigre rlepigre commented Sep 6, 2024

In this proposal, the resolution is always done in the following way:

  • If the give file path is absolute, keep it.
  • Remove leading coq:// if present, for backward compatibility. I don't think it is useful anymore.
  • Split the file name into two parts: a logical path (before the first /), and a remaining relative path.
    • Resolve the logical path using Coq, and turn it into a physical part.
    • Return the concatenation of the physical part and the relative path.

Example: if directory theory/dir is mapped to logical path my.project, then my.project/rest/of/path.elpi is mapped to theory/dir/rest_of_path.elpi. You get the same result if you use accumulate "my.project/rest/of/path". in an elpi file (note the absence of a .elpi extension in the string).

Note: this also removes the dune-workspace file, which I don't think should be there.

@gares
Copy link
Contributor

gares commented Sep 7, 2024

It makes sense, but the failure in HB seems to signals that this patch wants dune as a build system and dune needs to understand the extra sources.

@rlepigre
Copy link
Contributor Author

I'm not sure how this is specific to dune: independently of what build system you rely on, shouldn't the situation be the same? I mean, both make and dune rely on the compiler being passed -Q/-R, so the resolution should happen in the same way. Am I missing something here?

@rlepigre
Copy link
Contributor Author

After some investigation, it seems to me that the issue for HB is that the HB folder is not mapped. Adding HB/ to all the paths to elpi files seems to fix the issue. Another approach would probably be to change -Q . HB into -Q HB HB and to move structures.v into the HB folder.

@gares
Copy link
Contributor

gares commented Sep 10, 2024

ok, then there was an implicit ./ I guess... I'll look again at your code later today

@rlepigre
Copy link
Contributor Author

Cool, thanks. I pushed what I have for HB here: https://github.com/rlepigre/hierarchy-builder/tree/new_elpi_resolver.

@gares
Copy link
Contributor

gares commented Sep 10, 2024

Would you mind keeping a symlink to structures.v in the root for the sake of ci?

@rlepigre
Copy link
Contributor Author

Would you mind keeping a symlink to structures.v in the root for the sake of ci?

Sure, I pushed a fix to my HB branch.

@rlepigre
Copy link
Contributor Author

Another issue I have identified, but am not sure how to fix, is with the Elpi Print name "path/to/file". command. The involved code is here, and it basically treats "path/to/file" as a relative path from the current working directory. In a workspace build that cannot work, as coqc will be invoked from the workspace root, which is not necessarily the root of the coq-elpi repository. I'm not sure how to fix this in a satisfactory way though. Any idea @gares?

@rlepigre
Copy link
Contributor Author

I guess one approach would be to again rely on the Coq to resolve a directory path, given as first member of the path.

@gares
Copy link
Contributor

gares commented Sep 11, 2024

I guess one approach would be to again rely on the Coq to resolve a directory path, given as first member of the path.

That makes sense and also fix the ugly issue I have on proof general, since it runs coqtop from the directory where the .v file is, making Print behave differently depending your UI...

@rlepigre
Copy link
Contributor Author

I guess one approach would be to again rely on the Coq to resolve a directory path, given as first member of the path.

That makes sense and also fix the ugly issue I have on proof general, since it runs coqtop from the directory where the .v file is, making Print behave differently depending your UI...

This should now be fixed, but I had to make the file argument of Elpi Print mandatory, which seems reasonable to me. Something we could do alternatively, if it is not given, is write to a temporary file and report its name with a user message, but I don't know that it is worth it.

@rlepigre
Copy link
Contributor Author

@gares I confirmed that this MR makes it possible for us to use both elpi and coq-elpi as part of our dune workspace build, so we have started using it on our side. I'd be happy to push this MR through, but I'll need some guidance regarding what downstream projects need fixing, and how to test these fixes in CI. Do you use some kind of overlay mechanism like in the Coq repo?

@gares
Copy link
Contributor

gares commented Sep 20, 2024

no, I add a commit that thinkers with the nix configuration, and then remove it before merging.

About HB, I think some fixes were merged in nix upstream, so math-comp/hierarchy-builder#444 should work. I'll try updating it.

@gares gares force-pushed the resolver branch 2 times, most recently from 89d373e to 1d802c5 Compare September 20, 2024 12:43
src/dune Show resolved Hide resolved
@gares
Copy link
Contributor

gares commented Sep 20, 2024

I can merge it. I let you have a look at the minor points above and possibly add a line to the Changelog file

rlepigre and others added 3 commits September 24, 2024 09:27
This includes:
- Duplicating [elpi2html.elpi] and [elpi-quoted_syntax.elpi] form elpi.
- Always resolving file paths by treating the first component as a Coq
  directory path, and the rest as a relative path from its mapping.
- Require a file path argument in the [Elpi Print] command, resolved in
  the same way.
@rlepigre
Copy link
Contributor Author

Thanks @gares! I addressed the comments, and added a changelog entry.

@gares gares merged commit 62407f0 into LPCIC:master Sep 24, 2024
39 checks passed
@gares
Copy link
Contributor

gares commented Sep 24, 2024

Thanks!

@SkySkimmer SkySkimmer mentioned this pull request Sep 25, 2024
proux01 pushed a commit to rlepigre/coq that referenced this pull request Sep 26, 2024
rlepigre pushed a commit to rlepigre/coq that referenced this pull request Sep 26, 2024
@FissoreD
Copy link
Collaborator

Hello,
If I run make in master and try to interpret with vsCoq2 the following line:

Elpi Print foo "elpi.tests/test_link_order1".

I get this error message:

No loadpath found with logical name elpi.tests, cannot resolve file reference elpi.tests/test_link_order1.

And if I interpret this:

Elpi Print TC.Solver "elpi.apps.tc.examples/TC.Solver".

I get:

Multiple loadpaths found with logical name elpi.apps.tc.examples, while resolving file reference elpi.apps.tc.examples/TC.Solver:
- elpi.apps.tc.examples -> /home/dfissore/.../ce-master/apps/tc/examples
- elpi.apps.tc.examples -> /home/dfissore/.../ce-master/_build/default/apps/tc/examples

Am I missing something?

@gares
Copy link
Contributor

gares commented Oct 28, 2024

I suspect the _CoqProject file contains 2 mappings for elpi.apps.tc.examples.

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