-
Notifications
You must be signed in to change notification settings - Fork 24
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
Further CI adjustments for Java interoperability branch #169
Conversation
@GitMensch At the moment the tests with a call to Java fail:
In
so that lib seems to be built. I'm not sure what part is missing for the dynamic lookup to operate properly. |
6bd657a
to
d554c75
Compare
It would be good if someone merges 1231387 and by doing so applying these changes also to |
17aea01
to
eb61422
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm opposed to the bashrc adjustment as we won't want to do that when installing GnuCOBOL either.
jvm.dll is always in $JAVA_HOME/bin/server, isn't it?
Wouldn't it be reasonable then to check if JAVA_HOME is set, and if yes use it concatenated with "bin/server", passing it to lt_dladdsearchdir()
?
If we then cannot delay-load libcob-java,m we can also hint the user to set JAVA_HOME.
Maybe there's something similar that would be useful outside of WIN32 as well).
.github/workflows/windows-java.yml
Outdated
- name: install | ||
run: | | ||
make -C _build install | ||
find /opt/cobol > _build/install.log | ||
shell: msys2 {0} | ||
|
||
- name: Upload install.log | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: install-${{ matrix.os }}.log | ||
path: _build/install.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be real good reason to have the tests only run after install (if I remember correctly we did not found another way on MacOS only) - please move install+install log at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow is adapted from another I found in the gc4 branch. I guess that one needs to be fixed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all development is iterative - that's obviously true for CI definitions as well :-)
Thank you for fixing the gc3+gc4 with anything you find here (at least some action versioning and "no install before test" so far)
.github/workflows/windows-java.yml
Outdated
shell: msys2 {0} | ||
run: | | ||
# Be sure to have `jvm.dll` in path for tests with JNI below | ||
jvmdir="$(dirname $(find "$JAVA_HOME" -name jvm.dll))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does dirname produce a non unix path (and if it doesn't - why executing cygpath then)?
eb61422
to
0cb9ab3
Compare
Thanks @GitMensch for your review.
Is it a question or an assertion? I don't know the answer, as I said I'm completely unfamiliar with how things are done on Windows.
If |
0cb9ab3
to
78fbcf2
Compare
You're welcome. Thanks for taking care in the first place!
That seems more JVM related than Windows - it is what I've found in multiple Windows installs using JRE since 8 (aka 1.8.0) up top current ones with both x64 and x86 architectures... While I just have seen 1 very old installation that has it under $JAVA_HOME/bin/client... checking further led to https://www.oracle.com/java/technologies/ms-windows-install-64bit.html#jvm.dll, so these two seem to be the official paths. While this can be all made up, the following answer of ChatGPT4 seems quite reasonable and, from a glance, matches what https://www.oracle.com/java/technologies/whitepaper.html#overview documents:
which brings us to...
I'd say for now it would be reasonable to append both the server and the client directory (in this order) to the search path, as soon as JAVA_HOME is set. Please adjust it that way (doing that in the code for For Linux this is the libvjm.so. Checking some Linux distros shows that (on older OS?) there's a subfolder in the system package manager installs containing the architecture but binary jdk downloads (and newer OS?) have those "directly" under $JAVA_HOME/lib/server/libjvm.so (or $JAVA_HOME/lib/client/libjvm.so). Maybe that's overkill, but what do you think of the following:
|
... second thought about "JVM_LIB_PATH": as people can self-prefix the system specific search dir we likely can just drop that extra variable and just append to the search path according to JAVA_HOME |
Agreed ;-) |
f3852b2
to
eb021a0
Compare
@GitMensch While investigating this I'm wondering whether one should always assume |
BTW configuring with
Looking more and more like a rabbit hole. |
Hm... that change gets bigger than expected... Suggestion:
|
09a109d
to
78c2fe5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the solution looks nice to me (ChangeLogs entries missing including the separate one for the reasonable move to the systfines.h - if they are still needed in fileio.c, likely they can be kept in call.c only?)
Concerning the failing MacOS CI - is JAVA_HOME set there?
libcob/call.c
Outdated
#define lt_dlopenlcl(x) lt_dlopen(x) | ||
#define lt_dlclose(x) FreeLibrary(x) | ||
#define lt_dlopenlcl(x) lt_dlopen(x) | ||
#define lt_dlclose(x) FreeLibrary(x) | ||
#define lt_dlinit() | ||
#define lt_dlexit() | ||
#define lt_dlhandle HMODULE | ||
#define lt_dlhandle HMODULE | ||
#define lt_dladdsearchdir(x) AddDllDirectory(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a WIN32 definition and we don't use that for this case - you can therefore revert those lines
eb8b752
to
14a44f4
Compare
17c7e68
to
3e22b7b
Compare
https://github.com/OCamlPro/gnucobol/actions/runs/10486974200/job/29046364090?pr=169#step:8:230 I'll disable mingw32 as I'm unsable to find what package to install to get that on |
Better drop BDB, cjson and xml2 from all java CI definitions and explicit request I personally would not use pacboy for the matrix but the "good old" standard one:
You can then also use the default dependencies from msys2 https://github.com/msys2/MINGW-packages/blob/67aee9a6de4f153729397c6a5b09fd8bd39786ca/mingw-w64-gnucobol/PKGBUILD#L22-L28 - but as noted, those dependencies should get into the "common" CI for msys2, along with adding that build matrix to #170. |
3e22b7b
to
8c314e5
Compare
0528d97
to
4426107
Compare
As the failing Ubuntu one is not related to java-interop you may still merge that. You want to use:
|
Many thanks for this suggestion! I suspected there would be one variable to pass configure args, but I though since it's only for Java interop. we could discard the distribution. I'll do the change ASAP. |
1c0a315
to
ec81415
Compare
ec81415
to
499275d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nberth here's the detailed review you've asked for :-)
Someday we'll have to make a decision if we want to keep the java tests separated or not; if we know that this doesn't work on some environment, then using a conditional there depending on the build matrix seems useful;
for the binary nightly packages (MSYS2, MSVC, possibly Ubuntu) a "full featured" build would be useful...
@@ -49,9 +44,7 @@ jobs: | |||
- name: bootstrap | |||
run: | | |||
sed -i '' 's/-undefined suppress//g' configure.ac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this doing and why? It seems to originate from @ddeclerck.
We should at least add a comment there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Recent" versions of XCode (>= 15 ?) include a new linker, which does not support the -undefined
flag. This substitution removes that option, which would otherwise cause the testsuite to fail on MacOS. Until a definitive solution is found...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this would need to be checked by configure
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configure uses this only in a single place - "newer" MacOS and a compiler claiming to be GCC:
case $host_os in
rhapsody*)
if test "$COB_USES_GCC" = yes; then
COB_SHARED_OPT="-bundle -flat_namespace -undefined suppress"
fi
;;
... so I've learned something new: https://en.wikipedia.org/wiki/Rhapsody_(operating_system)
I've also checked the used files from https://git.savannah.gnu.org/cgit/config.git and will update some "soon" but that won't help for this specific issue.
For that we can add a separate case entry (what is $host_os
in the failing case?) or try if we can link successfully w/wo those options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The libtool.m4
seems to distinguish using darwin versions as well:
# serial 58 LT_INIT
…
case $host_os in
rhapsody* | darwin1.[[012]])
_lt_dar_allow_undefined='$wl-undefined ${wl}suppress' ;;
darwin1.*)
_lt_dar_allow_undefined='$wl-flat_namespace $wl-undefined ${wl}suppress' ;;
darwin*) # darwin 5.x on
# if running on 10.5 or later, the deployment target defaults
# to the OS version, if on x86, and 10.4, the deployment
# target defaults to 10.4. Don't you love it?
case ${MACOSX_DEPLOYMENT_TARGET-10.0},$host in
10.0,*86*-darwin8*|10.0,*-darwin[[912]]*)
_lt_dar_allow_undefined='$wl-undefined ${wl}dynamic_lookup' ;;
10.[[012]][[,.]]*)
_lt_dar_allow_undefined='$wl-flat_namespace $wl-undefined ${wl}suppress' ;;
10.*|11.*)
_lt_dar_allow_undefined='$wl-undefined ${wl}dynamic_lookup' ;;
esac
;;
esac
…
(not sure that's relevant though)
No description provided.