Additional Changes - Githubissues

bsdkurt commented 5 years ago

This removes --with-package-path and makes an adjustment to build-performance for build stability on OpenBSD.

--with-package-path removal leaves two TODO items: 1) os_bsd.cpp: Either patch and substitute /usr/local/ in DEFAULT_LIBPATH or provide alternate solution for adjusting the prefix. 2) iconv: Replace this with a more generally accepted autoconf solution.

bsdkurt commented 5 years ago

Thanks Greg. Let me know your thoughts on the two remaining items.

battleblow commented 5 years ago

Hi Kurt, I've pulled this change into the FreeBSD port locally. I'll commit shortly.

For os_bsd.cpp I think that is the right solution. It has a patch to os_bsd.cpp that replaces /usr/local with %%LOCALBASE%% and then substitutes that pattern (which is fairly standard in the FreeBSD ports system) as a post-patch target along with other relevant files (currently only configure).

In terms of iconv, your patches have actually fixed a jshell bug that was iconv related (thank you!). The FreeBSD system iconv routines in libc on supported versions of FreeBSD fully support what jshell needs. The bug was caused by /usr/local/include always being in the include path, which ended up using the third party version of iconv.h rather than the system version, which caused failures loading libjdwp since some iconv symbols were unresolved.

That aside, an autoconf solution that allows OSes that require a third party libiconv would the way to go, but it seems like it needs to potentially account for three scenarios (based on make/lib/Lib-jdk.jdwp.agent.gmk)

Systems which have iconv.h in a standard header location and iconv routines in libc (no additional libraries need linking)
Systems which have iconv.h in a standard header location but require a separate libiconv to be linked with
Systems which have iconv.h and libiconv as a third party library in a non-standard location

You could perhaps use the following for this:

--with-iconv=iconv
--with-iconv-include=/usr/local/include
--with-iconv-lib=/usr/local/lib

where

--with-iconv - the argument, if present, has -l prepended and is passed to the linker (for libraries/executables with iconv dependencies).
--with-iconv-include - the argument, if present, has -I prepended and is passed to the compiler/preprocessor
--with-iconv-lib - the argument, if present, has -L prepended and is passed to the linker

That might be what fits in most closely with other options. If arguments are not given then these are omitted from the linker/compiler arguments.

bsdkurt commented 5 years ago

Hi Greg, your welcome. Yea, I've run into issues with using CFLAGS="-I/usr/local/include" since it typically ends up at the beginning of the of the application's CFLAGS which makes any /usr/local/include headers override internal headers where there is a naming conflict. Its better to get the include path added at the end when needed.

All sounds good. Thank you for the pointers. I'll take a shot at the iconv configure args later this week.

I'm about to commit the jdk 11 to OpenBSD's ports tree. If you have a chance, could you cut a release?

battleblow commented 5 years ago

Yep, no problem. See

https://github.com/battleblow/openjdk-jdk11u/releases/tag/jdk-11.0.2%2B9-3

I'm hoping to look at pulling the relevant commits forward into openjdk12 later this week.

battleblow / openjdk-jdk11u

Additional Changes #11