Buildbot grammar

[dns2utf8]

This is the implementation of #28592.

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[est31]

Dose it checks the commit messages too?

[dns2utf8]

I am sorry, I do not understand which checks do you mean?
As far as I can tell, the bots do not parse the commit messages.

[alexcrichton]

Thanks for the PR! In light of #37817 we're probably going to want to eventually migrate this to rustbuild at some point as well if we're interested in continually gating on it. For now I'll wait for this to go green and then looks good to me!

cc @brson, expanding our matrix of what's tested

[alexcrichton]

So thinking a bit about this bot, I wonder if we can perhaps try to speed up its execution time? We could take strategies like:

• Use a system LLVM instead of our bundled version
• Perhaps only compile the stage1 compiler, not all the way to stage2

Thoughts?

[dns2utf8]

Good news: it works 😄

For the speedup, I think using the system llvm would be perfect.

I am not sure about not building the compiler because if I had broken something inside the compiler, how would I know when the compiler used for the checkes is still a good one?

Should I disable the ALLOW_PR=1 flag again? Because right now the rust-part of the verifyer gets builded with make tidy so breaking changes against the infrastructure are detected.

Running the grammar checks on every commit of every PR might be overkill. If I understand it correctly the script runs all tests as soon as bors is attempting to merge everything on the auto branch, yes?

Before we merge this, I guess I should switch back the ALLOW_PR=1 to the llvm-3.7 build?

[alexcrichton]

Oh installing llvm-3.9 is fine actually, should even get us a bit more coverage. But yeah before merging we'd want to turn ALLOW_PR=1 as we in theory just want this to run for merges, not all PRs.

The verify.rs script which this runs only needs three crates, syntax, syntax_pos, and rustc. In that sense we only need to build those three crates, not the entire compiler in three stages? We can also skip make tidy if necessary.

[alexcrichton]

Hm so this line seems suspicious:

writing 7877 files that did not yield the correct result with /tmp/obj//grammar/parser-lalr to parser-lalr.bad

Does that mean that we're actually differing on a bunch of files? Does this mean that the verification doesn't generate an error if a difference is detected?

[shepmaster]

I might be missing the joke, but it should be spelled "grammar", not "grammer" in the PR title.

[dns2utf8]

I think the reason is bash:

if [ -z"42" ] ;thentrue;elsefalse;fi

this returns false but this:

if [ -z"42" ] ;thentrue;elif [ -z"42" ];thenfalseelsefalse;fi

does not

[dns2utf8]

I found the bug in the shell script:

if [ -z"42" ] ;thenecho 1;false;elif [ -z"42" ];thenecho 2;falsefi;elseecho 3;false;fi
^^^

this fi is required.
I do not know why bash behaves like this, but I think it will require a little more.

Fun fact, with this code there is no path to echo 2.5:

if [ -z"42" ] ;thenecho 1;false;elif [ -z"42" ];thenecho 2;falseelseecho 2.5;falsefi;elseecho 3;false;fi

Do you have an idea how to solve this problem?

[alexcrichton]

Ah I was wondering what was wrong with that! Nice find :)

Wanna switch back the ALLOW_PR=1 and we can r+?

[dns2utf8]

Allright I rebased to current master.

I reverted my changes to the .travis.yml script, because my bash handles elif diffrently from travises bash and it works as is: build with failing make check-grammar

With this rebase we are at 625 failing grammar tests.

Unforunalty the verify.rs is broken in the current master and my fix (#38029) did not merge yet.

That said I expect this PR to fail on the auto branch and, once it is merged, all following PRs until the grammar test suite is fixed. 😃

[alexcrichton]

Hm I'd prefer to not check in a matrix entry which fails tests. Could we set ALLOW_PR=1 temporarily and avoid merging until the tests are fixed on this PR? We could perhaps add whitelists with FIXMEs attached for test cases that need to be fixed.

[dns2utf8]

I added this output because the output file will be lost with the container.

[dns2utf8]

This works for me, but I may take some time and I would appreciate help with fixing the tests.

I guess in theory interestes people could also send me PRs with fixied tests directly and I could merge them into this PR.

[alexcrichton]

Oh yeah sure, we can definitely help out. Want to set ALLOW_PR=1 as part of this PR on that entry in the travis matrix, and then we can see the failures we're dealing with?

[dns2utf8]

Cool 👍

[dns2utf8]

I looked into some of the errors and it looks like some of them are the parser not understanding the new error format.

I tried to fix one, but I guess somebody with knowledge of bison could help.

[alexcrichton]

Hm this does raise an interesting question. If we enable and start gating on this then we're requiring that all changes and extensions to the grammar are implemented as well (or ignored). If we require changes, then this is essentially starting out RFC 1331. I'd just want to make sure that we're all on board with that and consciously making that step..

cc @rust-lang/compiler, this may make it slightly more difficult to extend the grammar in the future (the actual grammar has to be updated), but that seems like it may be a good thing. If that's true, is the current implementation of src/grammar something that we're willing to gate on?

[bors]

☔ The latest upstream changes (presumably #37817) made this pull request unmergeable. Please resolve the merge conflicts.

[dns2utf8]

The make check-grammar and make check-lexer targes are gone with the new build infrastructure.

Is there a replacement yet?
I think switching to the new infrastructure will speedup the process when using --stage 1 or higher.

[alexcrichton]

The old infrastructure still exists temporarily with --disable-rustbuild but otherwise yeah it'll need to be migrated into rustbuild.

[sanxiyn]

You may want to update Dockerfile as in #38632.

[bors]

☔ The latest upstream changes (presumably #38748) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase!