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

Remove BisonHeaderToC++.sed #1517

Closed
wants to merge 1 commit into from
Closed

Remove BisonHeaderToC++.sed #1517

wants to merge 1 commit into from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Feb 28, 2017

We can do without it, and less is more?

We can do without it, and less is more?
@jablko jablko added this to the 7.2.0 milestone Feb 28, 2017
@jablko jablko self-assigned this Feb 28, 2017
@SolidWallOfCode
Copy link
Member

I'm not sure we can do without this. I know I had problems which lead me to do this which may be related to older versions of Bison. Note that it works for 3.0.4 compared to 2.7. My experience is that Bison versions on old operating systems we support tend to be very old. I will need to check this on older OS versions we use here at Y!.

Additionally the plan going forward for this is to remove all the parsing from TSConfig and have it depend on Lua.

@jablko
Copy link
Contributor Author

jablko commented Mar 1, 2017

I'm not sure we can do without this. I know I had problems which lead me to do this which may be related to older versions of Bison. Note that it works for 3.0.4 compared to 2.7. My experience is that Bison versions on old operating systems we support tend to be very old. I will need to check this on older OS versions we use here at Y!.

Thanks for taking a look! It should work with Bison 2.7, just based on the GitHub CI jobs (e.g. the FreeBSD job uses 2.7 [1]). The CentOS 6 machine uses 2.4 [2] -- but we don't run that for pull requests, I think.

I think the important difference between TsConfigGrammar.h and TsConfigGrammar.hpp is that TsConfigGrammar.h contains yyscan_t (the first tsconfigparse() parameter) and TsConfigGrammar.hpp doesn't -- but replacing %parse-param { yyscan_t lexer } with %parse-param { void *lexer } has the same effect?

[1] https://ci.trafficserver.apache.org/files/BuildMachines/FreeBSD10
[2] https://ci.trafficserver.apache.org/files/BuildMachines/CentOS6

Additionally the plan going forward for this is to remove all the parsing from TSConfig and have it depend on Lua.

Nice!

@zwoop
Copy link
Contributor

zwoop commented Mar 8, 2017

Lets re-run this with the new Github builders: [approve ci]

@zwoop
Copy link
Contributor

zwoop commented Mar 14, 2017

Note that it has to succeed on CentOS6, which is indeed running

[root@centos6 ~]# bison  --version
bison (GNU Bison) 2.4.1

@zwoop zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017
@PSUdaemon
Copy link
Contributor

@jablko can you rebase this?

@bryancall
Copy link
Contributor

[approve ci]

@zwoop
Copy link
Contributor

zwoop commented Jun 8, 2017

Still feels like we have to support the Bison that comes with CentOS6.

@bryancall
Copy link
Contributor

Ran a build on this for Centos 6 to see if there is an issue.

@zwoop
Copy link
Contributor

zwoop commented Aug 17, 2017

Part of the "trickiness" here is that the file is also checked into git, so many cases, we don't need bison at all. But it doesn't seem to be particularly deterministic.

@bryancall
Copy link
Contributor

There hasn't been an update on this PR for over 60 days, so I am closing it for now. Feel free to reopen the PR when you have updated it.

@bryancall bryancall closed this Nov 2, 2017
@bryancall bryancall removed this from the 8.0.0 milestone Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants