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

The JNI version is defined now correctly during adding jni plugin with cmake #2615

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dmoisej
Copy link
Contributor

commented Apr 10, 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 doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

@kodebach kodebach added the cm2019s label Apr 10, 2019

@@ -16,7 +16,7 @@ if (DEPENDENCY_PHASE)
set (DIRS ${JNI_INCLUDE_DIRS} ${JAVA_INCLUDE_PATH} ${JAVA_INCLUDE_PATH2} ${JAVA_AWT_INCLUDE_PATH})
set (LIBS ${JAVA_MAWT_LIBRARY} ${JNI_LIBRARIES} ${JAVA_AWT_LIBRARY} ${JAVA_JVM_LIBRARY}) # for check_symbol_exists
set (CMAKE_REQUIRED_INCLUDES ${DIRS})
safe_check_symbol_exists (JNI_VERSION_1_8 jni.h JNI_CORRECT_VERSION)
check_symbol_exists (JNI_VERSION_1_8 jni.h JNI_CORRECT_VERSION)

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 10, 2019

Contributor

The description for safe_check_symbol_exists reads:

# ~~~
# Check if a symbol is exported
#
# Same as check_symbol_exists but works around the problem of not detecting
# symbols when -Wpedantic is on.
#
# Also automatically adds the definitions of the current directory to
# CMAKE_REQUIRED_DEFINITIONS.
#
# https://issues.libelektra.org/2218
# ~~~
macro (safe_check_symbol_exists SYMBOL FILES VARIABLE)

So it seems to me that safe_check_symbol_exists isn't working correctly, if this change actually fixed a bug.
Please check whether safe_check_symbol_exists has to be fixed as well.

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.