Skip to content

8361216: Do not fork javac in J2DBench ant build #26077

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+3 −8
Open

8361216: Do not fork javac in J2DBench ant build #26077

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/demo/share/java2d/J2DBench/build.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@
<property name="dist" location="dist"/>
<property name="resources" location="resources"/>

<condition property="source" value="7">
<condition property="source" value="8">
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be inconsistent with the Make target. It doesn't seem right to update just the ant-based target.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I realized that yesterday, I never actually looked at or used the Makefile before. I will update the Makefile so it least this PR in its own right is consistent, regardless of these other discussion points.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can just drop ant and leave the makefile as it is. The source/target settings are set to that low level to indicate that the source code should be compatible with older versions of the JDK. Currently, JDK 1.4 compatibility is still maintained.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just drop ant and leave the makefile as it is. The source/target settings are set to that low level to indicate that the source code should be compatible with older versions of the JDK. Currently, JDK 1.4 compatibility is still maintained.

I have supposed that this PR exists because Eric wants to use ant. Eric ?

I think for today's mainline, not supporting jdk7 as an out of the box target isn't a huge problem.
Current javac doesn't support 7 as a target so it seems reasonable to me.

Copy link
Member

Choose a reason for hiding this comment

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

Current javac doesn't support 7 as a target so it seems reasonable to me.

That's why the makefile was parameterized, to easily set the target and source options based on the currently used minimal supported version of javac. But we may change it with a comment that 1.4 still should be supported, or something like that.

<not>
<isset property="source"/>
</not>
</condition>
<condition property="target" value="7">
<condition property="target" value="8">
<not>
<isset property="target"/>
</not>
Expand All @@ -54,11 +54,6 @@
<isset property="java"/>
</not>
</condition>
<condition property="javac" value="javac">
<not>
<isset property="javac"/>
</not>
</condition>

<target name="init">
<!-- Create the time stamp -->
Expand All @@ -70,7 +65,7 @@
<target name="compile" depends="init"
description="compile the source " >
<!-- Compile the java code from ${src} into ${build} -->
<javac debug="off" source="${source}" target="${target}" srcdir="${src}" destdir="${build}" fork="true" executable="${javac}"/>
<javac debug="off" source="${source}" target="${target}" srcdir="${src}" destdir="${build}"/>
</target>

<target name="run" depends="dist"
Expand Down