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

Add coq.env.add-context for inserting context declarations #737

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

VojtechStep
Copy link

There was no way to add implicit section variables, so context-decl objects could not be faithfully spliced from outside code.

I'd appreciate a naming suggestion for what I temporarily called coq.env.add-section-variable-two.

Copy link
Contributor

@gares gares left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good.

@gares
Copy link
Contributor

gares commented Jan 6, 2025

I would be tempted to break the existing api, giving a default value to the argument when omitted.

Surely HB will need adjustment.
Would you mind trying and see what CI does discover?

@VojtechStep
Copy link
Author

Would you mind trying and see what CI does discover?

I can try tomorrow.

@gares
Copy link
Contributor

gares commented Jan 7, 2025

For the failure in master (nix) CC @CohenCyril

@gares
Copy link
Contributor

gares commented Jan 7, 2025

Also, I forgot: here an example of an argument with a default (MP):

MLCode(Pred("coq.env.dependencies",
In(gref, "GR",
In(B.unspec modpath, "MP",
Out(gref_set, "Deps",
Read(unit_ctx, "Computes the direct dependencies of GR. If MP is given, Deps only contains grefs from that module")))),
(fun root inside _ ~depth _ _ state ->
let inside = unspec2opt inside in
!: (dep1 ?inside (get_sigma state) root))),
DocAbove);

@CohenCyril
Copy link
Collaborator

CohenCyril commented Jan 7, 2025

I would be tempted to break the existing api, giving a default value to the argument when omitted.

How about using an optional

  • @implicit! => coq.env.add-section-variable Name Ty C
  • @maximal! => coq.env.add-section-variable Name Ty C
  • @explicit! => coq.env.add-section-variable Name Ty C

to preserve backward compat?

@gares
Copy link
Contributor

gares commented Jan 7, 2025

That is also an option, but I'm not super fan of adding an attribute that is used only by one API.

As expected HB failed. @VojtechStep do you need guidance on how to have CI use a patched branch of HB?

@VojtechStep
Copy link
Author

I wanted to make my local checkout of HB use this branch of coq-elpi, but I'm struggling to get it set up. I tried overriding HB's bundles."coq-8.20".coqPackages.coq-elpi.override.version with "#737", "/absolute/path/string/to/local/coq-elpi", ../nix/path/object/to/local/coq-elpi and import ../coq-elpi {}, but no luck so far.
The first and third ones fail with unbound symbols (e.g. Unbound module Elpi.API.Ast.Scope in File "src/coq_elpi_HOAS.mli", line 160, characters 14-41).
The second with "do not know how to unpack source archive /absolute/path/..." (kind of as expected).
The fourth one fails with "function default-fetcher called without required argument rev" in build-support/coq/meta-fetch/default.nix.

Any help would be appreciated.

@VojtechStep
Copy link
Author

I also tried adding bundles."coq-8.20".ocamlPackages.elpi.override.version = "2.0.6";, which didn't have any effect — even elpi --version in a new nix-shell still gives 1.19.2.

@gares
Copy link
Contributor

gares commented Jan 7, 2025

I also tried adding bundles."coq-8.20".ocamlPackages.elpi.override.version = "2.0.6";, which didn't have any effect — even elpi --version in a new nix-shell still gives 1.19.2.

This is a known issue, but I don't know the solution while @CohenCyril does

@CohenCyril
Copy link
Collaborator

CohenCyril commented Jan 7, 2025

Ah yeah, I did a stupid mistake somewhere and we keep bumping into it...
The override for elpi is in the derivation of coq-elpi (instead of the general scope of coq packages 😕 ) see e.g. this commit to see where it is overriden f51d2c5

so sorry about that.

@VojtechStep
Copy link
Author

No worries, I figured it out in the meantime 👌

@gares
Copy link
Contributor

gares commented Jan 7, 2025

It seems HB is the only package impacted by the change, so I think this patch is fine.
I'll try to see how to make HB compile with 2.3.0 and also with this change.

@gares gares force-pushed the feature/add-context branch from dd8e5e8 to 657278d Compare January 13, 2025 13:19
@gares
Copy link
Contributor

gares commented Jan 13, 2025

waiting ocaml/opam-repository#27258

@gares gares force-pushed the feature/add-context branch from 657278d to 4532ebf Compare January 13, 2025 14:51
@gares
Copy link
Contributor

gares commented Jan 13, 2025

I tried to make nix build against the upstream PR and tag but I failed.
I guess I'll wait for a proper release...

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