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

Setup paths without FIXPATH to OMR configure #262

Closed
wants to merge 1 commit into from
Closed

Setup paths without FIXPATH to OMR configure #262

wants to merge 1 commit into from

Conversation

JasonFengJ9
Copy link
Member

Pass JAVA_SPEC_VERSION to buildj9tools.mk to accommodate JDK16+ specific change;
Setup paths without FIXPATH such as CC_NOFIXPATH, CXX_NOFIXPATH, VS_INCLUDE_NOFIXPATH, and VS_LIB_NOFIXPATH.

With this PR and its dependent PR, now JDK17 Windows builds and the -version output is:

openjdk version "17-internal" 2021-09-14
OpenJDK Runtime Environment (build 17-internal+0-adhoc.Administrator.openj9-openjdk-jdk)
Eclipse OpenJ9 VM (build jdk17win-f1de38020, JRE 17 Windows Server 2016 amd64-64-Bit Compressed References 20201227_000000 (JIT enabled, AOT enabled)
OpenJ9   - f1de38020
OMR      - 62dd1d42a
JCL      - bbf9223aedc based on jdk-17+3)

Depends on eclipse-openj9/openj9#11543
closes eclipse-openj9/openj9#11410

Signed-off-by: Jason Feng [email protected]

@JasonFengJ9 JasonFengJ9 mentioned this pull request Dec 27, 2020
Closed
@JasonFengJ9 JasonFengJ9 force-pushed the jdk17win branch 3 times, most recently from f1fb12a to 8523254 Compare December 27, 2020 19:49
pshipton
pshipton reviewed Dec 30, 2020
View reviewed changes
closed/autoconf/custom-hook.m4
@@ -464,7 +464,7 @@ AC_DEFUN([OPENJ9_THIRD_PARTY_REQUIREMENTS],
fi

if test "x$OPENJDK_BUILD_OS_ENV" = xwindows.cygwin ; then
FREEMARKER_JAR=`$CYGPATH -m "$with_freemarker_jar"`
FREEMARKER_JAR=`cygpath -m "$with_freemarker_jar"`
Copy link
Member

Choose a reason for hiding this comment

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

Is there a problem with $CYGPATH?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, CYGPATH is empty when the command in question is executed.

Copy link
Member Author

Choose a reason for hiding this comment

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

20:46:53  ./common/omrcuda.cpp(41): fatal error C1083: Cannot open include file: 'cuda.h': No such file or directory

Will test cuda.

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 should try to order things so this can continue to use $CYGPATH.

Copy link
Member

Choose a reason for hiding this comment

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

It appears that CYGPATH is superseded by PATHTOOL: see basic_tools.m4.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other hand, OpenJDK uses cygpath extensively from searching result.

Copy link
Member

Choose a reason for hiding this comment

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

There aren't so many references in the openj9-staging branch; most have been replaced by PATHTOOL. We should do likewise.

@pshipton
Copy link
Member

jenkins test sanity win jdknext depends eclipse-openj9/openj9#11543

@pshipton
Copy link
Member 

cl  -DOMRPORT_LIBRARY_DEFINE -I. -I./winamd64 -I./win32_include -I./win32 -I./common -I./include -I./ -I/NVIDIA/CUDA/v5.5/include  -I../../include -I../include_core -I../nls -D_AMD64_=1 -DOMR_OS_WINDOWS -DWIN64 -D_WIN64 -DWIN32 -D_WIN32 -DJ9HAMMER -D_CRT_SECURE_NO_WARNINGS -DCRTAPI1=_cdecl -DCRTAPI2=_cdecl -nologo -D_WIN95 -D_WIN32_WINDOWS=0x0500 /D_WIN32_DCOM -D_MT -D_WINSOCKAPI_ -D_WIN32_IE=0x0500 -DWINVER=0x0601 -D_WIN32_WINNT=0x0601 -D_DLL  -Zm400 -MD /Ox -WX -W3 -Fdomrcuda.pdb /Zi   -c ./common/omrcuda.cpp /Fo./

./common/omrcuda.cpp(41): fatal error C1083: Cannot open include file: 'cuda.h': No such file or directory

@JasonFengJ9
Copy link
Member Author

jenkins test sanity win jdknext depends eclipse-openj9/openj9#11543

@JasonFengJ9
Copy link
Member Author

jenkins copyright check

@pshipton pshipton requested a review from keithc-ca December 30, 2020 22:04
@JasonFengJ9
Setup paths without FIXPATH to OMR configure
12c0baf 
Pass JAVA_SPEC_VERSION to buildj9tools.mk to accommodate JDK16+ specific
change;
Setup paths without FIXPATH such as CC_NOFIXPATH,
CXX_NOFIXPATH, VS_INCLUDE_NOFIXPATH, and VS_LIB_NOFIXPATH.

Signed-off-by: Jason Feng <[email protected]>
keithc-ca
keithc-ca requested changes Jan 4, 2021
View reviewed changes
closed/GensrcJ9JCL.gmk
@@ -45,6 +45,7 @@ $(J9JCL_SOURCES_DONEFILE) : $(AllJclSource)
@$(ECHO) Building OpenJ9 Java Preprocessor
@$(MKDIR) -p $(J9TOOLS_DIR)
$(MAKE) $(MAKE_ARGS) -C $(OPENJ9_TOPDIR)/sourcetools -f buildj9tools.mk \
JAVA_SPEC_VERSION=$(VERSION_FEATURE) \
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary if a phony target, say jpp_jar is defined in buildj9tools.mk which builds jpp.jar; the occurrence of $(call FixPath,$(JPP_JAR)) below would be replaced by jpp_jar. The rules for that new target would be platform-independent.

Copy link
Member Author

@JasonFengJ9 JasonFengJ9 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

jpp.jar shares BuildJar_template function with a few other jar files om.jar, uma.jar, j9vmcp.jar, and j9nls.jar. A phony target like jpp_jar might have to duplicate some code from BuildJar_template which seem not desirable.

Just got another thought, removing FixPath from $(call FixPath,$(JPP_JAR)) can match $(JPP_JAR) with the target within OpenJ9 buildj9tools.mk. This can avoid eclipse-openj9/openj9#11543.
@keithc-ca does this sound a better approach?
Edit: this matches the target but later jar build requires fixed path hence fails.

closed/autoconf/custom-hook.m4
@@ -464,7 +464,7 @@ AC_DEFUN([OPENJ9_THIRD_PARTY_REQUIREMENTS],
fi

if test "x$OPENJDK_BUILD_OS_ENV" = xwindows.cygwin ; then
FREEMARKER_JAR=`$CYGPATH -m "$with_freemarker_jar"`
FREEMARKER_JAR=`cygpath -m "$with_freemarker_jar"`
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 should try to order things so this can continue to use $CYGPATH.

make/autoconf/basic.m4
Comment on lines +397 to +398
AC_SUBST(CC_NOFIXPATH)
AC_SUBST(CXX_NOFIXPATH)
Copy link
Member

Choose a reason for hiding this comment

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

These should appear in a 'closed' file.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to support spec.gmk generation.

  SPEC="$OUTPUTDIR/spec.gmk"
...
  AC_SUBST(CC_NOFIXPATH)
  AC_SUBST(CXX_NOFIXPATH)

  # The spec.gmk file contains all variables for the make system.
  AC_CONFIG_FILES([$OUTPUTDIR/spec.gmk:$AUTOCONF_DIR/spec.gmk.in])

It could be included in a closed file but that file need to be included here hence it doesn't seem much different.

make/autoconf/spec.gmk.in
Comment on lines +500 to +501
CC_NOFIXPATH := @CC_NOFIXPATH@
CXX_NOFIXPATH := @CXX_NOFIXPATH@
Copy link
Member

Choose a reason for hiding this comment

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

I don't think modifying this openjdk file is necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of the change to get CC_NOFIXPATH & CXX_NOFIXPATH into spec.gmk which exports environment variables to be referred by OMR configure script.
This is needed unless we find another way to pass those settings.

make/scripts/extract-vs-env.cmd Show resolved Hide resolved
@keithc-ca
Copy link
Member

I may have a cleaner fix that changes only one openjdk file (fixpath.sh) so that it handles relative paths properly. I'm starting a fresh local build (together with eclipse-openj9/openj9#11589). If that goes as I expect, I'll tidy up and open a pull request for this repo.

For those that want a preview, see https://github.com/keithc-ca/openj9-openjdk/tree/jdk17.

@JasonFengJ9 JasonFengJ9 closed this Jan 8, 2021
@JasonFengJ9 JasonFengJ9 deleted the jdk17win branch January 8, 2021 04:16
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 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JasonFengJ9 @pshipton @keithc-ca