Skip to content

Attempt to move ByteBufProxy into the main codebase so it can be used. #96

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

Closed
wants to merge 2 commits into from

Conversation

buko
Copy link

@buko buko commented Jul 11, 2018

Goal

Move ByteBufProxy into the main package so that it can be used by non-test clients. See #90 for more details.

Changes

  1. Move ByteBufProxy into the main src code.
  2. Add some tests and update the tutorial.
  3. Narrow the netty dependencies.

Issues

mdbjava is fortunately a standard Maven project but I still ran into several issues when trying to make changes:

  1. The build fails inside Eclipse. The culprit is the buildnumber plugin that wants git on the command line.
[ERROR] Failed to execute goal org.codehaus.mojo:buildnumber-maven-plugin:1.4:create (default) on project lmdbjava: Cannot get the revision information from the scm repository : 
[ERROR] Exception while executing SCM command. Error while executing command. Error while executing process. Cannot run program "git" (in directory "C:\data\3rdcode\lmdbjava.git"): CreateProcess error=2, The system cannot find the file specified
[ERROR] -> [Help 1]

I've fixed this issue before in other projects many moons ago but forgot exactly how. I think it's possible to reconfigure the buildnumber plugin to use jgit instead of requiring the Git CLI. See https://github.com/alx3apps/jgit-buildnumber.

There are also alternatives to the buildnumber plugin that, for example, (eg https://github.com/release-engineering/buildmetadata-maven-plugin) that could be considered.

Was able to work around this issue by disabling the buildnumber plugin on the commandline/eclipse configuration.

  1. Ran into this error: "The type package-info is already defined"

This happens because the package org.lmdbjava in src/main/java and src/test/java define a 'package-info.java' file. Surprised the JDK allows this (it is a duplicate type) but Eclipse catches this and complains.

  1. The build blows up on Java10. Got it working by using JDK 1.8.0_92

  2. Ran into several issues with 'optional' dependencies. For now have made certain dependencies non-optional. Will need to investigate this further.

  3. The checkstyle issues are very harsh and fail the build. Admittedly don't a lot of time to see how to reproduce the desired code formatting rules in Eclipse. For now was able to disable the checkstyle plugin, using -Dcheckstyle.skip=true.

  4. Ran into PMD issues in an attempt to duplicate the tutorial test:

[INFO] PMD Failure: org.lmdbjava.TutorialTest:439 Rule:AvoidDuplicateLiterals Priority:3 The String literal "yyy" appears 4 times in this file; the first occurrence is on line 439.
[INFO] PMD Failure: org.lmdbjava.TutorialTest:442 Rule:AvoidDuplicateLiterals Priority:3 The String literal "ggg" appears 4 times in this file; the first occurrence is on line 442.

This was fixed by changing the tutorial test for Netty to use slightly different test values.

  1. For some very strange reason the act of building this project locally modifies the source code files. This may have to do with the maven license plugin. Not sure but unfortunately after making a few minor changes locally and building the project every file shows up as modified in this PR :(

Note that the only files actually modified were:

pom.xml
ByteBufProxy.java
ByteBufProxyTest.java
TutorialTest.java

Conclusion

By working around all the issues was able to build the project with the command: $ ~/data/applications/apache-maven-3.5.4/bin/mvn clean install -Dbuildnumber.phase=none -Dcheckstyle.skip=true

Installed a SNAPSHOT locally that has the desired changes: lmdbjava-0.6.2-SNAPSHOT.

More work is needed to actually get this merged. Will try to coordinate further with lmdbjava team around this PR.

@buko buko mentioned this pull request Jul 11, 2018
@buko
Copy link
Author

buko commented Jul 13, 2018

Could somebody take a look at this?

While this PR really fit for merging I thought it would be useful for an actual merge operation. At the least it would be good to provide some guidance here. @krisskross mentioned that tests were required, do we have more details on this?

@krisskross
Copy link
Contributor

Sorry for the delay @buko. Im on vacation an trying to disconnect a little. Give some time and ill have a look.

Thanks a lot for the contribution!

@benalexau
Copy link
Member

Can you please do a fresh pull from master, as I have made many changes today including numerous dependency and plugin version updates, ensuring LmdbJava can compile on Java 11 (see Travis and AppVeyor builds), fixing the Build Number Maven Plugin so that it uses JGit and noticing an issue when using any version of Netty above 4.1.29.

@benalexau benalexau added enhancement Additional or new functionality awaiting-response labels Dec 9, 2018
@benalexau
Copy link
Member

As discussed in #111 the Netty integration is currently not working, so I will close this PR. Please feel free to open a fresh PR when the tests pass with the latest Netty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additional or new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants