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

[CM H2] 3713 java binding maven to gradle migration #3714

Merged

Conversation

tucek
Copy link
Contributor

@tucek tucek commented Mar 18, 2021

Basics

These points need to be fulfilled for every PR:

  • Short descriptions of your changes are in the release notes
    (added as entry in doc/news/_preparation_next_release.md which
    contains _(my name)_)
    Please always add something to the release notes.
  • Details of what you changed are in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildservers are happy. If not, fix in this order:
    • add a line in doc/news/_preparation_next_release.md
    • reformat the code with scripts/dev/reformat-all
    • make all unit tests pass
    • fix all memleaks
  • The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

Labels

If you are already Elektra developer:

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the basics are fulfilled and you also
    say that everything is ready to be merged.

@tucek tucek added this to the 0.9.5 milestone Mar 18, 2021
@tucek tucek linked an issue Mar 18, 2021 that may be closed by this pull request
6 tasks
@markus2330
Copy link
Contributor

Something went wrong in the rebase. Probably cherry-picking you own commits is the easiest way to resolve the problem here.

@tucek tucek force-pushed the 3713_javaBindingMavenToGradleMigration branch 4 times, most recently from 1cfb352 to 2c0469d Compare March 31, 2021 15:16
@markus2330 markus2330 mentioned this pull request Apr 6, 2021
6 tasks
@tucek tucek force-pushed the 3713_javaBindingMavenToGradleMigration branch from 19e2659 to 92a62b5 Compare April 6, 2021 16:39
Copy link
Member

@mpranj mpranj left a comment

Choose a reason for hiding this comment

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

I found some minor things in an initial review.

src/bindings/jna/libelektra/build.gradle Outdated Show resolved Hide resolved
scripts/docker/fedora/33/Dockerfile Outdated Show resolved Hide resolved
@tucek tucek force-pushed the 3713_javaBindingMavenToGradleMigration branch 2 times, most recently from 5436c90 to 22ad591 Compare April 7, 2021 16:21
@tucek
Copy link
Contributor Author

tucek commented Apr 8, 2021

The following issue may be closed when this PR is merged: #3269

@tucek tucek mentioned this pull request Apr 8, 2021
@tucek
Copy link
Contributor Author

tucek commented Apr 8, 2021

https://build.libelektra.org/blue/organizations/jenkins/libelektra/detail/PR-3714/43/pipeline failed with non-zero exit value 134 on `fedora-32-full´. Seems like a resource problem (e.g. insufficient RAM) to me. Any inputs?

@tucek tucek marked this pull request as ready for review April 8, 2021 02:14
@markus2330
Copy link
Contributor

jenkins build libelektra please

@markus2330
Copy link
Contributor

I cannot read from the output that it is about too little RAM*, SIGSEGV can be anything. As it happens in ksRewind, maybe we did something wrong in the JNA binding?

Btw. just because the tests run locally does not mean that there isn't a memory bug. Did you try to run with valgrind?

*Actually the machines have plenty of RAM (even i7 has 8GB).

@kodebach
Copy link
Member

kodebach commented Apr 8, 2021

ksRewind doesn't allocate any memory, so it can't be an out-of-memory problem. Looks more like an invalid pointer was passed to ksRewind.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Great job! So good to finally see this arriving.

@@ -157,25 +157,38 @@ you up to date with the multi-language support provided by Elektra.

### JNA

Since internal iterator support for `KeySet` is due to being dropped, the following methods have been removed:
- Since internal iterator support for `KeySet` is due to being dropped, the following methods have been removed:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also make the improvements of Java Bindings+publishing to maven a highlight.

### <<Binding2>>
- Renamed `KeyException` specializations: `KeyInvalidNameException`, `KeyTypeConversionException`, `KeyTypeMismatchException`

- Ongoing work on bringing the JNA binding up to scratch and improving developer experience. Both for JNA binding API consumers, as well as future JNA binding contrubutors. _(Michael Tucek)_
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this to a new section Outlook below.

doc/tutorials/python-kdb.md Outdated Show resolved Hide resolved
src/bindings/jna/README.md Outdated Show resolved Hide resolved
src/bindings/jna/README.md Outdated Show resolved Hide resolved
src/bindings/jna/gradlew Show resolved Hide resolved
return self.append(*list)

def remove(self, name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Problems with rebase?

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 you use merge instead of rebase, and some weird issues appear on github.

I keep my fork's master identical to upstream, and always rebase from master:

git checkout master
git fetch upstream master # assuming you have upstream set already
# do not do this if you ever work on your master branch
git reset --hard upstream/master # this is fine, I never work on master and I only want it to mirror upstream

git checkout mybranch
git rebase master

During this process you might have to fix some conflicts manually.
Maybe there are improvements to this process, but I think we should probably put this somewhere in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

I would also recommend, that the master branch of your fork is kept identical to that of the main repo.

I have set up my remotes like this (output of git remote -vv):

origin	git@github.com:kodebach/libelektra.git (fetch)
origin	git@github.com:kodebach/libelektra.git (push)
upstream	git@github.com:ElektraInitiative/libelektra (fetch)
upstream	git@github.com:ElektraInitiative/libelektra (push)

To update the master branch I do:

git fetch upstream master:master
git push origin master:master

This works without switching to the master branch first. After that you can do git rebase master to rebase the current branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kodebach can you copy what you wrote here to doc/GIT.md?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but do we really need a git tutorial? Doesn't Elektra have enough barely maintained docs already?

Copy link
Member

Choose a reason for hiding this comment

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

@kodebach can you copy what you wrote here to doc/GIT.md?

See #3769

@markus2330
Copy link
Contributor

markus2330 commented Apr 8, 2021

The ksRewind problem is reproducible. This needs to be fixed before merging this.

@kodebach
Copy link
Member

There also seems another problem being poped up: this night gradle released 7.0 and the macOS task fails with:

Aren't you using Gradle Wrapper? There should be no need to install gradle through homebrew. AFAIK the point of Gradle Wrapper is that it downloads a fixed version and uses that without the need to have a system-wide gradle install.

When i am isolating the org.libelektra.GOptsTest test (using the reproduction env above) i get the following error instead of the segfault (if i run all test again, then i get the segfault)

This is not an error. The GOptsTest adds some keys to the KDB at the start and remove them again at the end. If you get a segfault in between or the process is killed for some other reason, the keys are not removed. Because we cannot know, if the keys come from GOptsTest or were added by the user, the test simply fails, if keys are present below spec:/tests/java/gopts.

The fact that the errors alternate, seems to suggest that GOptsTest fails because of the existing keys, but also removes them. This would be a bug. If this Exception is triggered

throw new IllegalStateException ("Couldn't set up spec, keys exist!");

No further code should be executed by GOptsTest.

The segfault only seems to occur if -DPLUGINS=resolver_mf_xp_x;dump;sync;base64;spec;error;list;timeofday;profile;mathcheck;tracer;hosts;network;glob -DKDB_DEFAULT_RESOLVER=resolver_mf_xp_x
is used for building.

I seems to be triggered somewhere at the end of src/bindings/jna/libelektra/src/test/java/org/libelektra/GOptsTest.java setupSpec()...

This suggests that it happens during kdbSet() and only if the XDG resolver is used. If this indeed where the error occurs, I doubt it has anything to do with the Java binding. The Java binding could only be at fault, if ks is already broken (e.g. free'd) when kdb.set is called and I don't see how that could happen.

There is one more possibility. I noticed that KeySet has a finalize method that calls ksDel. If the garbage collector calls that during the kdb.set call this would cause problems. You should be able to detect that by adding a breakpoint to the finalize method to see if/when it is called (maybe add temporary System.out.print after kdb.set to know when it is finished).

@tucek
Copy link
Contributor Author

tucek commented Apr 10, 2021

@kodebach thank you for insights!

There also seems another problem being poped up: this night gradle released 7.0 and the macOS task fails with:

Aren't you using Gradle Wrapper? There should be no need to install gradle through homebrew. AFAIK the point of Gradle Wrapper is that it downloads a fixed version and uses that without the need to have a system-wide gradle install.

Gradle wrapper would download gradle each time a fresh container is used for building, therefore we decided to include the gradle binaries with the images.

When i am isolating the org.libelektra.GOptsTest test (using the reproduction env above) i get the following error instead of the segfault (if i run all test again, then i get the segfault)

This is not an error. The GOptsTest adds some keys to the KDB at the start and remove them again at the end. If you get a segfault in between or the process is killed for some other reason, the keys are not removed. Because we cannot know, if the keys come from GOptsTest or were added by the user, the test simply fails, if keys are present below spec:/tests/java/gopts.

That makes sense!

The fact that the errors alternate, seems to suggest that GOptsTest fails because of the existing keys, but also removes them. This would be a bug. If this Exception is triggered

throw new IllegalStateException ("Couldn't set up spec, keys exist!");

No further code should be executed by GOptsTest.

The segfault only seems to occur if -DPLUGINS=resolver_mf_xp_x;dump;sync;base64;spec;error;list;timeofday;profile;mathcheck;tracer;hosts;network;glob -DKDB_DEFAULT_RESOLVER=resolver_mf_xp_x
is used for building.
I seems to be triggered somewhere at the end of src/bindings/jna/libelektra/src/test/java/org/libelektra/GOptsTest.java setupSpec()...

This suggests that it happens during kdbSet() and only if the XDG resolver is used. If this indeed where the error occurs, I doubt it has anything to do with the Java binding. The Java binding could only be at fault, if ks is already broken (e.g. free'd) when kdb.set is called and I don't see how that could happen.

I concur with this conclusion, but the thing that bugs me is that the problem does not seem to occur on master.
And since i have not changed any C side stuff, the only difference i could imaging would be, that the GOptsTests is not executed in the xdg environment on master, but is now executed because i made a mistake in translating the test exceptions in the jna CMakeLists.txt. - The problem is, I do not see any exclusion for the GOptTests that apply for the failing environment...

There is one more possibility. I noticed that KeySet has a finalize method that calls ksDel. If the garbage collector calls that during the kdb.set call this would cause problems. You should be able to detect that by adding a breakpoint to the finalize method to see if/when it is called (maybe add temporary System.out.print after kdb.set to know when it is finished).

I am trying to narrow it down a little further right now.

@mpranj @kodebach If i continue to fail getting to the root of the problem, we would have to think what we should do about today's release. Disable GOptsTest and list it as known issue?

… be available before (if I'm interpreting the output correctly)
@kodebach
Copy link
Member

Gradle wrapper would download gradle each time a fresh container is used for building, therefore we decided to include the gradle binaries with the images.

Then you should probably make sure that the system-wide install of Gradle is the same version as the one configured via Gradle Wrapper. That means using e.g. brew install gradle@6.8.3 instead of brew install gradle. Otherwise you might get inconsistencies between local tests and build server tests.

but the thing that bugs me is that the problem does not seem to occur on master.

The memory bug might be present on master as well. But if it only occurs combination with the Java Binding, it won't be detected because the Java binding tests are not run with valgrind or ASAN, because you would always get false positive from the JVM.

Just because there is a memory bug does not necessarily mean there will be a segfault. That is danger of using a language with manual memory management.

If i continue to fail getting to the root of the problem, we would have to think what we should do about today's release. Disable GOptsTest and list it as known issue?

I can't make a decision about the release, but as long as we don't actually know the cause of the segfault, we probably shouldn't make a release.

@tucek
Copy link
Contributor Author

tucek commented Apr 10, 2021

I now can reproduce it locally with the defualt resolver set as mentioned above. Interestingly now at Problematic frame: C [libc.so.6+0x9d870] cfree+0x20...

I will now debug the JNA GOptsTests to narrow down what exact call to the native library triggers the problem down the line....

@tucek
Copy link
Contributor Author

tucek commented Apr 10, 2021

Gradle wrapper would download gradle each time a fresh container is used for building, therefore we decided to include the gradle binaries with the images.

Then you should probably make sure that the system-wide install of Gradle is the same version as the one configured via Gradle Wrapper. That means using e.g. brew install gradle@6.8.3 instead of brew install gradle. Otherwise you might get inconsistencies between local tests and build server tests.

It seems just removing the brew gradle does the trick. Gradle 6.8.3 seems to be installed prior to brew. Cmake includes JNA plugin, therefore gradle has been found. What image are these macOS builds based?

@kodebach
Copy link
Member

kodebach commented Apr 10, 2021

It seems just removing the brew gradle does the trick. Gradle 6.8.3 seems to be installed prior to brew.

I think you should still add it, in case the pre-installed Gradle version changes.

What image are these macOS builds based?

Cirrus CI uses Anka for it's macOS VMs and their configs can be found here: https://github.com/cirruslabs/osx-images

See also: https://cirrus-ci.org/guide/macOS/

@tucek
Copy link
Contributor Author

tucek commented Apr 10, 2021

It seems just removing the brew gradle does the trick. Gradle 6.8.3 seems to be installed prior to brew.

I think you should still add it, in case the pre-installed Gradle version changes.

this results in Error: No available formula or cask with the name "gradle@6.8.3"
I also think normally only enforcing a minimum gradle version is really necessary.

@tucek tucek force-pushed the 3713_javaBindingMavenToGradleMigration branch from 5172ac5 to 405e2b1 Compare April 10, 2021 16:22
@kodebach
Copy link
Member

this results in Error: No available formula or cask with the name "gradle@6.8.3"

I don't use Homebrew, possibly my quick Google search didn't give the correct way to install a fixed version.

I also think normally only enforcing a minimum gradle version is really necessary.

A minimum version isn't enough I think. Fixing a major version might be enough, but there are definitely Gradle releases with breaking changes (i.e. not backwards compatible) from time to time.

@tucek
Copy link
Contributor Author

tucek commented Apr 10, 2021

Final analysis from my side follows

Steps to reproduce

  • cmake with DPLUGINS=resolver_mf_xp_x;dump;sync;base64;spec;error;list;timeofday;profile;mathcheck;tracer;hosts;network;glob and DKDB_DEFAULT_RESOLVER=resolver_mf_xp_x
  • make install && ldconfig && cd src/bindings/jna
  • run GOptsTest (or run all tests via ./gradlew test)
  • remove left over test specs after crash: rm /usr/local/share/elektra/specification/default.ecf

The last call to the native library is triggered by try (final KDB kdb = KDB.open (contract, parentKey))
(https://github.com/tucek/libelektra/blob/405e2b19c70725fb19e4a8a15638cbbe17a7cc90/src/bindings/jna/libelektra/src/test/java/org/libelektra/GOptsTest.java#L69) only if the contract parameter is specified.
-> https://github.com/tucek/libelektra/blob/405e2b19c70725fb19e4a8a15638cbbe17a7cc90/src/bindings/jna/libelektra/src/main/java/org/libelektra/KDB.java#L56

I've included some JVM crash dumps jvm_crash_dumps.zip

I am afraid, at the moment, i cannot be of further help regarding this issue.

@tucek
Copy link
Contributor Author

tucek commented Apr 10, 2021

this results in Error: No available formula or cask with the name "gradle@6.8.3"

I don't use Homebrew, possibly my quick Google search didn't give the correct way to install a fixed version.

I also think normally only enforcing a minimum gradle version is really necessary.

A minimum version isn't enough I think. Fixing a major version might be enough, but there are definitely Gradle releases with breaking changes (i.e. not backwards compatible) from time to time.

I have opened ticket #3770 to track this issue.

@mpranj
Copy link
Member

mpranj commented Apr 11, 2021

Thank you all for working hard on this! ❤️
I'll take a look at this (segfault) today before doing the release, but I can't give a guarantee that I can fix it in this time frame.

Please ping me if anything changes in the mean time. (Fyi, I'll copy this PR to a different branch so I can test independently.)

@mpranj
Copy link
Member

mpranj commented Apr 11, 2021

I was able to get rid of the segfault by fixing the test exclusion mechanism.

The root cause was that the GOpts Tests were executed on a system where the gopts plugin was not compiled. dlopen() failed and caused a double free() as well in a "rare" execution path.

EDIT: I also think this has noting to do with ksRewind at all.

@mpranj mpranj merged commit dc6914e into ElektraInitiative:master Apr 11, 2021
@tucek tucek deleted the 3713_javaBindingMavenToGradleMigration branch April 12, 2021 08:04
Copy link
Member

@mpranj mpranj left a comment

Choose a reason for hiding this comment

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

Great work, nice to see such a refresh to the Java bindings.


Release versions of the Elektra JNA bindings are published to Maven Central with group id `org.libelektra`, artefact id `libelektra` and the same version as Elektra.

If you want to depend on a modified binding or just want to install it to your local maven repository, change your current working directory to the JNA binding folder `/src/bindings/jna`. Either use the bundled Gradle wrapper (`./gradlew` for Unix style OS or `./gradlew.bat` for windows) or ensure a recent Gradle version is available on your system and execute:
Copy link
Member

Choose a reason for hiding this comment

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

Is it recommended that we keep both the wrapper and also installed versions of gradle, or should we get rid of one of the two now?

Copy link
Member

Choose a reason for hiding this comment

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

Developers might have different Gradle versions installed. The wrapper is the recommended way to get consistent Gradle versions AFAIK.

Within CI and Docker it might be problematic (i.e. a waste of time) to let Gradle Wrapper download the Gradle binaries every time. If this can be avoided (e.g. via caching or multi-stage Docker builds), I think it would be preferable to use Gradle Wrapper everywhere. If this is not possible, then we should probably list all the places where Gradle Version is defined in a central location.

Copy link
Member

Choose a reason for hiding this comment

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

PS. for in the README I would only recommend using Gradle Wrapper, since that gives a consistent environment

@tucek tucek changed the title 3713 java binding maven to gradle migration [CM H2] 3713 java binding maven to gradle migration Apr 15, 2021
@tucek tucek added this to In progress in Java bindings overhaul via automation Aug 9, 2021
@tucek tucek moved this from In progress to Done in Java bindings overhaul Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants