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.

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

Jump to bottom

Add OSX support #262

Closed
wants to merge 2 commits into from
Closed

Add OSX support #262

wants to merge 2 commits into from

Conversation

jdekonin
Copy link
Contributor

@jdekonin jdekonin commented Oct 6, 2017

Fixes #36

Signed-off-by: Joe deKoning [email protected]

[skip ci]

@pshipton pshipton mentioned this pull request Oct 6, 2017
Closed
pshipton
pshipton reviewed Oct 6, 2017
View reviewed changes
runtime/buildtools.mk Outdated
@@ -144,7 +144,7 @@ OMRGLUE_INCLUDES = \
../gc_vlhgc

configure : uma
$(MAKE) -C omr -f run_configure.mk 'SPEC=$(SPEC)' 'OMRGLUE=$(OMRGLUE)' 'CONFIG_INCL_DIR=$(CONFIG_INCL_DIR)' 'OMRGLUE_INCLUDES=$(OMRGLUE_INCLUDES)' 'EXTRA_CONFIGURE_ARGS=$(EXTRA_CONFIGURE_ARGS)'
$(MAKE) -C omr -f run_configure.mk 'SPEC=osx_x86-64' 'OMRGLUE=$(OMRGLUE)' 'CONFIG_INCL_DIR=$(CONFIG_INCL_DIR)' 'OMRGLUE_INCLUDES=$(OMRGLUE_INCLUDES)' 'EXTRA_CONFIGURE_ARGS=$(EXTRA_CONFIGURE_ARGS)'
Copy link
Member

Choose a reason for hiding this comment

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

I know this pull request is WIP, but note this needs to be fixed.

@stevewallin
Copy link

need to also fix the $NUMCPU processing in OpenJ9.gmk

@pshipton
Copy link
Member

pshipton commented Oct 6, 2017

Joe mentioned he has other changes he hasn't pushed yet. Some are hacks just to get things going quickly.

@gdams
Copy link

gdams commented Oct 10, 2017

Where are we with this? Keen to get it landed soon

@jdekonin jdekonin force-pushed the osx branch 2 times, most recently from d88df5a to 5e780aa Compare October 11, 2017 01:09
@jdekonin jdekonin changed the title WIP: Add OSX support Add OSX support Oct 11, 2017
@keithc-ca
Copy link
Contributor

The file osx_x86-64.spec seems to be based on the Linux x86-64 spec without compressed references. Is that what we want rather than osx_x86-64_cmprssptrs.spec for consistency with other supported platforms?

@jdekonin
Copy link
Contributor Author

In discussion with @charliegracie, eclipse/omr does not support compressedrefs on OSX.

keithc-ca
keithc-ca reviewed Oct 11, 2017
View reviewed changes
runtime/gc_glue_java/configure_includes/configure_osx.mk
CONFIGURE_ARGS += 'AS=as'
CONFIGURE_ARGS += 'CC=cc'
CONFIGURE_ARGS += 'CXX=c++'
CONFIGURE_ARGS += 'CCLINK=$$(CC)'
Copy link
Contributor

Choose a reason for hiding this comment

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

CCLINK is defined a number of places, but I couldn't find any uses; perhaps this should define CCLINKEXE?

Copy link
Contributor Author

@jdekonin jdekonin Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That could be. I copied this from the omr/example/glue. I am still working on getting a successful build alongside openj9 and the extensions. My current area of area is the linking...so may change again. I will investigate down this path too, thanks for the pointer.

jdekonin
jdekonin commented Oct 11, 2017
View reviewed changes
runtime/makelib/targets.mk.osx.inc.ftl
@@ -0,0 +1,526 @@
<#--
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 file needs review for removal of 390/ppc/arm use cases.

keithc-ca
keithc-ca reviewed Oct 11, 2017
View reviewed changes
sourcetools/com.ibm.uma/com/ibm/j9/uma/configuration/ConfigurationImpl.java Outdated
@@ -43,6 +43,7 @@
import com.ibm.j9.uma.platform.PlatformUnix;
import com.ibm.j9.uma.platform.PlatformWindows;
import com.ibm.j9.uma.platform.PlatformZOS;
import com.ibm.j9.uma.platform.PlatformOSX;
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 please organize imports (alphabetically)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, in the next push.

@keithc-ca keithc-ca mentioned this pull request Oct 11, 2017
Merged
@stevewallin
Copy link

stevewallin commented Oct 11, 2017 •
edited
Loading

to fix the linking I had to update buildtools.mk to remove

'OMRGLUE_INCLUDES=
'# ../include
../nls
../gc_base
../gc_include
../gc_stats
../gc_structs
../gc_base
../include
../oti
../gc_include
../gc_structs
../gc_stats
../gc_modron_standard
../gc_staccato
../gc_realtime
../gc_trace
../gc_vlhgc`

charliegracie
charliegracie reviewed Oct 13, 2017
View reviewed changes
sourcetools/com.ibm.uma/com/ibm/j9/uma/platform/PlatformOSX.java
import com.ibm.uma.om.Export;
import com.ibm.uma.util.FileAssistant;

public class PlatformOSX extends PlatformImplementation {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you subclassed the Unix implementation this file could be smaller as you would only have to override getSharedLibSuffix() but it may also change some of the functionality...

Copy link
Contributor

Choose a reason for hiding this comment

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

That occurred to me as well, but the class is so small I don't think it's really worth it.

@jdekonin
Add initial OSX support
8c28bb0 
Works towards eclipse-openj9#36

Signed-off-by: Joe deKoning <[email protected]>
@mstoodle
Copy link
Contributor

JIT may have some #ifdef LINUX that need to include OSX too; we should add comp.jit if we start seeing those kinds of requirements. @andrewcraik for your awareness.

@andrewcraik
Copy link
Contributor

Sounds good @mstoodle there may also be some system call stuff in the codegen that might need some TLC depending on compatibility/naming etc. Happy to help answer questions if someone is starting down that road :)

@DanHeidinga
Copy link
Member

FYI - @jdekonin and I have been making progress on this. We'll push more changes to this PR later today

@mstoodle mstoodle mentioned this pull request Oct 16, 2017
Closed
@DanHeidinga
Temporarily disable IPv6 on macosx
044c545 
Disable IPv6 to work around build issues.  The code is currently
linux-specific.  This is a required "to get something working"
and should be re-investigated later.

Signed-off-by: Dan Heidinga <[email protected]>
@jdekonin
Copy link
Contributor Author

Closing as this was broken down into smaller prs.
#357 - merged
#359 - merged
#360 - merged
#365 - waiting review

@jdekonin jdekonin closed this Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pshipton pshipton pshipton left review comments

@keithc-ca keithc-ca keithc-ca left review comments

@charliegracie charliegracie charliegracie left review comments

Assignees
No one assigned
Labels
comp:build comp:vm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jdekonin @stevewallin @pshipton @gdams @keithc-ca @mstoodle @andrewcraik @DanHeidinga @charliegracie