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

Replace ocamlyacc with cpspg #165

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

adampsz
Copy link

@adampsz adampsz commented Dec 10, 2024

This PR replaces ocamlyacc with fram-lang/cpspg@67dfc0f to check its compatibility and experiment with the new parser generator tool.

Building now requires cpspg to be in the path - it can be dune build and dune install-ed from adampsz/cpspg repo.

Copy link
Member

@ppolesiuk ppolesiuk left a comment

Choose a reason for hiding this comment

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

Thanks! This PR opens new perspectives for a better parser. However, I cannot merge it yet, since it breaks CI.

(ocamllex (modules Lexer))
;(ocamlyacc (modules YaccParser));
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this line instead of commenting it out? As discussed on the last meeting, we are going to use nice features of cpspg (which are not implemented yet), so I think in the near future, we will be incompatible with ocamlyacc.


(rule
(deps YaccParser.mly)
(target YaccParser.ml)
Copy link
Member

Choose a reason for hiding this comment

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

Does cpspg generate .mli files? If yes, maybe it should be added to this rule (I'm sure, dune can do that. You can ask @Foxinio, our local dune guru). If not, maybe we could consider writing own YaccParser.mli file.

Copy link
Author

Choose a reason for hiding this comment

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

It does not generate .mli files, but it should be straightforward to implement.

@Brychlikov
Copy link
Contributor

I got CI to work using nix here. I packaged both dbl and cpspg as flakes.
For now, I had to make two changes:

  • test.sh doesn't try to run dune build
  • changed its first parameter so it now should be something in PATH or be a path to dbl binary

flake.nix Outdated Show resolved Hide resolved
Co-authored-by: ilikeheaps <[email protected]>

export DBL_LIB="lib/"

TIMEOUT=1.0

binary="_build/default/src/$1.exe"
binary=$1
Copy link
Member

Choose a reason for hiding this comment

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

I see a minor problem with this change. When I run tests locally, I would like to use a version of dbl compiled from source on the active branch, not the one that is installed on my system.

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit of an oversight on my part - in my dev environment, dbl in PATH is the one from _build/.... To be fair, you still can run ./test.sh ./_build/default/src/dbl.exe ./test/test_suite to test against your freshly built dbl, but I understand that the default workflow should probably remain unchanged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I restored most of the original behaviour of test.sh in my branch - when passed a relative path as PROGRAM it does what it used to do.

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.

4 participants