happy-frontend: check in generated code

int-index commented 1 month ago

This drastically simplifies the bootstrapping mechanism. If you have happy pre-installed, you can run:

packages/frontend/bootstrap.sh

to regenerate Parser.hs and AttrGrammar/Parser.hs

Ericson2314 commented 1 month ago

I would rather not do this. With @sheaf's Setup.hs replacement work around the corner, we should finally have a good solution for using the bootstrap code path as originally envisioned.

sgraf812 commented 1 month ago

Thanks a bunch for hacking this together on such short notice, Vlad!!

[x] I tried cabal build in a fresh checkout and it worked
[x] I tried happy_datadir="." cabal build happy-frontend in a fresh checkout and it worked as well
[x] I deleted packages/frontend/.../Parser.hs, ran packages/frontend/bootstrap.sh and verified that the file (generated by happy-1.20) is the original checked-in one
[ ] I think the CI files still need to remove the bootstrap flag

Documentation is crisp and correct as well. So I'd be fine with merging this patch as-is.

I wonder though whether we should add a test that the checked-in parser is actually up-to-date. For this check we would need to decide whether we want to assume a fixed, released bootstrap version of happy (currently happy-1.20) or whether we want to bootstrap with master (which actually yields a different Parser.hs). This would have the additional benefit of having it tested in CI.

I hacked a test script together for generating the parser with master, so it currently fails:

diff --git a/tests/Makefile b/tests/Makefile
index 7cc71c3..ea839d5 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -46,6 +46,8 @@ TESTS = Test.ly TestMulti.ly TestPrecedence.ly bug001.ly \

 ERROR_TESTS = error001.y

+BOOTSTRAP_TESTS = Parser.ly AttrGrammarParser.ly
+
 # NOTE: `cabal` will set the `happy_datadir` env-var accordingly before invoking the test-suite
 #TEST_HAPPY_OPTS = --strict --template=..
 TEST_HAPPY_OPTS = --strict
@@ -94,6 +96,8 @@ ALL_TESTS = $(patsubst %.hs, %.run, $(ALL_TEST_HS))

 CHECK_ERROR_TESTS = $(patsubst %, check.%, $(ERROR_TESTS))

+CHECK_BOOTSTRAP_TESTS = $(patsubst %, boot.%, $(BOOTSTRAP_TESTS))
+
 HC_OPTS += -fforce-recomp

 .PRECIOUS: %.hs %.o %.bin %.$(HS_PROG_EXT)
@@ -109,10 +113,16 @@ check.%.y : %.y
    @diff -u --ignore-all-space $*.stdout $*.run.stdout
    @diff -u --ignore-all-space $*.stderr $*.run.stderr

+boot.%.ly : ../packages/frontend/boot-src/%.ly
+   @rm -f $*.hs
+   @echo "--> Checking bootstrap for $<..."
+   $(HAPPY) -agc $< -o $*.g.hs 1>$*.boot.run.stdout 2>$*.boot.run.stderr
+   @diff -u --ignore-all-space ../packages/frontend/src/Happy/Frontend/$*.hs $*.g.hs
+
 %$(HS_PROG_EXT) : %.hs
    $(HC) $(HC_OPTS) $($*_LD_OPTS) $< -o $@

-all :: $(CHECK_ERROR_TESTS) $(ALL_TESTS)
+all :: $(CHECK_BOOTSTRAP_TESTS) $(CHECK_ERROR_TESTS) $(ALL_TESTS)

 check-todo::
    $(HAPPY) $(TEST_HAPPY_OPTS) -ad Test.ly

Feel free to take it (or leave it).

sgraf812 commented 1 month ago

I would rather not do this. With @sheaf's Setup.hs replacement work around the corner, we should finally have a good solution for using the bootstrap code path as originally envisioned.

The way I see it: happy master has not had a working release in nearly 3 years. This patch fixes master and removes a lot of complicated code in favor of a simple, non-build-tool-dependent mechanism. We should just merge it.

When @sheaf's Setup.hs replacement work sees the day and fixes the issues in https://github.com/haskell/happy/issues/274#issuecomment-2166631843 (which are ultimately all symptoms of https://github.com/haskell/cabal/issues/10072, IMO), then you can just revert this patch and commit the Setup.hs replacement in a PR that can be reviewed and judged for its cost/benefit ratio separately.

Ericson2314 commented 1 month ago

We should be able to have an old-style build without deleting any code? I.e. we vendor the happy output but don't delete the bootstraping parser combinators code. Then both ways can be built without bootstrapping --- one has vendored code and the other no vendored code but fewer features.

int-index commented 1 month ago

Then both ways can be built without bootstrapping --- one has vendored code and the other no vendored code but fewer features.

Why do we need both options? The intent behind the parser combinators version was to enable building happy without requiring a pre-built happy binary. Checking in generated code achieves the same. So it's just code duplication at that point.

sgraf812 commented 1 month ago

We should be able to have an old-style build without deleting any code?

What precisely do you mean by that? Do note that with this PR, I can simply type cabal build happy, or cabal install happy, in contrast to what is committed on master. Isn't that an "old-style build"?

Then both ways can be built without bootstrapping

What is the added benefit of the parser combinator version? I certainly don't see one. I see a number of costs, though: (1) the parser combinator version will fall out of date, (2) the additional flag needs documentation for when it is actually useful, (3) it looks as if happy cannot even bootstrap itself and needs a parser combinator library to do so.

As far as I'm concerned, I would be very happy to see this patch merged.

sgraf812 commented 3 weeks ago

Alright, let's merge this one as well. We can always test for idempotent bootstrap later.

Ericson2314 commented 3 weeks ago

@int-index I just wanted to ensure the parser combinators didn't rot in the meantime, for when the new Setup.hs stuff is ready. But I guess we fetch it out of history then...

haskell / happy

happy-frontend: check in generated code #277