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

Upgrade antlr4 to 4.10.1 #23937

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Conversation

zhfeng
Copy link
Contributor

@zhfeng zhfeng commented Feb 2, 2023

Fixes #19990.

Changes proposed in this pull request:

  • Bump antlr4 to 4.10.1
  • Only run antlr4-maven-plugin in JDK 11
  • Add generated sources in JDK 8
  • After merging this PR, we should clarify on docs and wiki that it needs to use JDK 11 to build sources at first as it can generate codes for the g4 parseres.

Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 2, 2023

@TeslaCN @terrymanu can you take a review?

@terrymanu terrymanu merged commit 4afa294 into apache:master Feb 3, 2023
@terrymanu
Copy link
Member

LGTM

@terrymanu terrymanu added this to the 5.3.2 milestone Feb 3, 2023
@TeslaCN
Copy link
Member

TeslaCN commented Feb 3, 2023

Hi @zhfeng
It seems that you have moved generated directory outside the target. The IDEA cannot recognize the code automatically.

image

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 3, 2023

Sorry for the inconvenience. I took these changes because it could build with JDK 11 to generate the parser codes only once, and mvn clean install could work with JDK 8. Ony when you change g4 files, you have to re-generate these codes by JDK 11. I suppose that this could less impact on the developers with JDK 8.

The for IDEA, I think you have to add source directory manually. such as src/main/generated. Do you have any other thought?

@TeslaCN
Copy link
Member

TeslaCN commented Feb 3, 2023

Sorry for the inconvenience. I took these changes because it could build with JDK 11 to generate the parser codes only once, and mvn clean install could work with JDK 8. Ony when you change g4 files, you have to re-generate these codes by JDK 11. I suppose that this could less impact on the developers with JDK 8.

The for IDEA, I think you have to add source directory manually. such as src/main/generated. Do you have any other thought?

I don't think manually add source directory is acceptable. Because we need to add 15 modules manually to start Proxy or run unit tests in IDEA, and these settings might be lost after doing some operation.

I will raise a PR to restore the path of generated codes.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 3, 2023

@TeslaCN can you try this setting?

@TeslaCN
Copy link
Member

TeslaCN commented Feb 3, 2023

@TeslaCN can you try this setting?

Of course I can. But I prefer the way that developers could open the project without addtional settings.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 3, 2023

OK, I just wonder why IDEA could find target\generate-sources\antlr4 as a source directory? It seems that antlr-maven-plugin is also running with generate-sources phase.

@zhfeng
Copy link
Contributor Author

zhfeng commented Feb 3, 2023

Hmm, it detects target and subdirectory automatically.

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.

Update antlr4 to 4.10+
3 participants