8000 Add tokenizer_data_gen to build process by iluuu1994 · Pull Request #6723 · php/php-src · GitHub
[go: up one dir, main page]
Skip to content

Add tokenizer_data_gen to build process #6723

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add tokenizer_data_gen to build process #6723

wants to merge 1 commit into from

Conversation

iluuu1994
Copy link
Member
@iluuu1994 iluuu1994 commented Feb 23, 2021 •
edited
Loading

It happens on occasion that people forget to run tokenizer_data_gen.sh when updating the grammar. So this PR migrates the script to PHP and runs it automatically when zend_language_parser.y is changed.

Open questions

  • Currently I'm only looking for the PHP binary in the build directory, the stubs also look for a global PHP binary. Not sure if that's necessary.

php-src/build/Makefile.global

Line 152 in dda0cea

if test `php -v | head -n1 | cut -d" " -f 2 | sed "s/$$/\n7.0.99/" | sort -rV | head -n1` != "7.0.99"; then \

  • Should this be back-ported to PHP 7.3?

Advantages

  • ext/tokenizer/tokenizer_data.c should never be out of sync again
  • Runs on Windows

Disadvantages

  • Might break some build tools/environments
  • Unnecessary execution of the script when changing the parser in most cases

Feedback appreciated.

@iluuu1994 iluuu1994 force-pushed the tokenizer-data-gen-on-build branch from 85971d1 to 4b7510f Compare February 23, 2021 20:04
@codecov-io
Copy link

Codecov Report

Merging #6723 (dda0cea) into master (dda0cea) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6723   +/-   ##
=======================================
  Coverage   72.16%   72.16%           
=======================================
  Files         892      892           
  Lines      317896   317896           
=======================================
  Hits       229395   229395           
  Misses      88501    88501

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dda0cea...4b7510f. Read the comment docs.

@iluuu1994 iluuu1994 force-pushed the tokenizer-data-gen-on-build branch from 4b7510f to b424cd6 Compare February 25, 2021 22:31
@nikic
Copy link
Member
nikic commented Feb 25, 2021

I think we need to add detection of a PHP binary on the autoconf side. Using PHP_EXECUTABLE for this purpose doesn't seem right -- it might not yet exist, or be broken. We should be using system PHP, not the one we're building.

@iluuu1994
Copy link
Member Author

or be broken

That's a good point. I wonder if we should also rethink using the freshly built PHP binary for the stubs then.

@cmb69
Copy link
Member
cmb69 commented Mar 10, 2021

Should this be back-ported to PHP 7.3?

7.3 is in security mode, so no. Might still be good for PHP 7.4, though.

@iluuu1994
Copy link
Member Author

Thinking about it, it's probably unnecessary for 7.4/8.0 as patches will almost certainly not introduce changes to the grammar.

@nikic
Copy link
Member
8000 nikic commented Mar 16, 2021

Please rebase over fa9e2b3 and use $(PHP) instead of $(PHP_EXECUTABLE).

nikic
nikic reviewed Mar 16, 2021
View reviewed changes
ext/tokenizer/tokenizer_data_gen.php

Please, generate the PHP parser files by scripts/dev/genfiles
or by running the ./configure build step.
ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

Remove indentation here to keep this compatible with PHP < 7.3. We should probably bump our PHP version requirement, but this heredoc block probably isn't the right reason to do it :)

@php-pulls php-pulls closed this in 9bbeb05 Mar 16, 2021
@iluuu1994
Copy link
Member Author

Thanks Nikita!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

@cmb69 cmb69 cmb69 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iluuu1994 @codecov-io @nikic @cmb69
0