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

Swift target does not build on JDK 6 #1327

Closed
ewanmellor opened this issue Nov 6, 2016 · 13 comments
Closed

Swift target does not build on JDK 6 #1327

ewanmellor opened this issue Nov 6, 2016 · 13 comments

Comments

@ewanmellor
Copy link
Contributor

The Swift target at https://github.com/ewanmellor/antlr4/tree/swift-target does not currently build on JDK 6. It is using java.lang.ProcessBuilder.Redirect, which was added in JDK 7.

I don't know if this is a problem for merging into mainline. @parrt ?

@parrt
Copy link
Member

parrt commented Nov 6, 2016

Hmm...If we can leave it as compatible with jdk 6 that would be nice. I don't think we have made an across-the-board requirement for jdk 7 yet.

@ewanmellor
Copy link
Contributor Author

@parrt Can I persuade you otherwise? ;-)

Java 6 hasn't had a publicly available update since April 2013 (according to Wikipedia) but Oracle have made dozens of security fixes under their support program. That means that the public version is known to be insecure. I don't think that we (or anyone) should be supporting it unless you have a user with a compelling need.

The use of ProcessBuilder.Redirect is in the Swift BaseTest, so it's not critical and I'm sure that we could rewrite it, but just as a matter of policy I don't think it's a good idea to cling onto JDK 6.

(I'm sure Travis would appreciate us dropping a platform too.)

@parrt
Copy link
Member

parrt commented Nov 10, 2016

Hmm...ok, anybody object to requiring Java 7 now? @sharwell @ericvergnaud ?

@ericvergnaud
Copy link
Contributor

I support dropping Java 6.

Envoyé de mon iPhone

Le 11 nov. 2016 à 03:23, Ewan Mellor notifications@github.com a écrit :

@parrt Can I persuade you otherwise? ;-)

Java 6 hasn't had a publicly available update since April 2013 (according to Wikipedia) but Oracle have made dozens of security fixes under their support program. That means that the public version is known to be insecure. I don't think that we (or anyone) should be supporting it unless you have a user with a compelling need.

The use of ProcessBuilder.Redirect is in the Swift BaseTest, so it's not critical and I'm sure that we could rewrite it, but just as a matter of policy I don't think it's a good idea to cling onto JDK 6.

(I'm sure Travis would appreciate us dropping a platform too.)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@parrt
Copy link
Member

parrt commented Nov 11, 2016

Added #1352

@marcohu
Copy link
Contributor

marcohu commented Nov 12, 2016

Does this involve only the build environment requirement or will the runtime also depend on Java 7?

@parrt
Copy link
Member

parrt commented Nov 12, 2016

just the build

@marcohu
Copy link
Contributor

marcohu commented Nov 12, 2016

Great! I would seriously consider going straight to Java 8 then.

Do you plan to use something like the Animal Sniffer plugin to ensure JDK 6 compatibility for the runtime?

@sharwell
Copy link
Member

sharwell commented Nov 14, 2016

@marcohu We already build the release using a newer JDK, and use the bootclasspath build option to ensure JDK 6 compatibility. Edit: While my optimized builds (a separate product/release) do use this, it appears this functionality was stripped from the builds used by the reference repository.

It seems like the use of ProcessBuilder.Redirect is likely not required. Can you clarify where it is used? We can replace it with the mechanism already used in the Java target tests to redirect/consume output from a separate process.

@marcohu
Copy link
Contributor

marcohu commented Nov 14, 2016

@sharwell I'm actually using your version, bootclasspath checking is not implemented universally. Don't know whether this is intentional. Compiling the maven plugin yields a warning for me:

[WARNING] bootstrap class path not set in conjunction with -source 1.6

I'm using Java 7 features in #1353 already and would very much prefer to be able to do so. At least for testing. For the runtime I'm absolutely pro Java 6.

@parrt
Copy link
Member

parrt commented Nov 14, 2016

@marcohu I think we'll do jdk 7 for now.

@parrt parrt closed this as completed in ea7a61c Nov 14, 2016
@parrt parrt added this to the 4.6 milestone Nov 14, 2016
@sharwell
Copy link
Member

sharwell commented Nov 14, 2016

@marcohu

I'm actually using your version, bootclasspath checking is not implemented universally. Don't know whether this is intentional.

This is intentional. Not all users have a Java 6 JDK laying around, so disabling this feature in the default build reduces the effort required for new developers to start working on the project. 😄

@marcohu
Copy link
Contributor

marcohu commented Nov 14, 2016

@sharwell While I strongly agree to have fully automated builds, I would prefer to have requirements still enforced. That's where animal sniffer etc. could prove useful. Otherwise developers might use unsupported features at one point and have to rework their code.

But as this discussion is off-topic here, we should continue in the forum if there is any interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants