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

Updates for docker based ARM build #2057

Merged
merged 1 commit into from
Dec 4, 2017
Merged

Updates for docker based ARM build #2057

merged 1 commit into from
Dec 4, 2017

Conversation

JamesKingdon
Copy link
Contributor

configure_linux_arm.mk and rules.linux.mk are updated to make
use of the environment variables provided by the docker file
(eclipse-openj9/openj9#622).

This allows the names of the toolchain executables to be passed
from the docker file and also avoids the need to hard-code the
location of the global include files.

Signed-off-by: James Kingdon jkingdon@ca.ibm.com

@@ -27,36 +27,54 @@ else
TEMP_TARGET_DATASIZE:=32
endif

ifeq ($(OPENJ9_DOCKER_CC_ARM),1)
TMP_HOST = $$(OPENJ9_CC_PREFIX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the name of the environment variable to remove the OpenJ9 from it? The OMR project is / can be used for many downstream projects so we should not be adding any new OpenJ9 specific options. I am assuming this would be useful for other projects as well if it had a different name. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @charliegracie ,
In this case OMR is the consumer of the variable which is defined in the Docker file added to openJ9 in eclipse-openj9/openj9#622. The variable is named for consistency with other variables defined in that file. OPENJ9_DOCKER_CC_ARM specifically is destined to be removed as soon as resources permit, as it is something of a quick fix to allow rapid prototyping of builds for armhf.

Copy link
Contributor

@jdekonin jdekonin Nov 29, 2017

Choose a reason for hiding this comment

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

If I understand the interaction between OpenJ9 and OMR correctly, openj9/runtime/gc_glue_java/configure_includes/configure_linux_arm.mk is the actual mk used by openj9 when omr configure is invoked, and the omr/glue is what is used for compiling OMR alone. omr/example/glue being a set of examples to work from. If this is the case, then these changes need to happen in the openj9 repository. Can you confirm or deny that @charliegracie?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's interesting. Changes to openj9/runtime/gc_glue_java/configure_includes/configure_linux_arm.mk were contributed in eclipse-openj9/openj9#686.
If the files changed in this PR are not actually needed for building OpenJDK then we can scrap them and avoid the whole issue of what to call the env vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something in this PR is needed as the build fails if I revert to the original branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the change to omrmakefiles/rules.linux.mk that is needed in order to get the -marm option to the new compiler, so I can reduce this PR to just that change.

else
TMP_HOST = arm-bcm2708hardfp-linux-gnueabi
endif

ifeq (linux_arm, $(SPEC))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like the wrong test, it should be
ifneq (,$(findstring linux_arm,$(SPEC)))
to also accept linux_arm_docker.

@JamesKingdon
Copy link
Contributor Author

@charliegracie I removed the unnecessary changes containing the OPENJ9 variables.

rules.linux.mk is updated to pass -marm to the compiler.
I also moved the architecture defining options out of
OPTIMIZATION_FLAGS as the current setup doesn't work for
a debug build.

Signed-off-by: James Kingdon <jkingdon@ca.ibm.com>
@JamesKingdon
Copy link
Contributor Author

Squashed the commits.

@JamesKingdon
Copy link
Contributor Author

@charliegracie any more changes needed?

@charliegracie charliegracie merged commit 9ca34e5 into eclipse:master Dec 4, 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

3 participants