hi,
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hi!
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?
I think we should, otherwise building from release package without re2c
would be impossible, and I think before it was possible. In any case, we
should be consistent - if we commit generated files (which we do), we
should do it everywhere.
--
Stas Malyshev
smalyshev@gmail.com
Hey:
hi,
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?
I think we should, and also use a better name...(.tab.c?)
thanks
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
--
--
Xinchen Hui
@Laruence
http://www.laruence.com/
Hey:
hi,
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?I think we should, and also use a better name...(.tab.c?)
that too :)
--
Pierre
@pierrejoye | http://www.libgd.org
Hey:
Hey:
hi,
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?I think we should, and also use a better name...(.tab.c?)
that too :)
simply, json_parser.c is good
something like: https://gist.github.com/laruence/f33903266cec737088aa
thanks
--
Pierre@pierrejoye | http://www.libgd.org
--
Xinchen Hui
@Laruence
http://www.laruence.com/
Hi,
Hey:
Hey:
On Mon, Feb 9, 2015 at 10:40 AM, Pierre Joye pierre.php@gmail.com
wrote:hi,
Should we push json_parser.tab.c? Which is generated (re2c), just
like what is done in date or other?I think we should, and also use a better name...(.tab.c?)
that too :)
simply, json_parser.c is goodsomething like: https://gist.github.com/laruence/f33903266cec737088aa
I'd be for applying this as well. Also, just added the generation calls to
config.w32, but actulaly a checked in file were much simpler to handle.
Regards
Anatol
Hi,
Hey:
On Mon, Feb 9, 2015 at 1:44 PM, Pierre Joye pierre.php@gmail.com
wrote:Hey:
On Mon, Feb 9, 2015 at 10:40 AM, Pierre Joye pierre.php@gmail.com
wrote:hi,
Should we push json_parser.tab.c? Which is generated (re2c), just
like what is done in date or other?I think we should, and also use a better name...(.tab.c?)
that too :)
simply, json_parser.c is goodsomething like: https://gist.github.com/laruence/f33903266cec737088aa
I'd be for applying this as well. Also, just added the generation calls to
config.w32, but actulaly a checked in file were much simpler to handle.Regards
Anatol
Hi,
The re2c generated files are pushed (see
https://github.com/php/php-src/blob/master/ext/json/json_scanner.c ). The
json_parser.tab.c is a BISON generated file. I haven't pushed that file
because we don't have bison generated files in repo (e.g.
zend_language_parser.c). I'm actually for pushing that file as well but
then we should probably push zend_language_parser.c too. Thoughts?
The name .tab.c is a standard name for bison files ( see last paragraph
http://www.gnu.org/software/bison/manual/html_node/Rpcalc-Generate.html#Rpcalc-Generate
). I think that it can be changed but not sure if it makes much sense. The
.tab.c says that it's a C file with parsing tables...
Cheers
Jakub
On Mon, Feb 9, 2015 at 9:18 AM, Anatol Belski anatol.php@belski.net
wrote:Hi,
Hey:
On Mon, Feb 9, 2015 at 1:44 PM, Pierre Joye pierre.php@gmail.com
wrote:Hey:
On Mon, Feb 9, 2015 at 10:40 AM, Pierre Joye pierre.php@gmail.com
wrote:hi,
Should we push json_parser.tab.c? Which is generated (re2c), just
like what is done in date or other?I think we should, and also use a better name...(.tab.c?)
that too :)
simply, json_parser.c is goodsomething like: https://gist.github.com/laruence/f33903266cec737088aa
I'd be for applying this as well. Also, just added the generation calls
to
config.w32, but actulaly a checked in file were much simpler to handle.Regards
Anatol
Hi,
The re2c generated files are pushed (see
https://github.com/php/php-src/blob/master/ext/json/json_scanner.c ). The
json_parser.tab.c is a BISON generated file. I haven't pushed that file
because we don't have bison generated files in repo (e.g.
zend_language_parser.c). I'm actually for pushing that file as well but
then we should probably push zend_language_parser.c too. Thoughts?The name .tab.c is a standard name for bison files ( see last paragraph
http://www.gnu.org/software/bison/manual/html_node/Rpcalc-Generate.html#Rpcalc-Generate
). I think that it can be changed but not sure if it makes much sense. The
.tab.c says that it's a C file with parsing tables...
I still think it is better to have this file applied. It is not like they
will change every 2nd day.
Cheers
Jakub
I still think it is better to have this file applied. It is not like they
will change every 2nd day.
Hi,
I think that I might have solution for this and also for my another build
problem which is re2c version.
I have just created a PR where I tried to explain everything:
https://github.com/php/php-src/pull/1072
Basically ti adds all sources but allows to regenerate them only if the
options --enable-json-filegen is set (more details are in the description).
The config changes are just for Linux as I'm not sure how to do it on win
and if it's even needed (if someone will send any patches to json scanner
or parser and needs to generate them automatically).
I'll keep the PR open for a week or so and if there are no objection, I
would like merge it. I'm also open changing the name of the option and will
be glad for any config.m4 related comments and reviews.
Cheers
Jakub
Hey:
I still think it is better to have this file applied. It is not like they
will change every 2nd day.Hi,
I think that I might have solution for this and also for my another build
problem which is re2c version.I have just created a PR where I tried to explain everything:
https://github.com/php/php-src/pull/1072
Basically ti adds all sources but allows to regenerate them only if the
options --enable-json-filegen is set (more details are in the description).The config changes are just for Linux as I'm not sure how to do it on win
and if it's even needed (if someone will send any patches to json scanner or
parser and needs to generate them automatically).I'll keep the PR open for a week or so and if there are no objection, I
would like merge it. I'm also open changing the name of the option and will
be glad for any config.m4 related comments and reviews.
why make this thing in this way complicated? why not just simply
include the generated files as others did?
thanks
Cheers
Jakub
--
Xinchen Hui
@Laruence
http://www.laruence.com/
why make this thing in this way complicated? why not just simply
include the generated files as others did?
There are cases where it is configure dependent so not including them
makes sense.
I do not recognize this case with json.
--
Pierre
@pierrejoye | http://www.libgd.org
Hey
why make this thing in this way complicated? why not just simply
include the generated files as others did?
First of all the others did exactly what I have already done. :) It means
they included re2c generated files and did not include bison generated
files.
There are few reasons why I would like to do it slightly differently in
json and make the re-generation optional.
-
I want to prevent an accidental re-generation of files with the old
version of re2c and bison. It happens to me from time to time when I switch
branches and build often. Then I see that I have changes (git diff) in the
zend_language_scanner.c and other re2c generated files even if I haven't
edited them. The reason is because the version that I use is different and
the files got regenerated (you won't see that if you use the same version).
That could be even bigger problem with bison where the the version
differences are much more often. -
As soon as I start using new features (like utf8 ranges in re2c) that
are not available in the old version and someone would try to do some
changes in the scanner and regenerate it, then it would fail with error. I
believe that it's better if the user gets a warning during the
configuration and the compilation exits the build instead of generating an
error.
3 It is sort of a signal which version is currently supported and it
hopefully prevents things like regenerating files with the old version and
pushing it to the repo (as Anatol did yesterday in
https://github.com/php/php-src/commit/89893541305594eba373c15156eb54d7a7cb5960
).
As I said in the PR. This change won't affect anyone who doesn't change
json scanner and parser which is 99% of all users that build PHP. The 1% is
probably just me so it's not really complicated IMHO...
I would also like to add all of this to the json README (it's very outdated
and I plan to update it anyway) and possibly other build doc if we have any.
Cheers
Jakub
Hi Jakub,
Hey
why make this thing in this way complicated? why not just simply include
the generated files as others did?First of all the others did exactly what I have already done. :) It means
they included re2c generated files and did not include bison generated
files.There are few reasons why I would like to do it slightly differently in
json and make the re-generation optional.
I want to prevent an accidental re-generation of files with the old
version of re2c and bison. It happens to me from time to time when I
switch branches and build often. Then I see that I have changes (git diff)
in the zend_language_scanner.c and other re2c generated files even if I
haven't edited them. The reason is because the version that I use is
different and the files got regenerated (you won't see that if you use the
same version). That could be even bigger problem with bison where the the
version differences are much more often.As soon as I start using new features (like utf8 ranges in re2c) that
are not available in the old version and someone would try to do some
changes in the scanner and regenerate it, then it would fail with error.
I
believe that it's better if the user gets a warning during the
configuration and the compilation exits the build instead of generating
an error.3 It is sort of a signal which version is currently supported and it
hopefully prevents things like regenerating files with the old version and
pushing it to the repo (as Anatol did yesterday in
https://github.com/php/php-src/commit/89893541305594eba373c15156eb54d7a7c
b5960 ).
It's actually so that there is a range of tool versions supported. That
means any version within the range should be valid for use. If another
range of versions is defined, so it's obviously to follow it. It's good to
make thoughts about the new features, but that'll be probably require
updating the guidelines and changing the valid versions range.
As I said in the PR. This change won't affect anyone who doesn't change
json scanner and parser which is 99% of all users that build PHP. The 1%
is probably just me so it's not really complicated IMHO...I would also like to add all of this to the json README (it's very
outdated and I plan to update it anyway) and possibly other build doc if
we have any.
Yeah, so why don't just push file and have it done? ATM there might be
some fixes needed, so regeneration required. Later on it's supposed to be
a very rare case - just in case of bugs or new development. Btw also
people who don't have a required re2c/bison/etc version for whatever
reasons would still be able to compile, test or develop other parts.
Regards
Anatol
Hi Anatol,
On Tue, Feb 10, 2015 at 12:24 PM, Anatol Belski anatol.php@belski.net
wrote:
It's actually so that there is a range of tool versions supported. That
means any version within the range should be valid for use. If another
range of versions is defined, so it's obviously to follow it. It's good to
make thoughts about the new features, but that'll be probably require
updating the guidelines and changing the valid versions range.
First of all it's great that you are adding the the type and cross platform
fixes into json. I understand why you needed to regenerate the file.
As you say there is a supported range. The problem is that the range is too
high which is very limiting for adding new features at the moment. What I
don't want to do is to force all users compile the new tool version from
source because distros are still outdated . There also is no need to
require the new version for the whole PHP if it's needed just for json
generation.
As I said in the PR. This change won't affect anyone who doesn't change
json scanner and parser which is 99% of all users that build PHP. The 1%
is probably just me so it's not really complicated IMHO...I would also like to add all of this to the json README (it's very
outdated and I plan to update it anyway) and possibly other build doc if
we have any.Yeah, so why don't just push file and have it done? ATM there might be
some fixes needed, so regeneration required. Later on it's supposed to be
a very rare case - just in case of bugs or new development. Btw also
people who don't have a required re2c/bison/etc version for whatever
reasons would still be able to compile, test or develop other parts.
That's not actually completely true on linux. At least not on my platform.
I can recreate this on my system when I do following:
- install a different version of re2c then the
one Zend/zend_language_scanner.c (e.g. 0.13.6) - git checkout master
- ./configure ... && make
- checkout PHP-5.6
- checkout master
- make
- git status ( it shows Zend/zend_language_scanner.c as regenerated using
0.13.6 )
The fact that I checked out the 5.6 [4] (it doesn't have to be just 5.6 but
some other branch with more changes) and then back master [5] caused
regeneration zend_language_scanner.c in [6]. The point is that as soon as
you start changing branches often, it gets regenerated even if you don't
change it. And then you need to have the required version because otherwise
the build fails.
I want to prevent that for json generated files. This would be especially
visible for bison where the version differs more between users compare to
re2c where the most users are on 0.13.5. That's exactly what the PR
(config.m4 changes) does.
Cheers
Jakub
Hi Anatol,
On Tue, Feb 10, 2015 at 12:24 PM, Anatol Belski anatol.php@belski.net
wrote:It's actually so that there is a range of tool versions supported. That
means any version within the range should be valid for use. If another
range of versions is defined, so it's obviously to follow it. It's
good to make thoughts about the new features, but that'll be probably
require updating the guidelines and changing the valid versions range.First of all it's great that you are adding the the type and cross
platform fixes into json. I understand why you needed to regenerate the
file.
Well, thank you for your work )
As you say there is a supported range. The problem is that the range is
too high which is very limiting for adding new features at the moment.
What I
don't want to do is to force all users compile the new tool version from
source because distros are still outdated . There also is no need to
require the new version for the whole PHP if it's needed just for json
generation.
IMHO that's exactly what is going to happen - users willing to regenerate
the parser will have to get eventually another re2c version if they don't
have it. As for me - I've just compiled re2c 0.13.6 for windows. But in
general - using different tools for different parts of the core were
probably a mess.
As I said in the PR. This change won't affect anyone who doesn't
change json scanner and parser which is 99% of all users that build
PHP. The 1%
is probably just me so it's not really complicated IMHO...I would also like to add all of this to the json README (it's very
outdated and I plan to update it anyway) and possibly other build doc
if we have any.Yeah, so why don't just push file and have it done? ATM there might be
some fixes needed, so regeneration required. Later on it's supposed to
be a very rare case - just in case of bugs or new development. Btw also
people who don't have a required re2c/bison/etc version for whatever
reasons would still be able to compile, test or develop other parts.That's not actually completely true on linux. At least not on my
platform. I can recreate this on my system when I do following:
- install a different version of re2c then the
one Zend/zend_language_scanner.c (e.g. 0.13.6) 2. git checkout master- ./configure ... && make
- checkout PHP-5.6
- checkout master
- make
- git status ( it shows Zend/zend_language_scanner.c as regenerated using
0.13.6 )The fact that I checked out the 5.6 [4] (it doesn't have to be just 5.6
but some other branch with more changes) and then back master [5] caused
regeneration zend_language_scanner.c in [6]. The point is that as soon as
you start changing branches often, it gets regenerated even if you don't
change it. And then you need to have the required version because
otherwise the build fails.I want to prevent that for json generated files. This would be especially
visible for bison where the version differs more between users compare
to re2c where the most users are on 0.13.5. That's exactly what the PR
(config.m4 changes) does.
As the versions in the range are interchangeable, it gives much more
flexibility. Right now, if you open some release tarball - it is possible
you see files generated with different bison/re2c versions because it
doesn't matter much.
Maybe it'd be worth it to move one step after another, see what features
can be implemented and how do they improve? Maybe they'll be so crucial to
even make a special case that one can say - yeah, don't care about
flexibility? Not that one would restrict something without having the bare
reason in the hand.
Regards
Anatol
Hi Anatol,
On Tue, Feb 10, 2015 at 3:35 PM, Anatol Belski anatol.php@belski.net
wrote:
Maybe it'd be worth it to move one step after another, see what features
can be implemented and how do they improve? Maybe they'll be so crucial to
even make a special case that one can say - yeah, don't care about
flexibility? Not that one would restrict something without having the bare
reason in the hand.
I have been thinking about it and think that you have got a point. Let's
wait when the implementation is ready and then we can make a decision if
it's worthy breaking the flexibility. I have got few other things on my
list before that so it will probably take some time to do the changes and
test it...
I would like to push the the bison tab files shortly as the majority of
people in this thread (including me) are for having them in the repo. The
only thing that I would like is to have a specific version in the repo to
prevent big diffs for small changes in the source files. For now I would
like to have there re2c 0.13.6 (thanks for regenerating it back ;) ) and
bison 2.7.1 gen files. I will update Readme at some point as well and add
there that info.
I think that that all compat are types fixes are sort of done so it would
be probably best to do PR for all json changes as it has lots of advantages
(I'm sure that you all know them so I won't name them... :) )
I won't be adding the config.m4 stuff that is in the PR though. I realized
that it would a bit of hack just for json which probably not good. However
I still like the idea of removing re2c and bison dependency for all users
that don't need to change parsers or scanners. That will require more
changes to the build and probably RFC so it's a bit off-topic. I'll see if
I get time to propose something like that in the future.
Cheers
Jakub
I would like to push the the bison tab files shortly as the majority of
people in this thread (including me) are for having them in the repo. The
only thing that I would like is to have a specific version in the repo to
prevent big diffs for small changes in the source files. For now I would
like to have there re2c 0.13.6 (thanks for regenerating it back ;) ) and
bison 2.7.1 gen files. I will update Readme at some point as well and add
there that info.
Hey just a quick update. I bumped the version in the repo for re2c 0.13.7.5
(latest - no changes in generated states ) and bison to 3.0.4 . I updated
Readme as well.
I have pushed the bison files in
http://git.php.net/?p=php-src.git;a=commit;h=911f7b10b1f4c9529bc01580d298a93a5cd6bbd2
. There is an explanation why the C preprocessor guard macro names are file
system dependent. It means why there is
YY_PHP_JSON_YY_HOME_JAKUB_PROG_PHP_MASTER_EXT_JSON_JSON_PARSER_TAB_H_INCLUDED .
It's due to bison algorithm for creating such name. As I noted the only
solution that works for me is using different yacc.c skeleton. I have done
it in jsond in
https://github.com/bukka/php-jsond/commit/583619d7962fa57bf97dcdac4147d8b599a42672
where I have optional bison generation which means that I can stick with
one bison version and use custom skeleton file. This is however not
possible in the core where we allow range of bison versions which doesn't
play well with skeletons that are version specific.
When I committed the changes, something went wrong with pushing to github.
I hope that my commit hasn't screwed anything up. There is some permission
problem probably:
Total 5 (delta 3), reused 0 (delta 0)
remote: Welcome bukka.
remote: Changesets accepted. Thank you for your contribution.
remote:
remote: Attempting to push to mirror git@github.com:php/php-src.git
remote: Permission denied (publickey).
remote: fatal: Could not read from remote repository.
remote:
remote: Please make sure you have the correct access rights
remote: and the repository exists.
To git@git.php.net:php-src.git
549c6fa..5f82c92 master -> master
Cheers
Jakub
Hi Jakub,
I would like to push the the bison tab files shortly as the majority of
people in this thread (including me) are for having them in the repo.
The
only thing that I would like is to have a specific version in the repo
to prevent big diffs for small changes in the source files. For now I
would like to have there re2c 0.13.6 (thanks for regenerating it back ;)
) and
bison 2.7.1 gen files. I will update Readme at some point as well and
add there that info.Hey just a quick update. I bumped the version in the repo for re2c
0.13.7.5
(latest - no changes in generated states ) and bison to 3.0.4 . I updated
Readme as well.I have pushed the bison files in
http://git.php.net/?p=php-src.git;a=commit;h=911f7b10b1f4c9529bc01580d298a
93a5cd6bbd2
. There is an explanation why the C preprocessor guard macro names are
file system dependent. It means why there is
YY_PHP_JSON_YY_HOME_JAKUB_PROG_PHP_MASTER_EXT_JSON_JSON_PARSER_TAB_H_INCL
UDED
.
It's due to bison algorithm for creating such name. As I noted the only
solution that works for me is using different yacc.c skeleton. I have done
it in jsond in
https://github.com/bukka/php-jsond/commit/583619d7962fa57bf97dcdac4147d8b
599a42672
where I have optional bison generation which means that I can stick with
one bison version and use custom skeleton file. This is however not
possible in the core where we allow range of bison versions which doesn't
play well with skeletons that are version specific.
thanks for pushing. I'm using re2c 0.13.7.5 now for master as well. With
bison, it'll be however hard to move from 2.4.1 on Windows (and it's not
that critical), but file generated with it seems to work. Anyway, nothing
prevents you or anyone to regenerate it and push over, just in case. Most
important is that fixes land in the .re/.y sources. And one knows who to
ping for verifications :)
Regards
Anatol
Hi Jakub,
Hi Jakub,
I would like to push the the bison tab files shortly as the majority
of people in this thread (including me) are for having them in the
repo. The
only thing that I would like is to have a specific version in the repo
to prevent big diffs for small changes in the source files. For now
I
would like to have there re2c 0.13.6 (thanks for regenerating it back
;)
) and
bison 2.7.1 gen files. I will update Readme at some point as well and
add there that info.Hey just a quick update. I bumped the version in the repo for re2c
0.13.7.5
(latest - no changes in generated states ) and bison to 3.0.4 . I
updated Readme as well.I have pushed the bison files in
http://git.php.net/?p=php-src.git;a=commit;h=911f7b10b1f4c9529bc01580d29
8a
93a5cd6bbd2
. There is an explanation why the C preprocessor guard macro names are
file system dependent. It means why there is
YY_PHP_JSON_YY_HOME_JAKUB_PROG_PHP_MASTER_EXT_JSON_JSON_PARSER_TAB_H_IN
CL
UDED
.
It's due to bison algorithm for creating such name. As I noted the only
solution that works for me is using different yacc.c skeleton. I have
done it in jsond in
https://github.com/bukka/php-jsond/commit/583619d7962fa57bf97dcdac4147d
8b
599a42672
where I have optional bison generation which means that I can stick with
one bison version and use custom skeleton file. This is however not
possible in the core where we allow range of bison versions which
doesn't play well with skeletons that are version specific.thanks for pushing. I'm using re2c 0.13.7.5 now for master as well. With
bison, it'll be however hard to move from 2.4.1 on Windows (and it's not
that critical), but file generated with it seems to work. Anyway, nothing
prevents you or anyone to regenerate it and push over, just in case.
Most
important is that fixes land in the .re/.y sources. And one knows who to
ping for verifications :)
FYI I had to downgrade re2c to 0.13.6 as the latest randomly crashes.
Regards
Anatol
Hi Anatol,
On Sun, Feb 22, 2015 at 6:09 PM, Anatol Belski anatol.php@belski.net
wrote:
FYI I had to downgrade re2c to 0.13.6 as the latest randomly crashes.
Ok. :) There are no differences in the generated DFA so it's not a problem
for me to use 0.13.6 too.
The preferred versions are more about nicer diffs when regenerating files.
So it's not a big issue if it gets regenerated with another supported
version. I test all supported versions when I do some changes to the parser
or scanner and I can always regenerate it back if someone else needs to do
some urgent changes ;)
Cheers
Jakub
Pierre Joye in php.internals (Mon, 9 Feb 2015 09:40:21 +0700):
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?
Yes, you should. I ran into this problem when I tried to compile PHP7
from git head. The Windows builds are currently broken:
http://windows.php.net/downloads/snaps/master/r0698901/logs/make-nts-windows-vc11-x86-r0698901.html
Jan
Pierre Joye in php.internals (Mon, 9 Feb 2015 09:40:21 +0700):
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?Yes, you should. I ran into this problem when I tried to compile PHP7
from git head. The Windows builds are currently broken:
http://windows.php.net/downloads/snaps/master/r0698901/logs/make-nts-windows-vc11-x86-r0698901.html
Yes, that's where I notice it was not present when I was working on
fixing the build in the 1st place (adding re2c call on win)
Pierre
@pierrejoye | http://www.libgd.org
hi,
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?
We do not include bison generated files in the git repo, because bison is
already a dependency for building PHP from git. We only include re2c
generated files. Please keep it that way.
Nikita
hi,
Should we push json_parser.tab.c? Which is generated (re2c), just like
what is done in date or other?We do not include bison generated files in the git repo, because bison is
already a dependency for building PHP from git. We only include re2c
generated files. Please keep it that way.Nikita
re2c is dependency for building PHP as well so that's really not the
reason, is it? :)
My point here is that I would like to use a new re2c (I'd really love to
use native utf-8 ranges implemented in 0.13.7) and possibly some bison
features (cleaner definitions, nicer %empty rules - that's not of course
necessary but the parser would look nicer :) ). It would be great if I
could have a finer control of the used version for both tools as it's also
critical for perf testing. If you look to my PR, it eliminates user
dependency on the increased re2c version and the same could be used for
bison. There won't be any git conflicts as well... That would be a bit more
complicated in the config.m4 if I had to generate bison files and ignore
re2c. The reason is is a single Makefile.frag that would have to be split
which would mean bigger changes to the build system.
Is there actually any reason why it is better not to have both json
generated files in the repo?
Cheers
Jakub