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

fixed bug in generating grammar script #5917

Merged
merged 1 commit into from
Jan 17, 2023
Merged

fixed bug in generating grammar script #5917

merged 1 commit into from
Jan 17, 2023

Conversation

ila
Copy link
Contributor

@ila ila commented Jan 16, 2023

obligatory premise: I have absolutely no idea what I am doing

I am no bison expert, but I am tasked to extend the parser for my phd stuff, and after I made my changes and tried to regenerate the grammar, I encountered a bug that also happens if I don't touch anything and just try to regenerate the existing grammar.

here's what happens:

[15:39:36] ila :: ila-XPS-9320  ➜  ~/Code/duckdb ‹master*› » python3 scripts/generate_grammar.py
bison -o third_party/libpg_query/grammar/grammar_out.cpp -d third_party/libpg_query/grammar/grammar.y.tmp
[15:50:49] ila :: ila-XPS-9320  ➜  ~/Code/duckdb ‹master*› » python3 scripts/generate_flex.py      

these are the instructions in the README. now, if I try to recompile duckdb with the new grammar cpp files, I get this error:

[176/205] Building CXX object third_party/libpg_query/CMakeFiles/duckdb_pg_query.dir/src_backend_parser_gram.cpp.o
FAILED: third_party/libpg_query/CMakeFiles/duckdb_pg_query.dir/src_backend_parser_gram.cpp.o 
/usr/bin/c++ -DDUCKDB_BUILD_LIBRARY -I/home/ila/Code/duckdb/src/include -I/home/ila/Code/duckdb/third_party/fsst -I/home/ila/Code/duckdb/third_party/fmt/include -I/home/ila/Code/duckdb/third_party/hyperloglog -I/home/ila/Code/duckdb/third_party/fastpforlib -I/home/ila/Code/duckdb/third_party/fast_float -I/home/ila/Code/duckdb/third_party/re2 -I/home/ila/Code/duckdb/third_party/miniz -I/home/ila/Code/duckdb/third_party/utf8proc/include -I/home/ila/Code/duckdb/third_party/miniparquet -I/home/ila/Code/duckdb/third_party/concurrentqueue -I/home/ila/Code/duckdb/third_party/pcg -I/home/ila/Code/duckdb/third_party/tdigest -I/home/ila/Code/duckdb/third_party/mbedtls/include -I/home/ila/Code/duckdb/third_party/jaro_winkler -I/home/ila/Code/duckdb/third_party/libpg_query/include -O3 -DNDEBUG -O3 -DNDEBUG   -fPIC -fvisibility=hidden -fdiagnostics-color=always -w -std=c++11 -MD -MT third_party/libpg_query/CMakeFiles/duckdb_pg_query.dir/src_backend_parser_gram.cpp.o -MF third_party/libpg_query/CMakeFiles/duckdb_pg_query.dir/src_backend_parser_gram.cpp.o.d -o third_party/libpg_query/CMakeFiles/duckdb_pg_query.dir/src_backend_parser_gram.cpp.o -c /home/ila/Code/duckdb/third_party/libpg_query/src_backend_parser_gram.cpp
third_party/libpg_query/grammar/grammar_out.cpp:265:10: fatal error: grammar_out.hpp: No such file or directory
compilation terminated.
[187/205] Building CXX object src/execution/expression_executor/CMakeFiles/duckdb_expression_executor.dir/ub_duckdb_expression_executor.cpp.o
ninja: build stopped: subcommand failed.

after investigating for a bit, I found that the python script to regenerate the grammar ultimately renames the header file generated with the -d flag in the filesystem, but does not rename the inclusion of this very same header in the src_backend_parser_gram.cpp file. therefore I opted for the quick and dirty fix in python.

as I said, I have no idea whether this is a me problem or whether this is the most efficient fix, but as always it works on my computer^tm

let me know if you have any thoughts and have a good day :)

@Tishj
Copy link
Contributor

Tishj commented Jan 16, 2023

Hmm I recall we use a specific version of flex and bison, maybe you have a different version? That might be the cause for the issues you're having

These are my versions:

➜  /usr/bin/flex --version 
flex 2.6.4 Apple(flex-34)
➜  /usr/bin/bison --version 
bison (GNU Bison) 2.3
Written by Robert Corbett and Richard Stallman.

Copyright (C) 2006 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

But if this fixes the issue and no longer requires a specific bison version, I see no problem with it :)

@ila
Copy link
Contributor Author

ila commented Jan 16, 2023

apparently bison 2.3 was released in 2006 and isn't even available in the ubuntu 22.04 package repository ^^ tried compiling from scratch but it gives me a bunch of errors, would be curious to see if anyone else encounters this error or if you're interested in a requirements bump :D

@Mytherin
Copy link
Collaborator

It's probably the last non-GPL version of it, which is why it's the default version included with MacOS :)

@ila
Copy link
Contributor Author

ila commented Jan 16, 2023

so you prefer something like this? (pseudocode)

if os == unix:
    [rename files]

would be interesting if someone with a macbook could test my code, just to know whether it would break everything with an older bison version (or maybe the CI/CD pipeline will take care of it, let's see)

@Mytherin
Copy link
Collaborator

It depends on the version of bison used - not the operating system. I can confirm this does fix compilation when using a later version of bison:

bison (GNU Bison) 3.8.2
Written by Robert Corbett and Richard Stallman.

Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

And it does not have any effect when using the earlier version of bison (as the include that is being replaced is not present in the file). Happy to merge this as-is to help development when using newer versions of bison.

@Mytherin Mytherin merged commit 4b2d12b into duckdb:master Jan 17, 2023
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.

None yet

3 participants