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

Add temporary fix for finding jni plugin #2784

Merged
merged 4 commits into from Jun 19, 2019

Conversation

Projects
None yet
4 participants
@dmoisej
Copy link
Contributor

commented Jun 13, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

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
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see Coding Guidelines)
  • meta data is updated (e.g. README.md of plugins and METADATA.ini)

Review

Reviewers will usually check the following:

Labels

  • 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 build server is happy and also you
    say that everything is ready to be merged.

@markus2330 @sanssecours , please review my pull request

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@markus2330 @sanssecours
This issue is related to previous pull request: #2615 (finding jni plugin).

So I was asked to review the function "safe_check_symbol_exists". After some debugging I found out, that the removed lines created the problems by finding the jni.h library. I read, that it is used for testing and I believe, that my solution is not correct, because I believe, the testing should still be there. Can you recommend me something?

@dmoisej dmoisej changed the title Add temporary fix for finsing jni plugin Add temporary fix for finding jni plugin Jun 13, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Could you please describe (in an issue) what the problem you try to solve here is? In particular CMakeFiles/CMakeOutput.log should tell you why exactly the symbol is not found (what it tried to compile and what the error was).

@markus2330 markus2330 requested a review from kodebach Jun 14, 2019

@kodebach
Copy link
Contributor

left a comment

Not sure, why my review was requested. I have no idea what the CMake file in question does, so cannot comment on the changes.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Not sure, why my review was requested. I have no idea what the CMake file in question does, so cannot comment on the changes.

Because you suggested in #2615 that he should do what he tried to do in this PR.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Because you suggested in #2615 that he should do what he tried to do in this PR.

I still don't know anything about this CMake macro. I just noted that his change indicated that there is something wrong with this macro. Because, if we don't need the safe_check_symbol_exists macro, why would it exist?

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@kodebach @markus2330 , so what we should do with this?

As far as I understood
cmake_push_check_state (RESET)

is used for testing purposes but why is it there, when it doesn't work when installing Elektra with jni plugin.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

The way I read the documentation, cmake_push_check_state and cmake_push_check_state are used here, because the macro modifies CMAKE_REQUIRED_DEFINITIONS. Without this push/pop, the change would leak outside of the macro, where it might not be wanted.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

@dmoisej As already said before please describe (in an issue) what the problem you try to solve here is? In particular CMakeFiles/CMakeOutput.log should tell you why exactly the symbol is not found (what it tried to compile and what the error was).

Thank you! Otherwise we can do here only guesswork.

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@markus2330 @kodebach

The problem is, that I am trying to follow this guide in order to enable jni plugin for creating Java Plugins and install everything without problems and additional modifications in the code:
https://www.libelektra.org/plugins/jni

Specifically, I am talking about

Enabling the Plugin
section.

My steps:

  • git clone https://github.com/ElektraInitiative/libelektra.git
  • cd libelektra
  • mkdir build && cd build
  • cmake -DPLUGINS="ALL;-EXPERIMENTAL;jni" ..

On the last command I get error:

Exclude Plugin jni because jni.h does not define JNI_VERSION_1_8

but I defined everything and specified all the paths.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Thank you for creating the issue!

As already said two times, the information in CMakeFiles/CMakeOutput.log is essential.

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@markus2330 , oh sorry, here is the output regarding jni.h from the
/libelektra/build/CMakeFiles/CMakeOutput.log

Determining if the JNI_VERSION_1_8 exist passed with the following output:
Change Dir: /home/dmoisej/Projects/dmoisej/libelektra/build/CMakeFiles/CMakeTmp
Run Build Command:"/usr/bin/make" "cmTC_3a2dd/fast"
/usr/bin/make -f CMakeFiles/cmTC_3a2dd.dir/build.make CMakeFiles/cmTC_3a2dd.dir/build
make[1]: Entering directory '/home/dmoisej/Projects/dmoisej/libelektra/build/CMakeFiles/CMakeTmp'
Building C object CMakeFiles/cmTC_3a2dd.dir/CheckSymbolExists.c.o
/usr/bin/cc -I/usr/lib/jvm/default-java/include -I/usr/lib/jvm/default-java/include/linux -std=gnu99 -Wno-deprecated-declarations -Wstrict-prototypes -Wno-long-long -Wpedantic -Wno-variadic-macros -Wall -Wextra -Wno-overlength-strings -Wsign-compare -Wfloat-equal -Wformat -Wformat-security -Wshadow -Wcomments -Wtrigraphs -Wundef -Wuninitialized -Winit-self -Wsign-compare -Wfloat-equal -o CMakeFiles/cmTC_3a2dd.dir/CheckSymbolExists.c.o -c /home/dmoisej/Projects/dmoisej/libelektra/build/CMakeFiles/CMakeTmp/CheckSymbolExists.c
Linking C executable cmTC_3a2dd
/usr/local/bin/cmake -E cmake_link_script CMakeFiles/cmTC_3a2dd.dir/link.txt --verbose=1
/usr/bin/cc -std=gnu99 -Wno-deprecated-declarations -Wstrict-prototypes -Wno-long-long -Wpedantic -Wno-variadic-macros -Wall -Wextra -Wno-overlength-strings -Wsign-compare -Wfloat-equal -Wformat -Wformat-security -Wshadow -Wcomments -Wtrigraphs -Wundef -Wuninitialized -Winit-self -Wsign-compare -Wfloat-equal -rdynamic CMakeFiles/cmTC_3a2dd.dir/CheckSymbolExists.c.o -o cmTC_3a2dd
make[1]: Leaving directory '/home/dmoisej/Projects/dmoisej/libelektra/build/CMakeFiles/CMakeTmp'

File /home/dmoisej/Projects/dmoisej/libelektra/build/CMakeFiles/CMakeTmp/CheckSymbolExists.c:
/* */
#include <jni.h>

int main(int argc, char** argv)
{
(void)argv;
#ifndef JNI_VERSION_1_8
return ((int*)(&JNI_VERSION_1_8))[argc];
#else
(void)argc;
return 0;
#endif
}

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Are you this is the output of the failed build? (without this PR) It seems to have succeed?

Please everything together as a new issue: http://issues.libelektra.org/new

This would be much easier for other people that search for this issue.

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@markus2330 the thing is, that I am playing right now with the code and try to find the difference between working code and code, that fails, so maybe it is some old output. I need some time for debugging and at the end, when I will get another output, then I will let you know and will open the issue.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Thank you!

You should easily get the output when you start from a fresh build directory without any changes in the source dir.

@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@markus2330
So I removed all content from CMakeError.log and CMakeOutput.log.
After that I tried to execute the command:
cmake -DPLUGINS="ALL;jni;" ..
(it is without any changes to the master code)

As a result I have nothing in CMakeOutput.log, which concerns jni.
Here are my both files from build/CMakeFiles directory:

CMakeError.log
CMakeOutput.log

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Please follow instructions more carefully:

  1. create a new issue
  2. you need to remove the whole build directory, not only the log files (CMake does not recheck symbols it already checked)
@dmoisej

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

@kodebach , removing RESET as argument did help.
@kodebach @markus2330 please review my pull request as soon as possible

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

It works on the build server, and it works locally for you, so this looks fine to me.

@markus2330 markus2330 merged commit 6b9ea9e into ElektraInitiative:master Jun 19, 2019

10 of 12 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Great that we found a fix after all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.