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

Fix grammar verifcation #34994

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@dns2utf8
Copy link
Contributor

dns2utf8 commented Jul 23, 2016

I wanted to improve the grammar-section but I can not build it.

r? @steveklabnik @brson

EDIT: I resolved some of the issues, removed the compiler output from the entry.

@dns2utf8

This comment has been minimized.

Copy link
Contributor Author

dns2utf8 commented Jul 23, 2016

I was able to fix the errors, yay 😁 The API of libsyntax was changed (eg 24e7491) but the veryfy.rs is not getting build.

Should we include the antlr4-complete.jar or just build the verify.rs with the compiler? So further changes to the API will triger an error.

@dns2utf8 dns2utf8 changed the title Enhance the spelling of the grammar Fix grammar verifcation Jul 23, 2016

@@ -4,11 +4,11 @@ Uses [antlr4](http://www.antlr.org/) and a custom Rust tool to compare
ASTs/token streams generated. You can use the `check-lexer` make target to
run all of the available tests.

To use manually:
To use manually, assuming antler4 ist installed at `/usr/share/java/antlr-complete.jar`:

This comment has been minimized.

@dbrgn

dbrgn Jul 23, 2016

Contributor

This should probably say "antlr", not "antler".

This comment has been minimized.

@dns2utf8

dns2utf8 Jul 23, 2016

Author Contributor

Ah, yes thank you 👍

@dns2utf8

This comment has been minimized.

Copy link
Contributor Author

dns2utf8 commented Jul 23, 2016

I run the check-lexer target and the result was this:

test result: failed. 4764 passed; 125 failed; 2 skipped

I am curious, what are the next steps to fix the failed tests?

@dns2utf8 dns2utf8 force-pushed the dns2utf8:doc_grammar branch from a1c65dc to a2f27ec Jul 24, 2016

@@ -37,7 +37,7 @@ $(BG):

$(BG)RustLexer.class: $(BG) $(SG)RustLexer.g4
$(Q)$(CFG_ANTLR4) -o $(BG) $(SG)RustLexer.g4
$(Q)$(CFG_JAVAC) -d $(BG) $(BG)RustLexer.java
$(Q)$(CFG_JAVAC) -d $(BG) -classpath /usr/share/java/antlr-complete.jar $(BG)RustLexer.java

This comment has been minimized.

@steveklabnik

steveklabnik Jul 25, 2016

Member

hm, is this always going to be where this is? Feels system-specific

This comment has been minimized.

@dns2utf8

dns2utf8 Jul 28, 2016

Author Contributor

It is the place in Arch Linux, but not on Mac OS X. I have not tested other distros.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jul 25, 2016

@alexcrichton what do you think of this PR?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 25, 2016

It does seems system specific, perhaps that could be refactored? That being said I know very little about all of this, and we don't run it on the bots, so I don't mind merging basically whatever here.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 25, 2016

I'm cool with pushing these improvements through even with the hardcoded path. It's better than it was, and I'm glad somebody is looking at this stuff. Would be good to get this onto a bot #28592

@dns2utf8

This comment has been minimized.

Copy link
Contributor Author

dns2utf8 commented Jul 28, 2016

The system specific path could be obmittet with an environement variable, so the javac can find the antlr4-complete.jar but this would require the user to set it manually or the configure script to search for it.

Another alternative would be to include the jar file and just ship it. Is this a good idea?

cc: @nikomatsakis

@dns2utf8 dns2utf8 force-pushed the dns2utf8:doc_grammar branch 6 times, most recently from 10f7384 to e8f4f4f Jul 28, 2016

@dns2utf8

This comment has been minimized.

Copy link
Contributor Author

dns2utf8 commented Jul 29, 2016

I add the target check-build-lexer-verifier to make tidy, so it will build the verifier with every build and catch future changes in the API.
This does not require javac or antlr.

I think this is a good state freeze/merge for now. I planed to add some documumentation about the process in the next weeks, but I am no sure whether it will be in a good shape before the 18. august or not.

@dns2utf8 dns2utf8 force-pushed the dns2utf8:doc_grammar branch from e8f4f4f to 76026d1 Jul 29, 2016

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Aug 3, 2016

@bors: r+ rollup

:shipit: now, we can make it better eventually 😄

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 3, 2016

📌 Commit 76026d1 has been approved by steveklabnik

jonathandturner added a commit to jonathandturner/rust that referenced this pull request Aug 4, 2016

Rollup merge of rust-lang#34994 - dns2utf8:doc_grammar, r=steveklabnik
Fix grammar verifcation

I wanted to improve the [grammar-section](https://doc.rust-lang.org/grammar.html#items-and-attributes) but I can not build it.

r? @steveklabnik @brson

EDIT: I resolved some of the issues, removed the compiler output from the entry.

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Aug 4, 2016

Rollup merge of rust-lang#34994 - dns2utf8:doc_grammar, r=steveklabnik
Fix grammar verifcation

I wanted to improve the [grammar-section](https://doc.rust-lang.org/grammar.html#items-and-attributes) but I can not build it.

r? @steveklabnik @brson

EDIT: I resolved some of the issues, removed the compiler output from the entry.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 4, 2016

Rollup merge of rust-lang#34994 - dns2utf8:doc_grammar, r=steveklabnik
Fix grammar verifcation

I wanted to improve the [grammar-section](https://doc.rust-lang.org/grammar.html#items-and-attributes) but I can not build it.

r? @steveklabnik @brson

EDIT: I resolved some of the issues, removed the compiler output from the entry.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 4, 2016

Rollup merge of rust-lang#34994 - dns2utf8:doc_grammar, r=steveklabnik
Fix grammar verifcation

I wanted to improve the [grammar-section](https://doc.rust-lang.org/grammar.html#items-and-attributes) but I can not build it.

r? @steveklabnik @brson

EDIT: I resolved some of the issues, removed the compiler output from the entry.

bors added a commit that referenced this pull request Aug 4, 2016

mk/clean.mk Outdated
@@ -47,6 +47,10 @@ clean-misc:
$(Q)rm -Rf dist/*
$(Q)rm -Rf doc

clean-grammar:
@$(call E, cleaning grammar verification)
$(Q)cd src/grammar && rm -Rf verify *.class *.java *.tokens

This comment has been minimized.

@jonathandturner

jonathandturner Aug 4, 2016

Contributor

This does not handle an out-of-tree build directory, which our bots use.

cleaning grammar verification
/bin/sh: 1: cd: can't cd to src/grammar
/buildslave/rust-buildbot/slave/auto-linux-64-nopt-t/build/mk/clean.mk:51: recipe for target 'clean-grammar' failed
make: *** [clean-grammar] Error 2
program finished with exit code 2
elapsedTime=0.958165

This comment has been minimized.

@dns2utf8

dns2utf8 Aug 6, 2016

Author Contributor

Interesting, looking on it.

@jonathandturner

This comment has been minimized.

Copy link
Contributor

jonathandturner commented Aug 4, 2016

@bors r-

@dns2utf8 dns2utf8 force-pushed the dns2utf8:doc_grammar branch from 76026d1 to 4e9ea80 Aug 6, 2016

Fix grammar verification
 * Use make check-lexer to verify the grammar.
 * Extend grammar/README
 * Add make clean-grammar rule
 * Add target check-build-lexer-verifier to make tidy, so it will build the verifier with every build and catch future errors

@dns2utf8 dns2utf8 force-pushed the dns2utf8:doc_grammar branch from 4e9ea80 to 7ce7dd1 Aug 6, 2016

@dns2utf8

This comment has been minimized.

Copy link
Contributor Author

dns2utf8 commented Aug 6, 2016

I moved the in-tree part to the README of the manual process, since it was just for convenience.

I was looking for the structure of the build bot, can I run one locally?

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Aug 15, 2016

@alexcrichton ping re last comment here, can @dns2utf8 do something to test this out?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 15, 2016

We've got a number of docker images which our bots are running, so running the tests inside those containers would perhaps be enough to test out?

@dns2utf8

This comment has been minimized.

Copy link
Contributor Author

dns2utf8 commented Aug 15, 2016

I was able to start a container and access it with this command:

docker run --rm -it --entrypoint=/bin/bash alexcrichton/rust-slave-linux:2016-08-04

then inside clone my branch like the script:

git clone https://github.com/dns2utf8/rust.git --branch doc_grammar

I can then build my branch:

cd rust
./configure && make -j$(nproc) docs

but I am not sure this is the same as you do.
Am I doing it wrong? @jonathandturner

EDIT: I am doing this, because the setup-slave.sh uses some internal server which I can not reach so I am strongly guessing

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 15, 2016

@dns2utf8 yeah that's basically the same as what we do, this'd just be testing for presence of the relevant tools.

Is this running by default on make check though? I thought you had a run a custom check target?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 9, 2016

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

@dns2utf8

This comment has been minimized.

Copy link
Contributor Author

dns2utf8 commented Sep 17, 2016

Thanks for the reminder, I rebased it.

Can I reopen the issue just with a comment?

@dns2utf8 dns2utf8 referenced this pull request Nov 5, 2016

Merged

Fix grammar verification #37607

bors added a commit that referenced this pull request Nov 17, 2016

Auto merge of #37607 - dns2utf8:doc_grammar, r=alexcrichton
Fix grammar verification

 * Use make check-lexer to verify the grammar.
 * Extend grammar/README
 * Add make clean-grammar rule
 * Add target check-build-lexer-verifier to make tidy, so it will build the verifier with every build and catch future errors

This is the continuation of #34994

r? @steveklabnik @jonathandturner @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.